linux-hardening - Re: [PATCH 05/19] fs: assign an anon dev

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20230914003424.GD800259@ZenIV>
Date:   Thu, 14 Sep 2023 01:34:24 +0100
From:   Al Viro <viro@...iv.linux.org.uk>
To:     Christoph Hellwig <hch@....de>
Cc:     Christian Brauner <brauner@...nel.org>,
        Heiko Carstens <hca@...ux.ibm.com>,
        Vasily Gorbik <gor@...ux.ibm.com>,
        Alexander Gordeev <agordeev@...ux.ibm.com>,
        Fenghua Yu <fenghua.yu@...el.com>,
        Reinette Chatre <reinette.chatre@...el.com>,
        Miquel Raynal <miquel.raynal@...tlin.com>,
        Richard Weinberger <richard@....at>,
        Vignesh Raghavendra <vigneshr@...com>,
        Dennis Dalessandro <dennis.dalessandro@...nelisnetworks.com>,
        Tejun Heo <tj@...nel.org>,
        Trond Myklebust <trond.myklebust@...merspace.com>,
        Anna Schumaker <anna@...nel.org>,
        Kees Cook <keescook@...omium.org>,
        Damien Le Moal <dlemoal@...nel.org>,
        Naohiro Aota <naohiro.aota@....com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-s390@...r.kernel.org, linux-rdma@...r.kernel.org,
        linux-nfs@...r.kernel.org, linux-hardening@...r.kernel.org,
        cgroups@...r.kernel.org
Subject: Re: [PATCH 05/19] fs: assign an anon dev_t in common code

On Wed, Sep 13, 2023 at 08:09:59AM -0300, Christoph Hellwig wrote:

> diff --git a/fs/super.c b/fs/super.c
> index bbe55f0651cca4..5c685b4944c2d6 100644
> --- a/fs/super.c
> +++ b/fs/super.c
> @@ -787,7 +787,7 @@ struct super_block *sget_fc(struct fs_context *fc,
>  	struct super_block *s = NULL;
>  	struct super_block *old;
>  	struct user_namespace *user_ns = fc->global ? &init_user_ns : fc->user_ns;
> -	int err;
> +	int err = 0;
>  
>  retry:
>  	spin_lock(&sb_lock);
> @@ -806,14 +806,26 @@ struct super_block *sget_fc(struct fs_context *fc,
>  	}
>  
>  	s->s_fs_info = fc->s_fs_info;
> -	err = set(s, fc);
> -	if (err) {
> -		s->s_fs_info = NULL;
> -		spin_unlock(&sb_lock);
> -		destroy_unused_super(s);
> -		return ERR_PTR(err);
> +	if (set) {
> +		err = set(s, fc);
> +		if (err) {
> +			s->s_fs_info = NULL;

Pointless (as the original had been); destroy_unused_super() doesn't
even look at ->s_fs_info.

> +			goto unlock_and_destroy;
> +		}
>  	}
>  	fc->s_fs_info = NULL;

Here we are transferring the ownership from fc to superblock; it used to sit
right next to insertion into lists and all failure exits past that point must
go through deactivate_locked_super(), so ->kill_sb() would be called on those
and it would take care of s->s_fs_info.  However, your variant has that
ownership transfer done at the point before get_anon_bdev(), and that got
you a new failure exit where you are still calling destroy_unused_super():

> +	if (!s->s_dev) {
> +		/*
> +		 * If the file system didn't set a s_dev (which is usually only
> +		 * done for block based file systems), set an anonymous dev_t
> +		 * here now so that we always have a valid ->s_dev.
> +		 */
> +		err = get_anon_bdev(&s->s_dev);
> +		if (err)
> +			goto unlock_and_destroy;

This.  And that's a leak - fc has no reference left in it, and your
unlock_and_destroy won't even look at what's in ->s_fs_info, let alone know
what to do with it.

IOW, clearing fc->s_fs_info should've been done after that chunk.

And looking at the change in sget(),

> +	if (set) {
> +		err = set(s, data);
> +		if (err)
> +			goto unlock_and_destroy;
>  	}
> +
> +	if (!s->s_dev) {
> +		err = get_anon_bdev(&s->s_dev);
> +		if (err)
> +			goto unlock_and_destroy;
> +	}

I'd rather expressed it (both there and in sget_fc() as well) as
	if (set)
		err = set(s, data);
	if (!err && !s->s_dev)
		err = get_anon_bdev(&s->s_dev);
	if (err)
		goto unlock_and_destroy;

That's really what your transformation does - you are lifting the
calls of get_anon_bdev() (in guise of set_anon_super()) from the
tails of 'set' callbacks into the caller, making them conditional
upon the lack of other errors from 'set' and upon the ->s_dev left
zero and allow NULL for the case when that was all that had been
there.

The only place where you do something different is this:

> @@ -1191,7 +1191,6 @@ static struct dentry *ceph_real_mount(struct ceph_fs_client *fsc,
>  static int ceph_set_super(struct super_block *s, struct fs_context *fc)
>  {
>  	struct ceph_fs_client *fsc = s->s_fs_info;
> -	int ret;
>  
>  	dout("set_super %p\n", s);
>  
> @@ -1211,11 +1210,7 @@ static int ceph_set_super(struct super_block *s, struct fs_context *fc)
>  	s->s_flags |= SB_NODIRATIME | SB_NOATIME;
>  
>  	ceph_fscrypt_set_ops(s);
> -
> -	ret = set_anon_super_fc(s, fc);
> -	if (ret != 0)
> -		fsc->sb = NULL;
> -	return ret;
> +	return 0;

fsc->sb = NULL has disappeared here; it *is* OK (the caller won't look at
fsc->sb after failed sget_fc()), but that's worth a mention somewhere.
A separate commit removing that clearing fsc->sb in ceph_set_super(),
perhaps?