[SMB3] enumerating snapshots was leaving part of the data off end

Message ID CAH2r5mv3GTwSa1_KuWtT2iK=T5Vmr3tqnLv6e80gfQ1GhBwfMQ@mail.gmail.com
State New
Headers show
Series
  • [SMB3] enumerating snapshots was leaving part of the data off end
Related show

Commit Message

Steve French Aug. 9, 2018, 6 a.m.
When enumerating snapshots, the last few bytes of the final
    snapshot could be left off since we were miscalculating the
    length returned (leaving off the sizeof struct SRV_SNAPSHOT_ARRAY)
    See MS-SMB2 section 2.2.32.2. In addition fixup the length used
    to allow smaller buffer to be passed in, in order to allow
    returning the size of the whole snapshot array more easily.

    CC: Stable <stable@vger.kernel.org>
    Signed-off-by: Steve French <stfrench@microsoft.com>

                   struct cifsFileInfo *cfile, void __user *ioc_buf)
@@ -1398,14 +1400,25 @@ smb3_enum_snapshots(const unsigned int xid,
struct cifs_tcon *tcon,
                        kfree(retbuf);
                        return rc;
                }
-               if (snapshot_in.snapshot_array_size < sizeof(struct
smb_snapshot_array)) {
-                       rc = -ERANGE;
-                       kfree(retbuf);
-                       return rc;
-               }

-               if (ret_data_len > snapshot_in.snapshot_array_size)
-                       ret_data_len = snapshot_in.snapshot_array_size;
+               /* check for min size, ie not large enough to fit even one GMT
+                * token (snapshot).  On the first ioctl some users may pass in
+                * smaller size (or zero) to simply get the size of the array
+                * so the user space caller can allocate sufficient memory
+                * and retry the ioctl again with larger array size sufficient
+                * to hold all of the snapshot GMT tokens on the second try.
+                */
+               if (snapshot_in.snapshot_array_size < GMT_TOKEN_SIZE)
+                       ret_data_len = sizeof(struct smb_snapshot_array);
+
+               /* we return struct SRV_SNAPSHOT_ARRAY, followed by
+                * the snapshot array (of 50 byte GMT tokens) each
+                * representing an available previous version of the data
+                */
+               if (ret_data_len > (snapshot_in.snapshot_array_size +
+                                       sizeof(struct smb_snapshot_array)))
+                       ret_data_len = snapshot_in.snapshot_array_size +
+                                       sizeof(struct smb_snapshot_array);

                if (copy_to_user(ioc_buf, retbuf, ret_data_len))
                        rc = -EFAULT;
(END)

Comments

Steve French Aug. 9, 2018, 6:32 a.m. | #1
Here is sample output running a userspace tool against the patched kernel
mounted to a Windows volume with two snapshots
(before the second snapshot would be missing a few bytes at the end)

# ~/enum-snapshots /mnt/file
press enter to issue the ioctl to retrieve snapshot information ...

Num snapshots: 2 Num returned: 2 Array Size: 102

