Re: [PATCH] ksmbd: Use unsafe_memcpy() for ntlm_negotiate

[Namjae Jeon <linkinjeon@xxxxxxxxxx>]

On Mon, Sep 23, 2024 at 8:12 AM Kees Cook <kees@xxxxxxxxxx> wrote:
>
> On Thu, Aug 15, 2024 at 08:56:35AM +0900, Namjae Jeon wrote:
> > rsp buffer is allocatged larger than spnego_blob from
> > smb2_allocate_rsp_buf().
> >
> > Signed-off-by: Namjae Jeon <linkinjeon@xxxxxxxxxx>
> > ---
> >  fs/smb/server/smb2pdu.c | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/smb/server/smb2pdu.c b/fs/smb/server/smb2pdu.c
> > index 2df1354288e6..3f4c56a10a86 100644
> > --- a/fs/smb/server/smb2pdu.c
> > +++ b/fs/smb/server/smb2pdu.c
> > @@ -1370,7 +1370,8 @@ static int ntlm_negotiate(struct ksmbd_work *work,
> >       }
> >
> >       sz = le16_to_cpu(rsp->SecurityBufferOffset);
> > -     memcpy((char *)&rsp->hdr.ProtocolId + sz, spnego_blob, spnego_blob_len);
> > +     unsafe_memcpy((char *)&rsp->hdr.ProtocolId + sz, spnego_blob, spnego_blob_len,
> > +                     /* alloc is larger than blob, see smb2_allocate_rsp_buf() */);
> >       rsp->SecurityBufferLength = cpu_to_le16(spnego_blob_len);
> >
> >  out:
> > @@ -1453,7 +1454,9 @@ static int ntlm_authenticate(struct ksmbd_work *work,
> >                       return -ENOMEM;
> >
> >               sz = le16_to_cpu(rsp->SecurityBufferOffset);
> > -             memcpy((char *)&rsp->hdr.ProtocolId + sz, spnego_blob, spnego_blob_len);
> > +             unsafe_memcpy((char *)&rsp->hdr.ProtocolId + sz, spnego_blob,
> > +                             spnego_blob_len,
> > +                             /* alloc is larger than blob, see smb2_allocate_rsp_buf() */);
> >               rsp->SecurityBufferLength = cpu_to_le16(spnego_blob_len);
> >               kfree(spnego_blob);
> >       }
>
> Ew, please fix these properly instead of continuing to lie to the compiler
> about the struct member sizes. :P
I have fully checked that this warning is a false alarm.
>
> The above, &rsp->hdr.ProtocolId, addresses a single __le32 member, and
> then tries to go past the end of it (i.e. "sz" is larger than 4). The
> struct layout therefore indicates to memcpy that you have no bytes left
> to write ("size 0" in the warning).
>
> It looks to me like you want to be calculating an offset into
> rsp->Buffer instead? Try:
>
>         sz = le16_to_cpu(rsp->SecurityBufferOffset) -
>                 offsetof(*typeof(rsp), Buffer);
>         memcpy(&rsp->Buffer[sz], ...)
SecurityBufferOffset is fixed to the value 72 and it points to ->Buffer.
So memcpy(&rsp->Buffer[0], ...)
>
> BTW, what is validating that this:
>
>         sz = le16_to_cpu(rsp->SecurityBufferOffset);
>         chgblob =
>                 (struct challenge_message *)((char *)&rsp->hdr.ProtocolId + sz);
>
> is within the original data buffer? Shouldn't something check that:
>          sz > offsetof(*typeof(rsp), Buffer)
> and
>         sz <= ...size of the buffer... (maybe that happened already earlier)
SecurityBufferOffset is fixed to the value 72. It is set in smb2_sess_setup().

int smb2_sess_setup(struct ksmbd_work *work)
...
        rsp->StructureSize = cpu_to_le16(9);
        rsp->SessionFlags = 0;
        rsp->SecurityBufferOffset = cpu_to_le16(72);
        rsp->SecurityBufferLength = 0;

So sz and offsetof(*typeof(rsp), Buffer) will be same.
and rsp buffer size is at least 448 bytes,  That's enough space to
contain a chgblob(48) or spnego_blob(249).

Thanks.
>
> --
> Kees Cook