diff mbox series

[v3] ksmbd: validate length in smb2_write()

Message ID 20220504224640.2865787-1-mmakassikis@freebox.fr
State New
Headers show
Series [v3] ksmbd: validate length in smb2_write() | expand

Commit Message

Marios Makassikis May 4, 2022, 10:46 p.m. UTC
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.

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(-)

Comments

Namjae Jeon May 5, 2022, 9:39 p.m. UTC | #1
2022-05-05 7:46 GMT+09: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.
>
> Signed-off-by: Marios Makassikis <mmakassikis@freebox.fr>
> ---
> Change since v2:
Hi Marios,
>  - the length check was wrong, as it did not account for the rfc1002
>  header in work->request_buf.
We get pdu size in ->request_buf using get_rfc1002_len(). We don't
need to account for it. So v2 patch is correct.

Thanks for your patch:)
>
>  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
>
>
Hyunchul Lee May 5, 2022, 11:42 p.m. UTC | #2
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
Marios Makassikis May 6, 2022, 5:34 a.m. UTC | #3
On Fri, May 6, 2022 at 1:42 AM Hyunchul Lee <hyc.lee@gmail.com> wrote:
>
> 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?
>

Hello Hyunchul, Namjae,

You are both right, v3 is not necessary. My confusion came from
looking at earlier code (without the changes to smb2_hdr) rather
than the latest tree.

Thanks for checking it.

Marios
diff mbox series

Patch

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)