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

[PATCH v2] 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>
---
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 | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/include/linux/fs.h b/include/linux/fs.h
index fa5ba1b1cbcd..0e92a85e212f 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1384,19 +1384,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

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

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

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

[Matthew Wilcox]

On Mon, Oct 31, 2022 at 09:48:11PM +0800, Gaosheng Cui wrote:
> +++ b/include/linux/fs.h
> @@ -1384,19 +1384,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

Shouldn't those ^^^ also be marked as unsigned?  And it's confusing to
have the style change halfway through the sequence; can you convert them
to (1U << n) as well?

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

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

[Christoph Hellwig]

On Mon, Oct 31, 2022 at 01:53:29PM +0000, Matthew Wilcox wrote:
> On Mon, Oct 31, 2022 at 09:48:11PM +0800, Gaosheng Cui wrote:
> > +++ b/include/linux/fs.h
> > @@ -1384,19 +1384,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
> 
> Shouldn't those ^^^ also be marked as unsigned?  And it's confusing to
> have the style change halfway through the sequence; can you convert them
> to (1U << n) as well?

I was planning on just sending a followup instead of blowing up the
scope even more, but yes - they should.

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

[cuigaosheng]

> Shouldn't those ^^^ also be marked as unsigned?  And it's confusing to
> have the style change halfway through the sequence; can you convert them
> to (1U << n) as well?

Thanks, I have made a patch v3 and submit it, but I'm not sure should I
add "Reviewed-by: Christoph Hellwig<hch@lst.de>"  because the code has been
changed,Thank you all again!

On 2022/10/31 21:53, Matthew Wilcox wrote:
> On Mon, Oct 31, 2022 at 09:48:11PM +0800, Gaosheng Cui wrote:
>> +++ b/include/linux/fs.h
>> @@ -1384,19 +1384,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
> Shouldn't those ^^^ also be marked as unsigned?  And it's confusing to
> have the style change halfway through the sequence; can you convert them
> to (1U << n) as well?
>
>> -#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
>>
> .

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

[Hao Ge]

> On Apr 24, 2023, at 13:02, Al Viro <viro@zeniv.linux.org.uk> wrote:
> 
> On Mon, Apr 24, 2023 at 12:51:22PM +0800, Hao Ge wrote:
>> Shifting signed 32-bit value by 31 bits is undefined, so changing
>> significant bit to unsigned. The UBSAN warning calltrace like below:
> 
>> UBSAN: shift-out-of-bounds in fs/nsfs.c:306:32
>> left shift of 1 by 31 places cannot be represented in type 'int'
>> CPU: 0 PID: 0 Comm: swapper/0 Not tainted 6.3.0-rc4+ #2
>> Call trace:
>> <TASK>
>> dump_backtrace+0x134/0x1e0
>> show_stack+0x2c/0x3c
>> dump_stack_lvl+0xb0/0xd4
>> dump_stack+0x14/0x1c
>> ubsan_epilogue+0xc/0x3c
>> __ubsan_handle_shift_out_of_bounds+0xb0/0x14c
>> nsfs_init+0x4c/0xb0
>> start_kernel+0x38c/0x738
>> __primary_switched+0xbc/0xc4
>> </TASK>
>> 
>> Fixes: e462ec50cb5f ("VFS: Differentiate mount flags (MS_*) from internal superblock flags")
>> Signed-off-by: Hao Ge <gehao@kylinos.cn>
> 
> *snort*
> 
> IMO something like "spotted by UBSAN" is more than enough here -
> stack trace is completely pointless.
> 
> Otherwise, no problems with the patch - it's obviously safe.
Thanks for taking time to review this patch.
I fully agree with your suggestion, as it is not just this one place that reported this error, although they are the same reason.
I will remove stack trace and send v3.

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

[Al Viro]

On Mon, Apr 24, 2023 at 12:51:22PM +0800, Hao Ge wrote:
> Shifting signed 32-bit value by 31 bits is undefined, so changing
> significant bit to unsigned. The UBSAN warning calltrace like below:

> UBSAN: shift-out-of-bounds in fs/nsfs.c:306:32
> left shift of 1 by 31 places cannot be represented in type 'int'
> CPU: 0 PID: 0 Comm: swapper/0 Not tainted 6.3.0-rc4+ #2
> Call trace:
> <TASK>
> dump_backtrace+0x134/0x1e0
> show_stack+0x2c/0x3c
> dump_stack_lvl+0xb0/0xd4
> dump_stack+0x14/0x1c
> ubsan_epilogue+0xc/0x3c
> __ubsan_handle_shift_out_of_bounds+0xb0/0x14c
> nsfs_init+0x4c/0xb0
> start_kernel+0x38c/0x738
> __primary_switched+0xbc/0xc4
> </TASK>
> 
> Fixes: e462ec50cb5f ("VFS: Differentiate mount flags (MS_*) from internal superblock flags")
> Signed-off-by: Hao Ge <gehao@kylinos.cn>

*snort*

IMO something like "spotted by UBSAN" is more than enough here -
stack trace is completely pointless.

Otherwise, no problems with the patch - it's obviously safe.

[PATCH V2] 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. The UBSAN warning calltrace like below:

UBSAN: shift-out-of-bounds in fs/nsfs.c:306:32
left shift of 1 by 31 places cannot be represented in type 'int'
CPU: 0 PID: 0 Comm: swapper/0 Not tainted 6.3.0-rc4+ #2
Call trace:
<TASK>
dump_backtrace+0x134/0x1e0
show_stack+0x2c/0x3c
dump_stack_lvl+0xb0/0xd4
dump_stack+0x14/0x1c
ubsan_epilogue+0xc/0x3c
__ubsan_handle_shift_out_of_bounds+0xb0/0x14c
nsfs_init+0x4c/0xb0
start_kernel+0x38c/0x738
__primary_switched+0xbc/0xc4
</TASK>

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

v2: add Fixes for 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