Re: [PATCH] nvmet: auth: validate dhchap id list lengths(KASAN: slab-out-of-bounds)

[Chris Leech <cleech@redhat.com>]

On Wed, Mar 11, 2026 at 04:06:44AM +0900, yunje shin wrote:
> Thank you for the clarification regarding the 64-byte structural
> constraints. If this approach looks good to you, I will format it
> properly with an updated commit message and send out a formal v2
> patch.

Yes, that looks much better to me.  Thanks!

- Chris

> diff --git a/drivers/nvme/target/fabrics-cmd-auth.c
> b/drivers/nvme/target/fabrics-cmd-auth.c
> index 5946681cb0e3..acba4878a873 100644
> --- a/drivers/nvme/target/fabrics-cmd-auth.c
> +++ b/drivers/nvme/target/fabrics-cmd-auth.c
> @@ -72,6 +72,14 @@ static u8 nvmet_auth_negotiate(struct nvmet_req
> *req, void *d)
>       NVME_AUTH_DHCHAP_AUTH_ID)
>   return NVME_AUTH_DHCHAP_FAILURE_INCORRECT_PAYLOAD;
> 
> + /*
> + * idlist[0..29]: hash IDs
> + * idlist[30..59]: DH group IDs
> + */
> + if (data->auth_protocol[0].dhchap.halen > NVME_AUTH_DHCHAP_MAX_HASH_IDS ||
> +     data->auth_protocol[0].dhchap.dhlen > NVME_AUTH_DHCHAP_MAX_DH_IDS)
> + return NVME_AUTH_DHCHAP_FAILURE_INCORRECT_PAYLOAD;
> +
>   for (i = 0; i < data->auth_protocol[0].dhchap.halen; i++) {
>   u8 host_hmac_id = data->auth_protocol[0].dhchap.idlist[i];
> 
> @@ -97,7 +105,7 @@ static u8 nvmet_auth_negotiate(struct nvmet_req
> *req, void *d)
>   dhgid = -1;
>   fallback_dhgid = -1;
>   for (i = 0; i < data->auth_protocol[0].dhchap.dhlen; i++) {
> - int tmp_dhgid = data->auth_protocol[0].dhchap.idlist[i + 30];
> + int tmp_dhgid = data->auth_protocol[0].dhchap.idlist[i +
> NVME_AUTH_DHCHAP_MAX_HASH_IDS];
> 
>   if (tmp_dhgid != ctrl->dh_gid) {
>   dhgid = tmp_dhgid;
> diff --git a/include/linux/nvme.h b/include/linux/nvme.h
> index b09dcaf5bcbc..ea0393ab16fc 100644
> --- a/include/linux/nvme.h
> +++ b/include/linux/nvme.h
> @@ -1824,6 +1824,8 @@ struct nvmf_auth_dhchap_protocol_descriptor {
>   __u8 dhlen;
>   __u8 idlist[60];
>  };
> +#define NVME_AUTH_DHCHAP_MAX_HASH_IDS 30
> +#define NVME_AUTH_DHCHAP_MAX_DH_IDS 30
> 
>  enum {
>   NVME_AUTH_DHCHAP_AUTH_ID = 0x01,
> -- 
> 2.43.0
> 
> Thanks
> Yunje Shin.
> 
> On Wed, Mar 11, 2026 at 3:07 AM Chris Leech <cleech@redhat.com> wrote:
> >
> > On Wed, Mar 11, 2026 at 02:52:36AM +0900, yunje shin wrote:
> > > Thanks for the review.
> > >
> > > Yes, I triggered the KASAN issue by injecting an invalid dhlen The
> > > reproduction steps are:
> > > 1. Connect to the NVMe/TCP target on port 4420 (ICReq + Fabrics CONNECT).
> > > 2. Send AUTH_SEND with a crafted NEGOTIATE payload where dhlen=200. 3.
> > > The kernel target code in nvmet_auth_negotiate() then iterates
> > >
> > >     for (i = 0; i < data->auth_protocol[0].dhchap.dhlen; i++) {
> > >         int tmp_dhgid = data->auth_protocol[0].dhchap.idlist[i + 30];
> > >
> > > With dhlen=200, this reads idlist[30..229], but idlist[] is only 60
> > > bytes (indices 0..59). The accesses at indices 60 and beyond read past
> > > the kmalloc'd slab object into adjacent slab memory, which KASAN
> > > catches as a slab-out-of-bounds read.
> >
> > Thank you, I appreciate understanding how this was triggered.
> >
> > > While the standard Linux NVMe host driver does use hardcoded halen=3
> > > and dhlen=6, the NVMe target is network-facing and must validate all
> > > fields from the wire. A malicious or non-standard host can send
> > > arbitrary values. The same applies to halen — if halen > 30, the
> > > first loop also reads out of bounds.
> >
> > Yes, this code absolutly should validate halen and dhlen bounds.
> >
> > > Regarding idlist_half — yes, idlist is currently a fixed 60-byte array
> > > and the DH offset is always 30. I derived it from sizeof(idlist)
> > > rather than hardcoding 30 so that the bounds check and the DH offset
> > > stay consistent with the array definition. If the struct ever changes,
> > > the validation adapts automatically instead of silently going stale.
> >
> > The 60-byte idlist (and 30:30 split) are part of the NVMe specification.
> > It's the maximum amount of space while keeping to a 64-byte struct.
> >
> > I'd rather see this made clearer with a define for the limit, but not
> > adding code that appears to calculate it at runtime.
> >
> > Thanks,
> > - Chris
> >
>