Snapshot 0:@GMT-2018.06.30-19.34.17
Snapshot 1:@GMT-2018.06.30-19.33.37
On Thu, Aug 9, 2018 at 1:00 AM Steve French <smfrench@gmail.com> wrote:
>
>     When enumerating snapshots, the last few bytes of the final
>     snapshot could be left off since we were miscalculating the
>     length returned (leaving off the sizeof struct SRV_SNAPSHOT_ARRAY)
>     See MS-SMB2 section 2.2.32.2. In addition fixup the length used
>     to allow smaller buffer to be passed in, in order to allow
>     returning the size of the whole snapshot array more easily.
>
>     CC: Stable <stable@vger.kernel.org>
>     Signed-off-by: Steve French <stfrench@microsoft.com>
>
> diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
> index 15c7cbde2f39..abd6142e1b4a 100644
> --- a/fs/cifs/smb2ops.c
> +++ b/fs/cifs/smb2ops.c
> @@ -1369,6 +1369,8 @@ smb3_set_integrity(const unsigned int xid,
> struct cifs_tcon *tcon,
>
>  }
>
> +/* GMT Token is @GMT-YYYY.MM.DD-HH.MM.SS Unicode which is 48 bytes + null */
> +#define GMT_TOKEN_SIZE 50
>  static int
>  smb3_enum_snapshots(const unsigned int xid, struct cifs_tcon *tcon,
>                    struct cifsFileInfo *cfile, void __user *ioc_buf)
> @@ -1398,14 +1400,25 @@ smb3_enum_snapshots(const unsigned int xid,
> struct cifs_tcon *tcon,
>                         kfree(retbuf);
>                         return rc;
>                 }
> -               if (snapshot_in.snapshot_array_size < sizeof(struct
> smb_snapshot_array)) {
> -                       rc = -ERANGE;
> -                       kfree(retbuf);
> -                       return rc;
> -               }
>
> -               if (ret_data_len > snapshot_in.snapshot_array_size)
> -                       ret_data_len = snapshot_in.snapshot_array_size;
> +               /* check for min size, ie not large enough to fit even one GMT
> +                * token (snapshot).  On the first ioctl some users may pass in
> +                * smaller size (or zero) to simply get the size of the array
> +                * so the user space caller can allocate sufficient memory
> +                * and retry the ioctl again with larger array size sufficient
> +                * to hold all of the snapshot GMT tokens on the second try.
> +                */
> +               if (snapshot_in.snapshot_array_size < GMT_TOKEN_SIZE)
> +                       ret_data_len = sizeof(struct smb_snapshot_array);
> +
> +               /* we return struct SRV_SNAPSHOT_ARRAY, followed by
> +                * the snapshot array (of 50 byte GMT tokens) each
> +                * representing an available previous version of the data
> +                */
> +               if (ret_data_len > (snapshot_in.snapshot_array_size +
> +                                       sizeof(struct smb_snapshot_array)))
> +                       ret_data_len = snapshot_in.snapshot_array_size +
> +                                       sizeof(struct smb_snapshot_array);
>
>                 if (copy_to_user(ioc_buf, retbuf, ret_data_len))
>                         rc = -EFAULT;
> (END)
>
>
> --
> Thanks,
>
> Steve
Pavel Shilovsky Aug. 9, 2018, 5:45 p.m. | #2
2018-08-08 23:00 GMT-07:00 Steve French <smfrench@gmail.com>:
>     When enumerating snapshots, the last few bytes of the final
>     snapshot could be left off since we were miscalculating the
>     length returned (leaving off the sizeof struct SRV_SNAPSHOT_ARRAY)
>     See MS-SMB2 section 2.2.32.2. In addition fixup the length used
>     to allow smaller buffer to be passed in, in order to allow
>     returning the size of the whole snapshot array more easily.
>
>     CC: Stable <stable@vger.kernel.org>
>     Signed-off-by: Steve French <stfrench@microsoft.com>
>
> diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
> index 15c7cbde2f39..abd6142e1b4a 100644
> --- a/fs/cifs/smb2ops.c
> +++ b/fs/cifs/smb2ops.c
> @@ -1369,6 +1369,8 @@ smb3_set_integrity(const unsigned int xid,
> struct cifs_tcon *tcon,
>
>  }
>
> +/* GMT Token is @GMT-YYYY.MM.DD-HH.MM.SS Unicode which is 48 bytes + null */
> +#define GMT_TOKEN_SIZE 50
>  static int
>  smb3_enum_snapshots(const unsigned int xid, struct cifs_tcon *tcon,
>                    struct cifsFileInfo *cfile, void __user *ioc_buf)
> @@ -1398,14 +1400,25 @@ smb3_enum_snapshots(const unsigned int xid,
> struct cifs_tcon *tcon,
>                         kfree(retbuf);
>                         return rc;
>                 }
> -               if (snapshot_in.snapshot_array_size < sizeof(struct
> smb_snapshot_array)) {
> -                       rc = -ERANGE;
> -                       kfree(retbuf);
> -                       return rc;
> -               }
>
> -               if (ret_data_len > snapshot_in.snapshot_array_size)
> -                       ret_data_len = snapshot_in.snapshot_array_size;
> +               /* check for min size, ie not large enough to fit even one GMT

use kernel style for multi-line comments:

/*
 * comment
 */

