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

[Eric Sandeen <sandeen@redhat.com>]

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