Re: [PATCH v3] ksmbd: validate length in smb2_write()

[Hyunchul Lee <hyc.lee@gmail.com>]

Hello Marios,

2022년 5월 5일 (목) 오후 4:00, Marios Makassikis <mmakassikis@freebox.fr>님이 작성:

>
> The SMB2 Write packet contains data that is to be written
> to a file or to a pipe. Depending on the client, there may
> be padding between the header and the data field.
> Currently, the length is validated only in the case padding
> is present.
>
> Since the DataOffset field always points to the beginning
> of the data, there is no need to have a special case for
> padding. By removing this, the length is validated in both
> cases.
>
> Additionally, fix the length check: DataOffset and Length
> fields are relative to the SMB header start, while the packet
> length returned by get_rfc1002_len() includes 4 additional
> bytes.
>

get_rfc1002_len doesn't include additional 4 bytes.
Can you check it again?

> Signed-off-by: Marios Makassikis <mmakassikis@freebox.fr>
> ---
> Change since v2:
>  - the length check was wrong, as it did not account for the rfc1002
>  header in work->request_buf.
>
>  fs/ksmbd/smb2pdu.c | 49 ++++++++++++++++++----------------------------
>  1 file changed, 19 insertions(+), 30 deletions(-)
>
> diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
> index 16c803a9d996..23b47e505e2b 100644
> --- a/fs/ksmbd/smb2pdu.c
> +++ b/fs/ksmbd/smb2pdu.c
> @@ -6328,23 +6328,18 @@ static noinline int smb2_write_pipe(struct ksmbd_work *work)
>         length = le32_to_cpu(req->Length);
>         id = req->VolatileFileId;
>
> -       if (le16_to_cpu(req->DataOffset) ==
> -           offsetof(struct smb2_write_req, Buffer)) {
> -               data_buf = (char *)&req->Buffer[0];
> -       } else {
> -               if ((u64)le16_to_cpu(req->DataOffset) + length >
> -                   get_rfc1002_len(work->request_buf)) {
> -                       pr_err("invalid write data offset %u, smb_len %u\n",
> -                              le16_to_cpu(req->DataOffset),
> -                              get_rfc1002_len(work->request_buf));
> -                       err = -EINVAL;
> -                       goto out;
> -               }
> -
> -               data_buf = (char *)(((char *)&req->hdr.ProtocolId) +
> -                               le16_to_cpu(req->DataOffset));
> +       if ((u64)le16_to_cpu(req->DataOffset) + length >
> +           get_rfc1002_len(work->request_buf) - 4) {
> +               pr_err("invalid write data offset %u, smb_len %u\n",
> +                      le16_to_cpu(req->DataOffset),
> +                      get_rfc1002_len(work->request_buf));
> +               err = -EINVAL;
> +               goto out;
>         }
>
> +       data_buf = (char *)(((char *)&req->hdr.ProtocolId) +
> +                          le16_to_cpu(req->DataOffset));
> +
>         rpc_resp = ksmbd_rpc_write(work->sess, id, data_buf, length);
>         if (rpc_resp) {
>                 if (rpc_resp->flags == KSMBD_RPC_ENOTIMPLEMENTED) {
> @@ -6489,22 +6484,16 @@ int smb2_write(struct ksmbd_work *work)
>
>         if (req->Channel != SMB2_CHANNEL_RDMA_V1 &&
>             req->Channel != SMB2_CHANNEL_RDMA_V1_INVALIDATE) {
> -               if (le16_to_cpu(req->DataOffset) ==
> -                   offsetof(struct smb2_write_req, Buffer)) {
> -                       data_buf = (char *)&req->Buffer[0];
> -               } else {
> -                       if ((u64)le16_to_cpu(req->DataOffset) + length >
> -                           get_rfc1002_len(work->request_buf)) {
> -                               pr_err("invalid write data offset %u, smb_len %u\n",
> -                                      le16_to_cpu(req->DataOffset),
> -                                      get_rfc1002_len(work->request_buf));
> -                               err = -EINVAL;
> -                               goto out;
> -                       }
> -
> -                       data_buf = (char *)(((char *)&req->hdr.ProtocolId) +
> -                                       le16_to_cpu(req->DataOffset));
> +               if ((u64)le16_to_cpu(req->DataOffset) + length >
> +                   get_rfc1002_len(work->request_buf) - 4) {
> +                       pr_err("invalid write data offset %u, smb_len %u\n",
> +                              le16_to_cpu(req->DataOffset),
> +                              get_rfc1002_len(work->request_buf));
> +                       err = -EINVAL;
> +                       goto out;
>                 }
> +               data_buf = (char *)(((char *)&req->hdr.ProtocolId) +
> +                                   le16_to_cpu(req->DataOffset));
>
>                 ksmbd_debug(SMB, "flags %u\n", le32_to_cpu(req->Flags));
>                 if (le32_to_cpu(req->Flags) & SMB2_WRITEFLAG_WRITE_THROUGH)
> --
> 2.25.1
>


--
Thanks,
Hyunchul