> +                * token (snapshot).  On the first ioctl some users may pass in
> +                * smaller size (or zero) to simply get the size of the array
> +                * so the user space caller can allocate sufficient memory
> +                * and retry the ioctl again with larger array size sufficient
> +                * to hold all of the snapshot GMT tokens on the second try.
> +                */
> +               if (snapshot_in.snapshot_array_size < GMT_TOKEN_SIZE)
> +                       ret_data_len = sizeof(struct smb_snapshot_array);
> +
> +               /* we return struct SRV_SNAPSHOT_ARRAY, followed by

and here as well.

> +                * the snapshot array (of 50 byte GMT tokens) each
> +                * representing an available previous version of the data
> +                */
> +               if (ret_data_len > (snapshot_in.snapshot_array_size +
> +                                       sizeof(struct smb_snapshot_array)))
> +                       ret_data_len = snapshot_in.snapshot_array_size +
> +                                       sizeof(struct smb_snapshot_array);
>
>                 if (copy_to_user(ioc_buf, retbuf, ret_data_len))
>                         rc = -EFAULT;
> (END)
>
>
> --
> Thanks,
>
> Steve

Other than two comments above the patch looks good.

Acked-by: Pavel Shilovsky <pshilov@microsoft.com>

--
Best regards,
Pavel Shilovsky
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Steve French Aug. 9, 2018, 7:46 p.m. | #3
fixed comment (and added one more comment at head of function) and repushed
On Thu, Aug 9, 2018 at 12:49 PM Steve French <smfrench@gmail.com> wrote:
>
> Interesting that that is the one aspect of comment style which checkpatch doesn't seem to mind ... But no problem to change the patch
>
> On Thu, Aug 9, 2018, 12:45 Pavel Shilovsky <piastryyy@gmail.com> wrote:
>>
>> 2018-08-08 23:00 GMT-07:00 Steve French <smfrench@gmail.com>:
>> >     When enumerating snapshots, the last few bytes of the final
>> >     snapshot could be left off since we were miscalculating the
>> >     length returned (leaving off the sizeof struct SRV_SNAPSHOT_ARRAY)
>> >     See MS-SMB2 section 2.2.32.2. In addition fixup the length used
>> >     to allow smaller buffer to be passed in, in order to allow
>> >     returning the size of the whole snapshot array more easily.
>> >
>> >     CC: Stable <stable@vger.kernel.org>
>> >     Signed-off-by: Steve French <stfrench@microsoft.com>
>> >
>> > diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
>> > index 15c7cbde2f39..abd6142e1b4a 100644
>> > --- a/fs/cifs/smb2ops.c
>> > +++ b/fs/cifs/smb2ops.c
>> > @@ -1369,6 +1369,8 @@ smb3_set_integrity(const unsigned int xid,
>> > struct cifs_tcon *tcon,
>> >
>> >  }
>> >
>> > +/* GMT Token is @GMT-YYYY.MM.DD-HH.MM.SS Unicode which is 48 bytes + null */
>> > +#define GMT_TOKEN_SIZE 50
>> >  static int
>> >  smb3_enum_snapshots(const unsigned int xid, struct cifs_tcon *tcon,
>> >                    struct cifsFileInfo *cfile, void __user *ioc_buf)
>> > @@ -1398,14 +1400,25 @@ smb3_enum_snapshots(const unsigned int xid,
>> > struct cifs_tcon *tcon,
>> >                         kfree(retbuf);
>> >                         return rc;
>> >                 }
>> > -               if (snapshot_in.snapshot_array_size < sizeof(struct
>> > smb_snapshot_array)) {
>> > -                       rc = -ERANGE;
>> > -                       kfree(retbuf);
>> > -                       return rc;
>> > -               }
>> >
>> > -               if (ret_data_len > snapshot_in.snapshot_array_size)
>> > -                       ret_data_len = snapshot_in.snapshot_array_size;
>> > +               /* check for min size, ie not large enough to fit even one GMT
>>
>> use kernel style for multi-line comments:
>>
>> /*
>>  * comment
>>  */
>>
>> > +                * token (snapshot).  On the first ioctl some users may pass in
>> > +                * smaller size (or zero) to simply get the size of the array
>> > +                * so the user space caller can allocate sufficient memory
>> > +                * and retry the ioctl again with larger array size sufficient
>> > +                * to hold all of the snapshot GMT tokens on the second try.
>> > +                */
>> > +               if (snapshot_in.snapshot_array_size < GMT_TOKEN_SIZE)
>> > +                       ret_data_len = sizeof(struct smb_snapshot_array);
>> > +
>> > +               /* we return struct SRV_SNAPSHOT_ARRAY, followed by
>>
>> and here as well.
>>
>> > +                * the snapshot array (of 50 byte GMT tokens) each
>> > +                * representing an available previous version of the data
>> > +                */
>> > +               if (ret_data_len > (snapshot_in.snapshot_array_size +
>> > +                                       sizeof(struct smb_snapshot_array)))
>> > +                       ret_data_len = snapshot_in.snapshot_array_size +
>> > +                                       sizeof(struct smb_snapshot_array);
>> >
>> >                 if (copy_to_user(ioc_buf, retbuf, ret_data_len))
>> >                         rc = -EFAULT;
>> > (END)
>> >
>> >
>> > --
>> > Thanks,
>> >
>> > Steve
>>
>> Other than two comments above the patch looks good.
>>
>> Acked-by: Pavel Shilovsky <pshilov@microsoft.com>
>>
>> --
>> Best regards,
>> Pavel Shilovsky

