[PATCH v3] fs: fix undefined behavior in bit shift for SB_NOUSER

[PATCH v3] fs: fix undefined behavior in bit shift for SB_NOUSER

[Gaosheng Cui]

Shifting signed 32-bit value by 31 bits is undefined, so changing most
significant bit to unsigned, and mark all of the flags as unsigned so
that we don't mix types. The UBSAN warning calltrace like below:

UBSAN: shift-out-of-bounds in fs/namespace.c:2330:33
left shift of 1 by 31 places cannot be represented in type 'int'
Call Trace:
 <TASK>
 dump_stack_lvl+0x7d/0xa5
 dump_stack+0x15/0x1b
 ubsan_epilogue+0xe/0x4e
 __ubsan_handle_shift_out_of_bounds+0x1e7/0x20c
 graft_tree+0x36/0xf0
 do_add_mount+0x98/0x100
 path_mount+0xbd6/0xd50
 init_mount+0x6a/0xa3
 devtmpfs_setup+0x47/0x7e
 devtmpfsd+0x1a/0x50
 kthread+0x126/0x160
 ret_from_fork+0x1f/0x30
 </TASK>

Fixes: e462ec50cb5f ("VFS: Differentiate mount flags (MS_*) from internal superblock flags")
Signed-off-by: Gaosheng Cui <cuigaosheng1@huawei.com>
---
v3:
- convert SB_XX to (1U << n), keep a consistent code style, thanks! 
v2:
- Mark all of the flags as unsigned instead of just one so that
- we don't mix types, and add the proper whitespaces around the
- shift operator everywhere, thanks!
 include/linux/fs.h | 42 +++++++++++++++++++++---------------------
 1 file changed, 21 insertions(+), 21 deletions(-)

diff --git a/include/linux/fs.h b/include/linux/fs.h
index fa5ba1b1cbcd..7cd7bc761fa9 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1374,29 +1374,29 @@ extern int send_sigurg(struct fown_struct *fown);
  * sb->s_flags.  Note that these mirror the equivalent MS_* flags where
  * represented in both.
  */
-#define SB_RDONLY	 1	/* Mount read-only */
-#define SB_NOSUID	 2	/* Ignore suid and sgid bits */
-#define SB_NODEV	 4	/* Disallow access to device special files */
-#define SB_NOEXEC	 8	/* Disallow program execution */
-#define SB_SYNCHRONOUS	16	/* Writes are synced at once */
-#define SB_MANDLOCK	64	/* Allow mandatory locks on an FS */
-#define SB_DIRSYNC	128	/* Directory modifications are synchronous */
-#define SB_NOATIME	1024	/* Do not update access times. */
-#define SB_NODIRATIME	2048	/* Do not update directory access times */
-#define SB_SILENT	32768
-#define SB_POSIXACL	(1<<16)	/* VFS does not apply the umask */
-#define SB_INLINECRYPT	(1<<17)	/* Use blk-crypto for encrypted files */
-#define SB_KERNMOUNT	(1<<22) /* this is a kern_mount call */
-#define SB_I_VERSION	(1<<23) /* Update inode I_version field */
-#define SB_LAZYTIME	(1<<25) /* Update the on-disk [acm]times lazily */
+#define SB_RDONLY	(1U)		/* Mount read-only */
+#define SB_NOSUID	(1U << 1)	/* Ignore suid and sgid bits */
+#define SB_NODEV	(1U << 2)	/* Disallow access to device special files */
+#define SB_NOEXEC	(1U << 3)	/* Disallow program execution */
+#define SB_SYNCHRONOUS	(1U << 4)	/* Writes are synced at once */
+#define SB_MANDLOCK	(1U << 6)	/* Allow mandatory locks on an FS */
+#define SB_DIRSYNC	(1U << 7)	/* Directory modifications are synchronous */
+#define SB_NOATIME	(1U << 10)	/* Do not update access times. */
+#define SB_NODIRATIME	(1U << 11)	/* Do not update directory access times */
+#define SB_SILENT	(1U << 15)
+#define SB_POSIXACL	(1U << 16)	/* VFS does not apply the umask */
+#define SB_INLINECRYPT	(1U << 17)	/* Use blk-crypto for encrypted files */
+#define SB_KERNMOUNT	(1U << 22)	/* this is a kern_mount call */
+#define SB_I_VERSION	(1U << 23)	/* Update inode I_version field */
+#define SB_LAZYTIME	(1U << 25)	/* Update the on-disk [acm]times lazily */
 
 /* These sb flags are internal to the kernel */
-#define SB_SUBMOUNT     (1<<26)
-#define SB_FORCE    	(1<<27)
-#define SB_NOSEC	(1<<28)
-#define SB_BORN		(1<<29)
-#define SB_ACTIVE	(1<<30)
-#define SB_NOUSER	(1<<31)
+#define SB_SUBMOUNT	(1U << 26)
+#define SB_FORCE	(1U << 27)
+#define SB_NOSEC	(1U << 28)
+#define SB_BORN		(1U << 29)
+#define SB_ACTIVE	(1U << 30)
+#define SB_NOUSER	(1U << 31)
 
 /* These flags relate to encoding and casefolding */
 #define SB_ENC_STRICT_MODE_FL	(1 << 0)
-- 
2.25.1

Re: [PATCH v3] fs: fix undefined behavior in bit shift for SB_NOUSER

[Matthew Wilcox]

