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. :)
--
Kees Cook