Patch

From b505088cfa0c22b4cc1bbed08b72c9bdf16c8522 Mon Sep 17 00:00:00 2001
From: Steve French <stfrench@microsoft.com>
Date: Thu, 9 Aug 2018 00:51:34 -0500
Subject: [PATCH] smb3: enumerating snapshots was leaving part of the data off
 end

When enumerating snapshots, the last few bytes of the final
snapshot could be left off since we were miscalculating the
length returned (leaving off the sizeof struct SRV_SNAPSHOT_ARRAY)
See MS-SMB2 section 2.2.32.2. In addition fixup the length used
to allow smaller buffer to be passed in, in order to allow
returning the size of the whole snapshot array more easily.

CC: Stable <stable@vger.kernel.org>
Signed-off-by: Steve French <stfrench@microsoft.com>
---
 fs/cifs/smb2ops.c | 27 ++++++++++++++++++++-------
 1 file changed, 20 insertions(+), 7 deletions(-)

diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
index 15c7cbde2f39..abd6142e1b4a 100644
--- a/fs/cifs/smb2ops.c
+++ b/fs/cifs/smb2ops.c
@@ -1369,6 +1369,8 @@  smb3_set_integrity(const unsigned int xid, struct cifs_tcon *tcon,
 
 }
 
+/* GMT Token is @GMT-YYYY.MM.DD-HH.MM.SS Unicode which is 48 bytes + null */
+#define GMT_TOKEN_SIZE 50
 static int
 smb3_enum_snapshots(const unsigned int xid, struct cifs_tcon *tcon,
 		   struct cifsFileInfo *cfile, void __user *ioc_buf)
@@ -1398,14 +1400,25 @@  smb3_enum_snapshots(const unsigned int xid, struct cifs_tcon *tcon,
 			kfree(retbuf);
 			return rc;
 		}
-		if (snapshot_in.snapshot_array_size < sizeof(struct smb_snapshot_array)) {
-			rc = -ERANGE;
-			kfree(retbuf);
-			return rc;
-		}
 
-		if (ret_data_len > snapshot_in.snapshot_array_size)
-			ret_data_len = snapshot_in.snapshot_array_size;
+		/* check for min size, ie not large enough to fit even one GMT
+		 * token (snapshot).  On the first ioctl some users may pass in
+		 * smaller size (or zero) to simply get the size of the array
+		 * so the user space caller can allocate sufficient memory
+		 * and retry the ioctl again with larger array size sufficient
+		 * to hold all of the snapshot GMT tokens on the second try.
+		 */
+		if (snapshot_in.snapshot_array_size < GMT_TOKEN_SIZE)
+			ret_data_len = sizeof(struct smb_snapshot_array);
+
+		/* we return struct SRV_SNAPSHOT_ARRAY, followed by
+		 * the snapshot array (of 50 byte GMT tokens) each
+		 * representing an available previous version of the data
+		 */
+		if (ret_data_len > (snapshot_in.snapshot_array_size +
+					sizeof(struct smb_snapshot_array)))
+			ret_data_len = snapshot_in.snapshot_array_size +
+					sizeof(struct smb_snapshot_array);
 
 		if (copy_to_user(ioc_buf, retbuf, ret_data_len))
 			rc = -EFAULT;
-- 
2.17.1