All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v3] ksmbd: validate length in smb2_write()
@ 2022-05-04 22:46 Marios Makassikis
  2022-05-05 21:39 ` Namjae Jeon
  2022-05-05 23:42 ` Hyunchul Lee
  0 siblings, 2 replies; 4+ messages in thread
From: Marios Makassikis @ 2022-05-04 22:46 UTC (permalink / raw)
  To: linux-cifs; +Cc: Marios Makassikis

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

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


^ permalink raw reply related	[flat|nested] 4+ messages in thread
* Re: [PATCH v3] ksmbd: validate length in smb2_write()
  2022-05-04 22:46 [PATCH v3] ksmbd: validate length in smb2_write() Marios Makassikis
@ 2022-05-05 21:39 ` Namjae Jeon
  2022-05-05 23:42 ` Hyunchul Lee
  1 sibling, 0 replies; 4+ messages in thread
From: Namjae Jeon @ 2022-05-05 21:39 UTC (permalink / raw)
  To: Marios Makassikis; +Cc: linux-cifs

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

^ permalink raw reply	[flat|nested] 4+ messages in thread
* Re: [PATCH v3] ksmbd: validate length in smb2_write()
  2022-05-04 22:46 [PATCH v3] ksmbd: validate length in smb2_write() Marios Makassikis
  2022-05-05 21:39 ` Namjae Jeon
@ 2022-05-05 23:42 ` Hyunchul Lee
  2022-05-06  5:34   ` Marios Makassikis
  1 sibling, 1 reply; 4+ messages in thread
From: Hyunchul Lee @ 2022-05-05 23:42 UTC (permalink / raw)
  To: Marios Makassikis; +Cc: linux-cifs

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

^ permalink raw reply	[flat|nested] 4+ messages in thread
* Re: [PATCH v3] ksmbd: validate length in smb2_write()
  2022-05-05 23:42 ` Hyunchul Lee
@ 2022-05-06  5:34   ` Marios Makassikis
  0 siblings, 0 replies; 4+ messages in thread
From: Marios Makassikis @ 2022-05-06  5:34 UTC (permalink / raw)
  To: Hyunchul Lee; +Cc: linux-cifs

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

^ permalink raw reply	[flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-05-06  5:35 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-04 22:46 [PATCH v3] ksmbd: validate length in smb2_write() Marios Makassikis
2022-05-05 21:39 ` Namjae Jeon
2022-05-05 23:42 ` Hyunchul Lee
2022-05-06  5:34   ` Marios Makassikis

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.