[PTACH] tmpfs: Fix undefined-behaviour in shmem_reconfigure()

[PTACH] tmpfs: Fix undefined-behaviour in shmem_reconfigure()

[Luo Meng]

When shmem_reconfigure() calls __percpu_counter_compare(), the second
parameter is unsigned int. But when define __percpu_counter_compare(),
the second parameter is signed int.
So the abs of count - rhs, UBSAN shows the following warning:

================================================================================
UBSAN: Undefined behaviour in lib/percpu_counter.c:209:6
signed integer overflow:
0 - -9223372036854775808 cannot be represented in type 'long long int'
CPU: 1 PID: 9636 Comm: syz-executor.2 Tainted: G                 ---------r-  - 4.18.0 #2
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.14.0-0-g155821a1990b-prebuilt.qemu.org 04/01/2014
Call Trace:
 __dump_stack home/install/linux-rh-3-10/lib/dump_stack.c:77 [inline]
 dump_stack+0x125/0x1ae home/install/linux-rh-3-10/lib/dump_stack.c:117
 ubsan_epilogue+0xe/0x81 home/install/linux-rh-3-10/lib/ubsan.c:159
 handle_overflow+0x19d/0x1ec home/install/linux-rh-3-10/lib/ubsan.c:190
 __percpu_counter_compare+0x124/0x140 home/install/linux-rh-3-10/lib/percpu_counter.c:209
 percpu_counter_compare home/install/linux-rh-3-10/./include/linux/percpu_counter.h:50 [inline]
 shmem_remount_fs+0x1ce/0x6b0 home/install/linux-rh-3-10/mm/shmem.c:3530
 do_remount_sb+0x11b/0x530 home/install/linux-rh-3-10/fs/super.c:888
 do_remount home/install/linux-rh-3-10/fs/namespace.c:2344 [inline]
 do_mount+0xf8d/0x26b0 home/install/linux-rh-3-10/fs/namespace.c:2844
 ksys_mount+0xad/0x120 home/install/linux-rh-3-10/fs/namespace.c:3075
 __do_sys_mount home/install/linux-rh-3-10/fs/namespace.c:3089 [inline]
 __se_sys_mount home/install/linux-rh-3-10/fs/namespace.c:3086 [inline]
 __x64_sys_mount+0xbf/0x160 home/install/linux-rh-3-10/fs/namespace.c:3086
 do_syscall_64+0xca/0x5c0 home/install/linux-rh-3-10/arch/x86/entry/common.c:298
 entry_SYSCALL_64_after_hwframe+0x6a/0xdf
RIP: 0033:0x46b5e9
Code: 5d db fa ff c3 66 2e 0f 1f 84 00 00 00 00 00 66 90 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 0f 83 2b db fa ff c3 66 2e 0f 1f 84 00 00 00 00
RSP: 002b:00007f54d5f22c68 EFLAGS: 00000246 ORIG_RAX: 00000000000000a5
RAX: ffffffffffffffda RBX: 000000000077bf60 RCX: 000000000046b5e9
RDX: 0000000000000000 RSI: 0000000020000000 RDI: 0000000000000000
RBP: 000000000077bf60 R08: 0000000020000140 R09: 0000000000000000
R10: 00000000026740a4 R11: 0000000000000246 R12: 0000000000000000
R13: 00007ffd1fb1592f R14: 00007f54d5f239c0 R15: 000000000077bf6c
================================================================================

Signed-off-by: Luo Meng <luomeng12@huawei.com>
---
 mm/shmem.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/mm/shmem.c b/mm/shmem.c
index 4b2fea33158e..15733c480318 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -3487,6 +3487,10 @@ static int shmem_reconfigure(struct fs_context *fc)
 
 	raw_spin_lock(&sbinfo->stat_lock);
 	inodes = sbinfo->max_inodes - sbinfo->free_inodes;
+	if (ctx->blocks > S64_MAX) {
+		err = "The number of blocks too large";
+		goto out;
+	}
 	if ((ctx->seen & SHMEM_SEEN_BLOCKS) && ctx->blocks) {
 		if (!sbinfo->max_blocks) {
 			err = "Cannot retroactively limit size";
-- 
2.31.1

Re: [PTACH] tmpfs: Fix undefined-behaviour in shmem_reconfigure()

[Andrew Morton]

On Fri, 13 May 2022 10:52:25 +0800 Luo Meng <luomeng12@huawei.com> wrote:

> When shmem_reconfigure() calls __percpu_counter_compare(), the second
> parameter is unsigned int.

Actually unsigned long long.

> But when define __percpu_counter_compare(),
> the second parameter is signed int.

Actually s64.

> So the abs of count - rhs, UBSAN shows the following warning:
> 

Here's my amended paragraph:

: When shmem_reconfigure() calls __percpu_counter_compare(), the second
: parameter is unsigned long long.  But in the definition of
: __percpu_counter_compare(), the second parameter is s64.  So when
: __percpu_counter_compare() executes abs(count - rhs), UBSAN shows the
: following warning:

> ...
> 
> Signed-off-by: Luo Meng <luomeng12@huawei.com>
> ---
>  mm/shmem.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/mm/shmem.c b/mm/shmem.c
> index 4b2fea33158e..15733c480318 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -3487,6 +3487,10 @@ static int shmem_reconfigure(struct fs_context *fc)
>  
>  	raw_spin_lock(&sbinfo->stat_lock);
>  	inodes = sbinfo->max_inodes - sbinfo->free_inodes;
> +	if (ctx->blocks > S64_MAX) {
> +		err = "The number of blocks too large";

Should be "is too large".  But to match the other error messages'
style, we can use simply "Number of blocks too large".

> +		goto out;
> +	}
>  	if ((ctx->seen & SHMEM_SEEN_BLOCKS) && ctx->blocks) {
>  		if (!sbinfo->max_blocks) {
>  			err = "Cannot retroactively limit size";

I suppose that's a reasonable fix.  I prefer "get the darned types
right" but shmem_options.blocks should be unsigned (can't have a
negative block count) and changing percpu_counter() would be tough.

--- a/mm/shmem.c~tmpfs-fix-undefined-behaviour-in-shmem_reconfigure-fix
+++ a/mm/shmem.c
@@ -3477,7 +3477,7 @@ static int shmem_reconfigure(struct fs_c
 	raw_spin_lock(&sbinfo->stat_lock);
 	inodes = sbinfo->max_inodes - sbinfo->free_inodes;
 	if (ctx->blocks > S64_MAX) {
-		err = "The number of blocks too large";
+		err = "Number of blocks too large";
 		goto out;
 	}
 	if ((ctx->seen & SHMEM_SEEN_BLOCKS) && ctx->blocks) {
_