[syzbot] [v9fs?] UBSAN: shift-out-of-bounds in v9fs_get_tree

[syzbot] [v9fs?] UBSAN: shift-out-of-bounds in v9fs_get_tree

[syzbot]

Hello,

syzbot found the following issue on:

HEAD commit:    3ac864c2d9bb Add linux-next specific files for 20250818
git tree:       linux-next
console+strace: https://syzkaller.appspot.com/x/log.txt?x=13706442580000
kernel config:  https://syzkaller.appspot.com/x/.config?x=6d1acc6b9e1fca1b
dashboard link: https://syzkaller.appspot.com/bug?extid=30c83da54e948f6e9436
compiler:       Debian clang version 20.1.7 (++20250616065708+6146a88f6049-1~exp1~20250616065826.132), Debian LLD 20.1.7
syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=141586f0580000
C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=124c9ba2580000

Downloadable assets:
disk image: https://storage.googleapis.com/syzbot-assets/37dbe82593f0/disk-3ac864c2.raw.xz
vmlinux: https://storage.googleapis.com/syzbot-assets/d2fea0824445/vmlinux-3ac864c2.xz
kernel image: https://storage.googleapis.com/syzbot-assets/6f2a83735a01/bzImage-3ac864c2.xz

IMPORTANT: if you fix the issue, please add the following tag to the commit:
Reported-by: syzbot+30c83da54e948f6e9436@syzkaller.appspotmail.com

------------[ cut here ]------------
UBSAN: shift-out-of-bounds in fs/9p/vfs_super.c:57:22
shift exponent 32 is too large for 32-bit type 'int'
CPU: 0 UID: 0 PID: 5861 Comm: syz-executor379 Not tainted 6.17.0-rc2-next-20250818-syzkaller #0 PREEMPT(full) 
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 07/12/2025
Call Trace:
 <TASK>
 dump_stack_lvl+0x189/0x250 lib/dump_stack.c:120
 ubsan_epilogue+0xa/0x40 lib/ubsan.c:233
 __ubsan_handle_shift_out_of_bounds+0x386/0x410 lib/ubsan.c:494
 v9fs_fill_super fs/9p/vfs_super.c:57 [inline]
 v9fs_get_tree+0x957/0xa90 fs/9p/vfs_super.c:125
 vfs_get_tree+0x8f/0x2b0 fs/super.c:1752
 do_new_mount+0x2a2/0xa30 fs/namespace.c:3810
 do_mount fs/namespace.c:4138 [inline]
 __do_sys_mount fs/namespace.c:4349 [inline]
 __se_sys_mount+0x317/0x410 fs/namespace.c:4326
 do_syscall_x64 arch/x86/entry/syscall_64.c:63 [inline]
 do_syscall_64+0xfa/0x3b0 arch/x86/entry/syscall_64.c:94
 entry_SYSCALL_64_after_hwframe+0x77/0x7f
RIP: 0033:0x7ff35edd46a9
Code: 48 83 c4 28 c3 e8 37 17 00 00 0f 1f 80 00 00 00 00 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 73 01 c3 48 c7 c1 b8 ff ff ff f7 d8 64 89 01 48
RSP: 002b:00007ffeee8a4078 EFLAGS: 00000246 ORIG_RAX: 00000000000000a5
RAX: ffffffffffffffda RBX: 00002000000025c0 RCX: 00007ff35edd46a9
RDX: 00002000000000c0 RSI: 00002000000025c0 RDI: 0000000000000000
RBP: 0000200000000280 R08: 0000200000000280 R09: 00007ffeee8a4258
R10: 0000000000000000 R11: 0000000000000246 R12: 00007ff35ee1d017
R13: 00007ffeee8a4248 R14: 0000000000000001 R15: 0000000000000001
 </TASK>
---[ end trace ]---


---
This report is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzkaller@googlegroups.com.

syzbot will keep track of this issue. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.

If the report is already addressed, let syzbot know by replying with:
#syz fix: exact-commit-title

If you want syzbot to run the reproducer, reply with:
#syz test: git://repo/address.git branch-or-commit-hash
If you attach or paste a git patch, syzbot will apply it before testing.

If you want to overwrite report's subsystems, reply with:
#syz set subsystems: new-subsystem
(See the list of subsystem names on the web dashboard)

If the report is a duplicate of another one, reply with:
#syz dup: exact-subject-of-another-report

If you want to undo deduplication, reply with:
#syz undup

Re: [syzbot] [v9fs?] UBSAN: shift-out-of-bounds in v9fs_get_tree

[Dominique Martinet]

Hi Eric,

syzbot wrote on Wed, Aug 20, 2025 at 07:58:31PM -0700:
> UBSAN: shift-out-of-bounds in fs/9p/vfs_super.c:57:22
> shift exponent 32 is too large for 32-bit type 'int'
> Call Trace:
>  <TASK>
>  dump_stack_lvl+0x189/0x250 lib/dump_stack.c:120
>  ubsan_epilogue+0xa/0x40 lib/ubsan.c:233
>  __ubsan_handle_shift_out_of_bounds+0x386/0x410 lib/ubsan.c:494
>  v9fs_fill_super fs/9p/vfs_super.c:57 [inline]
>  v9fs_get_tree+0x957/0xa90 fs/9p/vfs_super.c:125
>  vfs_get_tree+0x8f/0x2b0 fs/super.c:1752
>  do_new_mount+0x2a2/0xa30 fs/namespace.c:3810
>  do_mount fs/namespace.c:4138 [inline]

