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

[Eric Sandeen <sandeen@redhat.com>]

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);
>