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

[Kees Cook <kees@xxxxxxxxxx>]

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

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)

-- 
Kees Cook