I thinks the mount rework triggered this one (full copy below or at [1])
[1] https://lore.kernel.org/all/68a68b57.050a0220.3d78fd.0012.GAE@google.com/T/#u

From a quick look the old code bound msize to 4k-INT_MAX, but the new
code accepts higher uint32 values.
To be honest I'm not sure INT_MAX even makes sense as later allocations
are likely to work :) but for now something as simple as this is likely
to work (I'm not sure I got the test thing right, let's see...)

Shall I just roll that into your patch, unless you know of a more
appropriate limit?
There doesn't seem to be any easy to use variable about max allocation
size, a limit of a few MB is probably sensible but I don't like
artificial restrictions just to please syzbot so happy to defer to
someone else here.


#syz test

diff --git a/fs/9p/v9fs.c b/fs/9p/v9fs.c
index 55ba26186351..cc65330ee684 100644
--- a/fs/9p/v9fs.c
+++ b/fs/9p/v9fs.c
@@ -302,6 +302,10 @@ int v9fs_parse_param(struct fs_context *fc, struct fs_parameter *param)
                        p9_debug(P9_DEBUG_ERROR, "msize should be at least 4k\n");
                        return -EINVAL;
                }
+               if (result.uint_32 > INT_MAX) {
+                       p9_debug(P9_DEBUG_ERROR, "msize too big\n");
+                       return -EINVAL;
+               }
                clnt->msize = result.uint_32;
                break;
        case Opt_trans:

syzbot wrote on Wed, Aug 20, 2025 at 07:58:31PM -0700:
> Hello,
> 
> syzbot found the following issue on:
> 
> HEAD commit:    3ac864c2d9bb Add linux-next specific files for 20250818
> git tree:       linux-next
> console+strace: https://syzkaller.appspot.com/x/log.txt?x=13706442580000
> kernel config:  https://syzkaller.appspot.com/x/.config?x=6d1acc6b9e1fca1b
> dashboard link: https://syzkaller.appspot.com/bug?extid=30c83da54e948f6e9436
> compiler:       Debian clang version 20.1.7 (++20250616065708+6146a88f6049-1~exp1~20250616065826.132), Debian LLD 20.1.7
> syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=141586f0580000
> C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=124c9ba2580000
> 
> Downloadable assets:
> disk image: https://storage.googleapis.com/syzbot-assets/37dbe82593f0/disk-3ac864c2.raw.xz
> vmlinux: https://storage.googleapis.com/syzbot-assets/d2fea0824445/vmlinux-3ac864c2.xz
> kernel image: https://storage.googleapis.com/syzbot-assets/6f2a83735a01/bzImage-3ac864c2.xz
> 
> IMPORTANT: if you fix the issue, please add the following tag to the commit:
> Reported-by: syzbot+30c83da54e948f6e9436@syzkaller.appspotmail.com
> 
> ------------[ cut here ]------------
> UBSAN: shift-out-of-bounds in fs/9p/vfs_super.c:57:22
> shift exponent 32 is too large for 32-bit type 'int'
> CPU: 0 UID: 0 PID: 5861 Comm: syz-executor379 Not tainted 6.17.0-rc2-next-20250818-syzkaller #0 PREEMPT(full) 
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 07/12/2025
> Call Trace:
>  <TASK>
>  dump_stack_lvl+0x189/0x250 lib/dump_stack.c:120
>  ubsan_epilogue+0xa/0x40 lib/ubsan.c:233
>  __ubsan_handle_shift_out_of_bounds+0x386/0x410 lib/ubsan.c:494
>  v9fs_fill_super fs/9p/vfs_super.c:57 [inline]
>  v9fs_get_tree+0x957/0xa90 fs/9p/vfs_super.c:125
>  vfs_get_tree+0x8f/0x2b0 fs/super.c:1752
>  do_new_mount+0x2a2/0xa30 fs/namespace.c:3810
>  do_mount fs/namespace.c:4138 [inline]
>  __do_sys_mount fs/namespace.c:4349 [inline]
>  __se_sys_mount+0x317/0x410 fs/namespace.c:4326
>  do_syscall_x64 arch/x86/entry/syscall_64.c:63 [inline]
>  do_syscall_64+0xfa/0x3b0 arch/x86/entry/syscall_64.c:94
>  entry_SYSCALL_64_after_hwframe+0x77/0x7f
> RIP: 0033:0x7ff35edd46a9
> Code: 48 83 c4 28 c3 e8 37 17 00 00 0f 1f 80 00 00 00 00 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 73 01 c3 48 c7 c1 b8 ff ff ff f7 d8 64 89 01 48
> RSP: 002b:00007ffeee8a4078 EFLAGS: 00000246 ORIG_RAX: 00000000000000a5
> RAX: ffffffffffffffda RBX: 00002000000025c0 RCX: 00007ff35edd46a9
> RDX: 00002000000000c0 RSI: 00002000000025c0 RDI: 0000000000000000
> RBP: 0000200000000280 R08: 0000200000000280 R09: 00007ffeee8a4258
> R10: 0000000000000000 R11: 0000000000000246 R12: 00007ff35ee1d017
> R13: 00007ffeee8a4248 R14: 0000000000000001 R15: 0000000000000001
>  </TASK>
> ---[ end trace ]---
> 
> 
> ---
> This report is generated by a bot. It may contain errors.
> See https://goo.gl/tpsmEJ for more information about syzbot.
> syzbot engineers can be reached at syzkaller@googlegroups.com.
> 
> syzbot will keep track of this issue. See:
> https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
> 
> If the report is already addressed, let syzbot know by replying with:
> #syz fix: exact-commit-title
> 
> If you want syzbot to run the reproducer, reply with:
> #syz test: git://repo/address.git branch-or-commit-hash
> If you attach or paste a git patch, syzbot will apply it before testing.
> 
> If you want to overwrite report's subsystems, reply with:
> #syz set subsystems: new-subsystem
> (See the list of subsystem names on the web dashboard)
> 
> If the report is a duplicate of another one, reply with:
> #syz dup: exact-subject-of-another-report
> 
> If you want to undo deduplication, reply with:
> #syz undup

