| Message ID | 20240814235635.7998-1-linkinjeon@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ksmbd: Use unsafe_memcpy() for ntlm_negotiate | expand |
This fixed the 'field spanning write' warnings for me. merged into ksmbd-for-next On Wed, Aug 14, 2024 at 6:56 PM Namjae Jeon <linkinjeon@kernel.org> wrote: > > rsp buffer is allocatged larger than spnego_blob from > smb2_allocate_rsp_buf(). > > Signed-off-by: Namjae Jeon <linkinjeon@kernel.org> > --- > 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); > } > -- > 2.25.1 >
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@kernel.org> > --- > 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 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], ...) 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)
On Mon, Sep 23, 2024 at 8:12 AM Kees Cook <kees@kernel.org> 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@kernel.org> > > --- > > 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
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@kernel.org> 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@kernel.org> > > > --- > > > 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. :)
On Mon, Sep 23, 2024 at 9:27 AM Kees Cook <kees@kernel.org> 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@kernel.org> 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@kernel.org> > > > > --- > > > > 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
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); }
rsp buffer is allocatged larger than spnego_blob from smb2_allocate_rsp_buf(). Signed-off-by: Namjae Jeon <linkinjeon@kernel.org> --- fs/smb/server/smb2pdu.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)