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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <aKlg5Ci4WC11GZGz@codewreck.org>
Date: Sat, 23 Aug 2025 15:34:12 +0900
From: Dominique Martinet <asmadeus@...ewreck.org>
To: Edward Adam Davis <eadavis@...com>, Eric Sandeen <sandeen@...hat.com>
Cc: ericvh@...nel.org, linux-kernel@...r.kernel.org,
	linux_oss@...debyte.com, lucho@...kov.net,
	syzbot+30c83da54e948f6e9436@...kaller.appspotmail.com,
	syzkaller-bugs@...glegroups.com, v9fs@...ts.linux.dev
Subject: Re: [PATCH next V2] 9p: Correct the session info

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@...kaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=30c83da54e948f6e9436
> Signed-off-by: Edward Adam Davis <eadavis@...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