On Mon, Oct 31, 2022 at 10:26:21PM +0800, Gaosheng Cui wrote:
> Shifting signed 32-bit value by 31 bits is undefined, so changing most
> significant bit to unsigned, and mark all of the flags as unsigned so
> that we don't mix types. The UBSAN warning calltrace like below:
> 
> UBSAN: shift-out-of-bounds in fs/namespace.c:2330:33
> left shift of 1 by 31 places cannot be represented in type 'int'
> Call Trace:
>  <TASK>
>  dump_stack_lvl+0x7d/0xa5
>  dump_stack+0x15/0x1b
>  ubsan_epilogue+0xe/0x4e
>  __ubsan_handle_shift_out_of_bounds+0x1e7/0x20c
>  graft_tree+0x36/0xf0
>  do_add_mount+0x98/0x100
>  path_mount+0xbd6/0xd50
>  init_mount+0x6a/0xa3
>  devtmpfs_setup+0x47/0x7e
>  devtmpfsd+0x1a/0x50
>  kthread+0x126/0x160
>  ret_from_fork+0x1f/0x30
>  </TASK>
> 
> Fixes: e462ec50cb5f ("VFS: Differentiate mount flags (MS_*) from internal superblock flags")
> Signed-off-by: Gaosheng Cui <cuigaosheng1@huawei.com>

Reviewed-by: Matthew Wilcox (Oracle) <willy@infradead.org>

Re: [PATCH v3] fs: fix undefined behavior in bit shift for SB_NOUSER

[Al Viro]

On Mon, Oct 31, 2022 at 10:26:21PM +0800, Gaosheng Cui wrote:
> Shifting signed 32-bit value by 31 bits is undefined, so changing most
> significant bit to unsigned, and mark all of the flags as unsigned so
> that we don't mix types. The UBSAN warning calltrace like below:

... is completely irrelevant.  If you want to credit UBSAN for finding
that - sure, no problem, but who cares about the call chain leading to
use of SB_NOUSER?  If nothing else, report would be just as valid if
the only use had been in initializer for static variable...

I've no problem with the change itself, but I'd go with something along
the lines of

----
SB_... flags are masks for struct super_block ->s_flags, which is an
unsigned long.  In particular, SB_NOUSER lives in bit 31.  1 << 31
is not the right way to spell that - it's not just that it is an
undefined behaviour; it's not the right value when used to initialize
an unsigned long variable.  Nasal demons aside, on 64bit architecture
	unsigned long v = 1 << 31;
is *not* going to have v equal to 0x80000000 - it'll be 0xffffffff80000000.

Make the entire bunch explicitly unsigned - 1U << constant.  Note that
it doesn't make sense to go for 1UL - the flags are arch-independent
and that limits us to 32 bits anyway.

Spotted by UBSAN.
----

for commit message.  Not sure about "Fixes:", to be honest...

[PATCH V3] fs: fix undefined behavior in bit shift for SB_NOUSER

[Hao Ge]

Shifting signed 32-bit value by 31 bits is undefined, so changing
significant bit to unsigned. It was spotted by UBSAN.

Fixes: e462ec50cb5f ("VFS: Differentiate mount flags (MS_*) from internal superblock flags")
Signed-off-by: Hao Ge <gehao@kylinos.cn>
---

v2: add Fixes for changelog
v3: streamline changelog
---
 include/linux/fs.h | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/include/linux/fs.h b/include/linux/fs.h
index c85916e9f7db..86ab23a05b61 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1069,19 +1069,19 @@ extern int send_sigurg(struct fown_struct *fown);
 #define SB_NOATIME	1024	/* Do not update access times. */
 #define SB_NODIRATIME	2048	/* Do not update directory access times */
 #define SB_SILENT	32768
-#define SB_POSIXACL	(1<<16)	/* VFS does not apply the umask */
-#define SB_INLINECRYPT	(1<<17)	/* Use blk-crypto for encrypted files */
-#define SB_KERNMOUNT	(1<<22) /* this is a kern_mount call */
-#define SB_I_VERSION	(1<<23) /* Update inode I_version field */
-#define SB_LAZYTIME	(1<<25) /* Update the on-disk [acm]times lazily */
+#define SB_POSIXACL	(1U<<16)	/* VFS does not apply the umask */
+#define SB_INLINECRYPT	(1U<<17)	/* Use blk-crypto for encrypted files */
+#define SB_KERNMOUNT	(1U<<22) /* this is a kern_mount call */
+#define SB_I_VERSION	(1U<<23) /* Update inode I_version field */
+#define SB_LAZYTIME	(1U<<25) /* Update the on-disk [acm]times lazily */
 
 /* These sb flags are internal to the kernel */
-#define SB_SUBMOUNT     (1<<26)
-#define SB_FORCE    	(1<<27)
-#define SB_NOSEC	(1<<28)
-#define SB_BORN		(1<<29)
-#define SB_ACTIVE	(1<<30)
-#define SB_NOUSER	(1<<31)
+#define SB_SUBMOUNT     (1U<<26)
+#define SB_FORCE	(1U<<27)
+#define SB_NOSEC	(1U<<28)
+#define SB_BORN		(1U<<29)
+#define SB_ACTIVE	(1U<<30)
+#define SB_NOUSER	(1U<<31)
 
 /* These flags relate to encoding and casefolding */
 #define SB_ENC_STRICT_MODE_FL	(1 << 0)
-- 
2.25.1


No virus found
		Checked by Hillstone Network AntiVirus