-- 
Dominique Martinet | Asmadeus

Re: [syzbot] [v9fs?] UBSAN: shift-out-of-bounds in v9fs_get_tree

[syzbot]

Hello,

syzbot has tested the proposed patch but the reproducer is still triggering an issue:
UBSAN: shift-out-of-bounds in v9fs_get_tree

------------[ cut here ]------------
UBSAN: shift-out-of-bounds in fs/9p/vfs_super.c:57:22
shift exponent 32 is too large for 32-bit type 'int'
CPU: 1 UID: 0 PID: 6530 Comm: syz.0.17 Not tainted syzkaller #0 PREEMPT(full) 
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 07/12/2025
Call Trace:
 <TASK>
 dump_stack_lvl+0x189/0x250 lib/dump_stack.c:120
 ubsan_epilogue+0xa/0x40 lib/ubsan.c:233
 __ubsan_handle_shift_out_of_bounds+0x386/0x410 lib/ubsan.c:494
 v9fs_fill_super fs/9p/vfs_super.c:57 [inline]
 v9fs_get_tree+0x957/0xa90 fs/9p/vfs_super.c:125
 vfs_get_tree+0x92/0x2b0 fs/super.c:1752
 do_new_mount+0x2a2/0xa30 fs/namespace.c:3810
 do_mount fs/namespace.c:4138 [inline]
 __do_sys_mount fs/namespace.c:4349 [inline]
 __se_sys_mount+0x317/0x410 fs/namespace.c:4326
 do_syscall_x64 arch/x86/entry/syscall_64.c:63 [inline]
 do_syscall_64+0xfa/0x3b0 arch/x86/entry/syscall_64.c:94
 entry_SYSCALL_64_after_hwframe+0x77/0x7f
RIP: 0033:0x7f6fe798ebe9
Code: ff ff c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 40 00 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 73 01 c3 48 c7 c1 a8 ff ff ff f7 d8 64 89 01 48
RSP: 002b:00007f6fe87a7038 EFLAGS: 00000246 ORIG_RAX: 00000000000000a5
RAX: ffffffffffffffda RBX: 00007f6fe7bb5fa0 RCX: 00007f6fe798ebe9
RDX: 00002000000000c0 RSI: 00002000000025c0 RDI: 0000000000000000
RBP: 00007f6fe7a11e19 R08: 0000200000000280 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000000
R13: 00007f6fe7bb6038 R14: 00007f6fe7bb5fa0 R15: 00007ffebc4cd908
 </TASK>
---[ end trace ]---


Tested on:

commit:         5303936d Add linux-next specific files for 20250820
git tree:       linux-next
console output: https://syzkaller.appspot.com/x/log.txt?x=156cb3bc580000
kernel config:  https://syzkaller.appspot.com/x/.config?x=2619a6495a03d773
dashboard link: https://syzkaller.appspot.com/bug?extid=30c83da54e948f6e9436
compiler:       Debian clang version 20.1.7 (++20250616065708+6146a88f6049-1~exp1~20250616065826.132), Debian LLD 20.1.7
patch:          https://syzkaller.appspot.com/x/patch.diff?x=17fcd442580000

Re: [syzbot] [v9fs?] UBSAN: shift-out-of-bounds in v9fs_get_tree

[Eric Sandeen]

On 8/20/25 10:48 PM, Dominique Martinet wrote:
> Hi Eric,

Again, apologies, not sure how I missed this as well.

But circling back:

> syzbot wrote on Wed, Aug 20, 2025 at 07:58:31PM -0700:
>> UBSAN: shift-out-of-bounds in fs/9p/vfs_super.c:57:22
>> shift exponent 32 is too large for 32-bit type 'int'
>> Call Trace:
>>  <TASK>
>>  dump_stack_lvl+0x189/0x250 lib/dump_stack.c:120
>>  ubsan_epilogue+0xa/0x40 lib/ubsan.c:233
>>  __ubsan_handle_shift_out_of_bounds+0x386/0x410 lib/ubsan.c:494
>>  v9fs_fill_super fs/9p/vfs_super.c:57 [inline]
>>  v9fs_get_tree+0x957/0xa90 fs/9p/vfs_super.c:125
>>  vfs_get_tree+0x8f/0x2b0 fs/super.c:1752
>>  do_new_mount+0x2a2/0xa30 fs/namespace.c:3810
>>  do_mount fs/namespace.c:4138 [inline]
> 
> I thinks the mount rework triggered this one (full copy below or at [1])
> [1] https://lore.kernel.org/all/68a68b57.050a0220.3d78fd.0012.GAE@google.com/T/#u
> 
> From a quick look the old code bound msize to 4k-INT_MAX, but the new
> code accepts higher uint32 values.
> To be honest I'm not sure INT_MAX even makes sense as later allocations
> are likely to work :) but for now something as simple as this is likely
> to work (I'm not sure I got the test thing right, let's see...)
> 
> Shall I just roll that into your patch, unless you know of a more
> appropriate limit?
> There doesn't seem to be any easy to use variable about max allocation
> size, a limit of a few MB is probably sensible but I don't like
> artificial restrictions just to please syzbot so happy to defer to
> someone else here.
> 
> 
> #syz test
> 
> diff --git a/fs/9p/v9fs.c b/fs/9p/v9fs.c
> index 55ba26186351..cc65330ee684 100644
> --- a/fs/9p/v9fs.c
> +++ b/fs/9p/v9fs.c
> @@ -302,6 +302,10 @@ int v9fs_parse_param(struct fs_context *fc, struct fs_parameter *param)
>                         p9_debug(P9_DEBUG_ERROR, "msize should be at least 4k\n");
>                         return -EINVAL;
>                 }
> +               if (result.uint_32 > INT_MAX) {
> +                       p9_debug(P9_DEBUG_ERROR, "msize too big\n");
> +                       return -EINVAL;
> +               }

