Re: [PATCH] nvmet: fix heap out-of-bounds read in nvmet_auth_negotiate()

On Tue, Aug 04, 2026 at 11:38:00AM +0800, Guixin Liu wrote:
> nvmet_execute_auth_send() allocates the DH-HMAC-CHAP message buffer with
> the host-supplied transfer length (tl) and hands it to
> nvmet_auth_negotiate() without passing tl along. nvmet_auth_negotiate()
> then reads the negotiate header and, for each of the halen hash
> identifiers and dhlen DH group identifiers, indexes into the fixed
> idlist[60] array (hashes at idlist[0..halen), groups at idlist[30..]).
> 
> Neither the transfer length nor halen/dhlen is validated. A malicious or
> non-conformant host can report a tl smaller than the negotiate structure,
> or a halen/dhlen larger than the array (both are u8, up to 255), making
> the loops read past the end of the allocated buffer (heap out-of-bounds
> read). The sibling nvmet_auth_reply() already validates tl against the
> structure size; the negotiate path did not.
> 
> Pass tl into nvmet_auth_negotiate(), reject a tl that does not cover the
> negotiate data plus one full protocol descriptor, and reject halen/dhlen
> larger than NVME_AUTH_DHCHAP_MAX_DH_IDS.
> 
> Fixes: db1312dd9548 ("nvmet: implement basic In-Band Authentication")
> Signed-off-by: Guixin Liu <[email protected]>
> ---
>  drivers/nvme/target/fabrics-cmd-auth.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/nvme/target/fabrics-cmd-auth.c b/drivers/nvme/target/fabrics-cmd-auth.c
> index d1b39e64d877..d015968bae2d 100644
> --- a/drivers/nvme/target/fabrics-cmd-auth.c
> +++ b/drivers/nvme/target/fabrics-cmd-auth.c
> @@ -31,12 +31,16 @@ void nvmet_auth_sq_init(struct nvmet_sq *sq)
>  	sq->dhchap_step = NVME_AUTH_DHCHAP_MESSAGE_NEGOTIATE;
>  }
>  
> -static u8 nvmet_auth_negotiate(struct nvmet_req *req, void *d)
> +static u8 nvmet_auth_negotiate(struct nvmet_req *req, void *d, u32 tl)
>  {
>  	struct nvmet_ctrl *ctrl = req->sq->ctrl;
>  	struct nvmf_auth_dhchap_negotiate_data *data = d;
>  	int i, hash_id = 0, fallback_hash_id = 0, dhgid, fallback_dhgid;
>  
> +	if (tl < sizeof(*data) +
> +		 sizeof(struct nvmf_auth_dhchap_protocol_descriptor))

Odd formatting here.  Condition continuations either get two-tab indents
or after the brace:

	if (tl < sizeof(*data) +
			sizeof(struct nvmf_auth_dhchap_protocol_descriptor))

	if (tl <
	   sizeof(*data) + sizeof(struct nvmf_auth_dhchap_protocol_descriptor))

Otherwise looks good:

Reviewed-by: Christoph Hellwig <[email protected]>