On Mon, Sep 23, 2024 at 9:27 AM Kees Cook <kees@xxxxxxxxxx> wrote:
>
> On Mon, Sep 23, 2024 at 09:12:31AM +0900, Namjae Jeon wrote:
> > 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).
>
> Okay, in that case, please just use:
>
> memcpy(rsp->Buffer, ...)
>
> and the "unsafe" usage can be removed. :)
Okay, I will update it:)
Thanks!
>
> --
> Kees Cook