FWIW if we need to limit msize to a signed int, we can just change the
Opt_msize entry in v9fs_param_spec[] to an fsparam_s32 and anything
bigger should be rejected by the core parsers. The parsed value would be
retrieved via result.int_32 (vs. result.uint_32 here).

(I had seen {Opt_msize, "msize=%u"} and thought "unsigned" but missed
that it actually used match_int(). So probably a couple other spots
diverged with my patch as well, though maybe they are of less
consequence.)

-Eric

[PATCH next] 9p: Correct the session info

[Edward Adam Davis]

syz report a shift-out-of-bounds in v9fs_get_tree.

This is because the maxdata value is 0, causing fls to return 32, meaning
the s_blocksize_bits value is 32, which causes an out of bounds error.
The root cause of this is incorrect session information obtained during
fill super. Since v9ses is stored in sb, it is used directly.

Fixes: 4d18c32a395d ("9p: convert to the new mount API")
Reported-by: syzbot+30c83da54e948f6e9436@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=30c83da54e948f6e9436
Signed-off-by: Edward Adam Davis <eadavis@qq.com>
---
 fs/9p/vfs_super.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/9p/vfs_super.c b/fs/9p/vfs_super.c
index f6065b5e0e5d..cc2056dd0bef 100644
--- a/fs/9p/vfs_super.c
+++ b/fs/9p/vfs_super.c
@@ -50,7 +50,7 @@ static int v9fs_fill_super(struct super_block *sb, struct fs_context *fc)
 {
 	int ret;
 	struct v9fs_context	*ctx = fc->fs_private;
-	struct v9fs_session_info *v9ses = &ctx->v9ses;
+	struct v9fs_session_info *v9ses = sb->s_fs_info;
 
 	sb->s_maxbytes = MAX_LFS_FILESIZE;
 	sb->s_blocksize_bits = fls(v9ses->maxdata - 1);
-- 
2.43.0

[PATCH next V2] 9p: Correct the session info

[Edward Adam Davis]

syz report a shift-out-of-bounds in v9fs_get_tree.

This is because the maxdata value is 0, causing fls to return 32, meaning
the s_blocksize_bits value is 32, which causes an out of bounds error.
The root cause of this is incorrect session information obtained during
fill super. Since v9ses is stored in sb, it is used directly.

Fixes: 4d18c32a395d ("9p: convert to the new mount API")
Reported-by: syzbot+30c83da54e948f6e9436@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=30c83da54e948f6e9436
Signed-off-by: Edward Adam Davis <eadavis@qq.com>
---
V1 -> V2: remove the unused ctx

 fs/9p/vfs_super.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/fs/9p/vfs_super.c b/fs/9p/vfs_super.c
index f6065b5e0e5d..bcb6ebdb8037 100644
--- a/fs/9p/vfs_super.c
+++ b/fs/9p/vfs_super.c
@@ -49,8 +49,7 @@ static int v9fs_set_super(struct super_block *s, struct fs_context *fc)
 static int v9fs_fill_super(struct super_block *sb, struct fs_context *fc)
 {
 	int ret;
-	struct v9fs_context	*ctx = fc->fs_private;
-	struct v9fs_session_info *v9ses = &ctx->v9ses;
+	struct v9fs_session_info *v9ses = sb->s_fs_info;
 
 	sb->s_maxbytes = MAX_LFS_FILESIZE;
 	sb->s_blocksize_bits = fls(v9ses->maxdata - 1);
-- 
2.43.0

Re: [PATCH next V2] 9p: Correct the session info

[Dominique Martinet]

Edward Adam Davis wrote on Sat, Aug 23, 2025 at 07:22:13AM +0800:
> syz report a shift-out-of-bounds in v9fs_get_tree.
> 
> This is because the maxdata value is 0, causing fls to return 32, meaning
> the s_blocksize_bits value is 32, which causes an out of bounds error.
> The root cause of this is incorrect session information obtained during
> fill super. Since v9ses is stored in sb, it is used directly.

Thanks for the patch.

Eric, ignore the other part of the thread -- I guess the int max limit
wasn't related...

What I'm not following now is how the v9ses is created/handled around
the new mount API:
- in v9fs_get_tree a v9ses is allocated and passed along in
fc->s_fs_info (that this patches now uses)
- but in v9fs_init_fs_context then a `v9fs_context` is created that
also embeds (not a pointer) a v9ses struct, which is accessed through
fc->fs_private as the code before this patch.

So at least for some time we have two v9ses which obviously don't hold
the same fields, and I'm not confident I can review which is used where
and when.

Now I probably should read up about the "new" mount API, but I don't
like that there are two v9ses around.
I don't have a clue about the fs_context lifetime: is it kept around all
the time the fs is mounted and can we rely on it to be present (and get
rid of the v9ses allocated in v9fs_get_tree), or is the context only a
temporary thing and we should avoid having a v9ses in there instead?
(I'd be tempted to think the later?)


Edward, thanks for investingating this; at this point I'm worried there
are other inconsistencies so I'll just remove the new mount API patches
from my -next branch instead of applying the patch, but this is really
appreciated.

> Fixes: 4d18c32a395d ("9p: convert to the new mount API")
> Reported-by: syzbot+30c83da54e948f6e9436@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=30c83da54e948f6e9436
> Signed-off-by: Edward Adam Davis <eadavis@qq.com>
> ---
> V1 -> V2: remove the unused ctx
> 
>  fs/9p/vfs_super.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/fs/9p/vfs_super.c b/fs/9p/vfs_super.c
> index f6065b5e0e5d..bcb6ebdb8037 100644
> --- a/fs/9p/vfs_super.c
> +++ b/fs/9p/vfs_super.c
> @@ -49,8 +49,7 @@ static int v9fs_set_super(struct super_block *s, struct fs_context *fc)
>  static int v9fs_fill_super(struct super_block *sb, struct fs_context *fc)
>  {
>  	int ret;
> -	struct v9fs_context	*ctx = fc->fs_private;
> -	struct v9fs_session_info *v9ses = &ctx->v9ses;
> +	struct v9fs_session_info *v9ses = sb->s_fs_info;
>  
>  	sb->s_maxbytes = MAX_LFS_FILESIZE;
>  	sb->s_blocksize_bits = fls(v9ses->maxdata - 1);

-- 
Dominique Martinet | Asmadeus

Re: [PATCH next V2] 9p: Correct the session info

[Eric Sandeen]

On 8/23/25 1:34 AM, Dominique Martinet wrote:
> Edward Adam Davis wrote on Sat, Aug 23, 2025 at 07:22:13AM +0800:
>> syz report a shift-out-of-bounds in v9fs_get_tree.
>>
>> This is because the maxdata value is 0, causing fls to return 32, meaning
>> the s_blocksize_bits value is 32, which causes an out of bounds error.
>> The root cause of this is incorrect session information obtained during
>> fill super. Since v9ses is stored in sb, it is used directly.
> 
> Thanks for the patch.
> 
> Eric, ignore the other part of the thread -- I guess the int max limit
> wasn't related...
> 
> What I'm not following now is how the v9ses is created/handled around
> the new mount API:
> - in v9fs_get_tree a v9ses is allocated and passed along in
> fc->s_fs_info (that this patches now uses)

This one should exist for the lifetime of the mount, and is the real,
live session info.

> - but in v9fs_init_fs_context then a `v9fs_context` is created that
> also embeds (not a pointer) a v9ses struct, which is accessed through
> fc->fs_private as the code before this patch.

This one should be ephemeral, used to hold options during mount/remount.
(see the patch description for
"9p: create a v9fs_context structure to hold parsed options")

The idea is that the v9ses within the v9fs_context is used to hold
state/options while parsing or reconfiguring. Its lifetime is only during
mount/remount, and freed in v9fs_free_fc when that is done.

Before it is freed, at the end of mount/remount, the values stored in it
are copied into the "live" v9ses in v9fs_apply_options().

> So at least for some time we have two v9ses which obviously don't hold
> the same fields, and I'm not confident I can review which is used where
> and when.

The one within v9fs_context should only store options during parsing,
and those options are transferred to the "real" v9ses when parsing
is complete.

Re-using the v9ses structure was just convenient; IIRC there might be fields
in there that aren't needed during parsing (i.e. slst), but it was an easy
way to keep things in sync. We could create a new similar structure with a
new name, if that's better.

> Now I probably should read up about the "new" mount API, but I don't
> like that there are two v9ses around.
> I don't have a clue about the fs_context lifetime: is it kept around all
> the time the fs is mounted and can we rely on it to be present (and get
> rid of the v9ses allocated in v9fs_get_tree), or is the context only a
> temporary thing and we should avoid having a v9ses in there instead?
> (I'd be tempted to think the later?)

the context is created and initialized in fsopen, and is destroyed
when the operation is complete and userspace closes the fd it got
from fsopen.

> Edward, thanks for investingating this; at this point I'm worried there
> are other inconsistencies so I'll just remove the new mount API patches
> from my -next branch instead of applying the patch, but this is really
> appreciated.

So, back to our regularly scheduled bug programming ... ;)

>> Fixes: 4d18c32a395d ("9p: convert to the new mount API")
>> Reported-by: syzbot+30c83da54e948f6e9436@syzkaller.appspotmail.com
>> Closes: https://syzkaller.appspot.com/bug?extid=30c83da54e948f6e9436
>> Signed-off-by: Edward Adam Davis <eadavis@qq.com>
>> ---
>> V1 -> V2: remove the unused ctx
>>
>>  fs/9p/vfs_super.c | 3 +--
>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/fs/9p/vfs_super.c b/fs/9p/vfs_super.c
>> index f6065b5e0e5d..bcb6ebdb8037 100644
>> --- a/fs/9p/vfs_super.c
>> +++ b/fs/9p/vfs_super.c
>> @@ -49,8 +49,7 @@ static int v9fs_set_super(struct super_block *s, struct fs_context *fc)
>>  static int v9fs_fill_super(struct super_block *sb, struct fs_context *fc)
>>  {
>>  	int ret;
>> -	struct v9fs_context	*ctx = fc->fs_private;
>> -	struct v9fs_session_info *v9ses = &ctx->v9ses;
>> +	struct v9fs_session_info *v9ses = sb->s_fs_info;

This seems correct (though we can get rid of *fc too right?) 
To get here (v9fs_fill_super), we've gone through:

v9fs_get_tree(fc)
	// this will be our real live v9ses for the mount
	v9ses = kzalloc(sizeof(struct v9fs_session_info), GFP_KERNEL);

        v9fs_session_init(v9ses, fc)
		// copies fc/context options in context v9ses
		// into the "real" v9ses
                v9fs_apply_options(v9ses, fc)
		// inits maxdata on the real, live v9ses
		v9ses->maxdata = v9ses->clnt->msize - P9_IOHDRSZ;

	fc->s_fs_info = v9ses;	// real, live v9ses on fc->s_fs_info now.
        sb = sget_fc(fc, NULL, v9fs_set_super);
		s->s_fs_info = fc->s_fs_info; // live v9ses on s->fs_info now
                err = set(s, fc)
			v9fs_set_super(s, fc)
				// assigns s->s_fs_info to fc->s_fs_info
				// but I think this is redundant
				// and should be removed
				*v9ses = fc->s_fs_info;
	                        s->s_fs_info = v9ses;
				return set_anon_super(s, NULL);

        v9fs_fill_super(sb, fc)

and yes, with my patch were were using the option-holding "v9ses" here,
not the actual, fully initialized, legitimate v9ses for this mount, and
because v9ses->maxdata is not something that gets options parsed into it,
it was unset, and it so hit this bug. :(

Going back to the prior explanation of the fc context, it's probably
a better idea to not re-use the v9ses struct "for convenience" and
make a new dedicated structure just for option parsing, so unset
structure members aren't even present and therefore inaccessible.
I'm sorry for the mistake.

I should spin up a new version of the series fixing this and the
uint32/int32 difference you spotted as well, and credit Edward for his
fix in the new commit log somehow?

Thanks,
-Eric

>>  	sb->s_maxbytes = MAX_LFS_FILESIZE;
>>  	sb->s_blocksize_bits = fls(v9ses->maxdata - 1);
> 

Re: [PATCH next V2] 9p: Correct the session info

[Dominique Martinet]

Eric Sandeen wrote on Wed, Sep 24, 2025 at 05:19:11PM -0500:
> > What I'm not following now is how the v9ses is created/handled around
> > the new mount API:
> > - in v9fs_get_tree a v9ses is allocated and passed along in
> > fc->s_fs_info (that this patches now uses)
> 
> This one should exist for the lifetime of the mount, and is the real,
> live session info.
> 
> > - but in v9fs_init_fs_context then a `v9fs_context` is created that
> > also embeds (not a pointer) a v9ses struct, which is accessed through
> > fc->fs_private as the code before this patch.
> 
> This one should be ephemeral, used to hold options during mount/remount.
> (see the patch description for
> "9p: create a v9fs_context structure to hold parsed options")
> 
> The idea is that the v9ses within the v9fs_context is used to hold
> state/options while parsing or reconfiguring. Its lifetime is only during
> mount/remount, and freed in v9fs_free_fc when that is done.
> 
> Before it is freed, at the end of mount/remount, the values stored in it
> are copied into the "live" v9ses in v9fs_apply_options().

Thanks for clarifying this;

I had which is a pointer and which is embedded wrong on the first
reading so I first wrote that this should be ok if we could just null
the pointer in fs_private to avoid access after the copy, but the
temporary values are the embedded ones we can't "unaccess"...
Hmm, do we need anything else in fs_private after v9fs_apply_options()?
Would it make sense to call v9fs_free_fc() early on and null the full
fs_private in or after v9fs_apply_options()?

Using a different struct tailored for mount options is certainly more
appropriate if you have the time to do it, but as long as the risk of
accessing "the wrong one" goes away I'm fine either way, so if you think
nulling fs_private is possible without too much churn I think that's
good enough.

What do you think?



> >> diff --git a/fs/9p/vfs_super.c b/fs/9p/vfs_super.c
> >> index f6065b5e0e5d..bcb6ebdb8037 100644
> >> --- a/fs/9p/vfs_super.c
> >> +++ b/fs/9p/vfs_super.c
> >> @@ -49,8 +49,7 @@ static int v9fs_set_super(struct super_block *s, struct fs_context *fc)
> >>  static int v9fs_fill_super(struct super_block *sb, struct fs_context *fc)
> >>  {
> >>  	int ret;
> >> -	struct v9fs_context	*ctx = fc->fs_private;
> >> -	struct v9fs_session_info *v9ses = &ctx->v9ses;
> >> +	struct v9fs_session_info *v9ses = sb->s_fs_info;
> 
> This seems correct (though we can get rid of *fc too right?) 

(Sounds right as nothing else seems to use fc)

> To get here (v9fs_fill_super), we've gone through:
> 
> v9fs_get_tree(fc)
> 	// this will be our real live v9ses for the mount
> 	v9ses = kzalloc(sizeof(struct v9fs_session_info), GFP_KERNEL);
> 
>         v9fs_session_init(v9ses, fc)
> 		// copies fc/context options in context v9ses
> 		// into the "real" v9ses
>                 v9fs_apply_options(v9ses, fc)
> 		// inits maxdata on the real, live v9ses
> 		v9ses->maxdata = v9ses->clnt->msize - P9_IOHDRSZ;
> 
> 	fc->s_fs_info = v9ses;	// real, live v9ses on fc->s_fs_info now.
>         sb = sget_fc(fc, NULL, v9fs_set_super);
> 		s->s_fs_info = fc->s_fs_info; // live v9ses on s->fs_info now
>                 err = set(s, fc)
> 			v9fs_set_super(s, fc)
> 				// assigns s->s_fs_info to fc->s_fs_info
> 				// but I think this is redundant
> 				// and should be removed

My reading is that we need it, because the super block isn't the fs
context, and we need it for v9fs_umount_begin (it doesn't help that the
field name is the same between both structs, and that some super_block
variables are just 's' and others are 'sb', but I think that's the only
place where it's used)

At this point these are both the "real live" v9ses so that should be
fine as far as I can see.

> 				*v9ses = fc->s_fs_info;
> 	                        s->s_fs_info = v9ses;
> 				return set_anon_super(s, NULL);
> 
>         v9fs_fill_super(sb, fc)
> 
> and yes, with my patch were were using the option-holding "v9ses" here,
> not the actual, fully initialized, legitimate v9ses for this mount, and
> because v9ses->maxdata is not something that gets options parsed into it,
> it was unset, and it so hit this bug. :(
> 
> Going back to the prior explanation of the fc context, it's probably
> a better idea to not re-use the v9ses struct "for convenience" and
> make a new dedicated structure just for option parsing, so unset
> structure members aren't even present and therefore inaccessible.
> I'm sorry for the mistake.
> 
> I should spin up a new version of the series fixing this and the
> uint32/int32 difference you spotted as well

That would be great!

I'm not sure the unsigned makes much different ultimately, but might as
well make it a fsparam_s32 as you suggested...
Now I understand the lifecycle a bit better I can have another read with
that in mind before merging, and we can do the nulling fs_private or
other "make sure this bug doesn't come back" later if you don't have
time, I'm leaving this up to you.

> and credit Edward for his fix in the new commit log somehow?

I've never really been sure what kind of tag is appropriate in this
case, but I assume any mention would be welcome :)


Thanks,
-- 
Dominique Martinet | Asmadeus

Re: [PATCH next V2] 9p: Correct the session info

[Eric Sandeen]

On 9/24/25 9:17 PM, Dominique Martinet wrote:
> Eric Sandeen wrote on Wed, Sep 24, 2025 at 05:19:11PM -0500:
>>> What I'm not following now is how the v9ses is created/handled around
>>> the new mount API:
>>> - in v9fs_get_tree a v9ses is allocated and passed along in
>>> fc->s_fs_info (that this patches now uses)
>>
>> This one should exist for the lifetime of the mount, and is the real,
>> live session info.
>>
>>> - but in v9fs_init_fs_context then a `v9fs_context` is created that
>>> also embeds (not a pointer) a v9ses struct, which is accessed through
>>> fc->fs_private as the code before this patch.
>>
>> This one should be ephemeral, used to hold options during mount/remount.
>> (see the patch description for
>> "9p: create a v9fs_context structure to hold parsed options")
>>
>> The idea is that the v9ses within the v9fs_context is used to hold
>> state/options while parsing or reconfiguring. Its lifetime is only during
>> mount/remount, and freed in v9fs_free_fc when that is done.
>>
>> Before it is freed, at the end of mount/remount, the values stored in it
>> are copied into the "live" v9ses in v9fs_apply_options().
> 
> Thanks for clarifying this;
> 
> I had which is a pointer and which is embedded wrong on the first
> reading so I first wrote that this should be ok if we could just null
> the pointer in fs_private to avoid access after the copy, but the
> temporary values are the embedded ones we can't "unaccess"...
> Hmm, do we need anything else in fs_private after v9fs_apply_options()?
> Would it make sense to call v9fs_free_fc() early on and null the full
> fs_private in or after v9fs_apply_options()?
> 
> Using a different struct tailored for mount options is certainly more
> appropriate if you have the time to do it, but as long as the risk of
> accessing "the wrong one" goes away I'm fine either way, so if you think
> nulling fs_private is possible without too much churn I think that's
> good enough.
> 
> What do you think?

I think that in retrospect, (ab)using the full v9ses was a poor choice by
me, it clearly caused confusion (for me!)

>>>> diff --git a/fs/9p/vfs_super.c b/fs/9p/vfs_super.c
>>>> index f6065b5e0e5d..bcb6ebdb8037 100644
>>>> --- a/fs/9p/vfs_super.c
>>>> +++ b/fs/9p/vfs_super.c
>>>> @@ -49,8 +49,7 @@ static int v9fs_set_super(struct super_block *s, struct fs_context *fc)
>>>>  static int v9fs_fill_super(struct super_block *sb, struct fs_context *fc)
>>>>  {
>>>>  	int ret;
>>>> -	struct v9fs_context	*ctx = fc->fs_private;
>>>> -	struct v9fs_session_info *v9ses = &ctx->v9ses;
>>>> +	struct v9fs_session_info *v9ses = sb->s_fs_info;
>>
>> This seems correct (though we can get rid of *fc too right?) 
> 
> (Sounds right as nothing else seems to use fc)
> 
>> To get here (v9fs_fill_super), we've gone through:
>>
>> v9fs_get_tree(fc)
>> 	// this will be our real live v9ses for the mount
>> 	v9ses = kzalloc(sizeof(struct v9fs_session_info), GFP_KERNEL);
>>
>>         v9fs_session_init(v9ses, fc)
>> 		// copies fc/context options in context v9ses
>> 		// into the "real" v9ses
>>                 v9fs_apply_options(v9ses, fc)
>> 		// inits maxdata on the real, live v9ses
>> 		v9ses->maxdata = v9ses->clnt->msize - P9_IOHDRSZ;
>>
>> 	fc->s_fs_info = v9ses;	// real, live v9ses on fc->s_fs_info now.
>>         sb = sget_fc(fc, NULL, v9fs_set_super);
>> 		s->s_fs_info = fc->s_fs_info; // live v9ses on s->fs_info now
>>                 err = set(s, fc)
>> 			v9fs_set_super(s, fc)
>> 				// assigns s->s_fs_info to fc->s_fs_info
>> 				// but I think this is redundant
>> 				// and should be removed
> 
> My reading is that we need it, because the super block isn't the fs
> context, and we need it for v9fs_umount_begin (it doesn't help that the
> field name is the same between both structs, and that some super_block
> variables are just 's' and others are 'sb', but I think that's the only
> place where it's used)
> 
> At this point these are both the "real live" v9ses so that should be
> fine as far as I can see.

I said remove because sget_fc() does s->s_fs_info = fc->s_fs_info, and
then v9fs_set_super essentially does s->s_fs_info = fc->s_fs_info again,
so I think it's redundant but I'll look again when I'm less sleepy. 
>> 				*v9ses = fc->s_fs_info;
>> 	                        s->s_fs_info = v9ses;
>> 				return set_anon_super(s, NULL);
>>
>>         v9fs_fill_super(sb, fc)
>>
>> and yes, with my patch were were using the option-holding "v9ses" here,
>> not the actual, fully initialized, legitimate v9ses for this mount, and
>> because v9ses->maxdata is not something that gets options parsed into it,
>> it was unset, and it so hit this bug. :(
>>
>> Going back to the prior explanation of the fc context, it's probably
>> a better idea to not re-use the v9ses struct "for convenience" and
>> make a new dedicated structure just for option parsing, so unset
>> structure members aren't even present and therefore inaccessible.
>> I'm sorry for the mistake.
>>
>> I should spin up a new version of the series fixing this and the
>> uint32/int32 difference you spotted as well
> 
> That would be great!
> 
> I'm not sure the unsigned makes much different ultimately, but might as
> well make it a fsparam_s32 as you suggested...
> Now I understand the lifecycle a bit better I can have another read with
> that in mind before merging, and we can do the nulling fs_private or
> other "make sure this bug doesn't come back" later if you don't have
> time, I'm leaving this up to you.

Sounds good. Thanks again, and sorry for somehow completely missing this
thread earlier.

(Assume we've missed this merge window by now, so I probably won't rush
on this but will try to do it sooner than later.)

Thanks,
-Eric

>> and credit Edward for his fix in the new commit log somehow?
> 
> I've never really been sure what kind of tag is appropriate in this
> case, but I assume any mention would be welcome :)
> 
> 
> Thanks,

Re: [PATCH next V2] 9p: Correct the session info

[Dominique Martinet]

Eric Sandeen wrote on Wed, Sep 24, 2025 at 11:29:16PM -0500:
> On 9/24/25 9:17 PM, Dominique Martinet wrote:
> > Using a different struct tailored for mount options is certainly more
> > appropriate if you have the time to do it, but as long as the risk of
> > accessing "the wrong one" goes away I'm fine either way, so if you think
> > nulling fs_private is possible without too much churn I think that's
> > good enough.
> > 
> > What do you think?
> 
> I think that in retrospect, (ab)using the full v9ses was a poor choice by
> me, it clearly caused confusion (for me!)

I definitely won't complain if we get a dedicated struct :)

> > My reading is that we need it, because the super block isn't the fs
> > context, and we need it for v9fs_umount_begin (it doesn't help that the
> > field name is the same between both structs, and that some super_block
> > variables are just 's' and others are 'sb', but I think that's the only
> > place where it's used)
> > 
> > At this point these are both the "real live" v9ses so that should be
> > fine as far as I can see.
> 
> I said remove because sget_fc() does s->s_fs_info = fc->s_fs_info, and
> then v9fs_set_super essentially does s->s_fs_info = fc->s_fs_info again,
> so I think it's redundant but I'll look again when I'm less sleepy. 

Ah, I had missed the setting in sget_fc() -- you're right, we should get
rid of the one in v9fs_set_super()
(or actually, get rid of v9fs_set_super altogether and pass
set_anon_super() directly to sget_fc())


> > Now I understand the lifecycle a bit better I can have another read with
> > that in mind before merging, and we can do the nulling fs_private or
> > other "make sure this bug doesn't come back" later if you don't have
> > time, I'm leaving this up to you.
> 
> Sounds good. Thanks again, and sorry for somehow completely missing this
> thread earlier.

No worry, I should have re-sent a ping earlier as well.

> (Assume we've missed this merge window by now, so I probably won't rush
> on this but will try to do it sooner than later.)

Alright, I don't think we want to rush this, so let's get this next
cycle :)

Thanks,
-- 
Dominique Martinet | Asmadeus