linux-kernel - Re: [PATCH] afs: Fix compile warning in afs_dynroot

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: <12483.1577437812@warthog.procyon.org.uk>
Date:   Fri, 27 Dec 2019 09:10:12 +0000
From:   David Howells <dhowells@...hat.com>
To:     Al Viro <viro@...iv.linux.org.uk>
Cc:     dhowells@...hat.com, Tiezhu Yang <yangtiezhu@...ngson.cn>,
        linux-afs@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] afs: Fix compile warning in afs_dynroot_lookup()

Al Viro <viro@...iv.linux.org.uk> wrote:

> > -	int len;
> > +	int len = 0;
> >  
> >  	if (!net->ws_cell)
> >  		return ERR_PTR(-ENOENT);
> 
> NAK.  This is really, really wrong - passing zero to lookup_one_len() is
> always a bug.

You can't create a cell with a name of "" as afs_alloc_cell() will object with
EINVAL, so if net->ws_cell points to an afs_cell struct, that should never
have a zero-length name.

> BTW, what guarantees that cell->name won't be "@cell"?

afs_alloc_cell() won't allow that a cell with that that name either.

> The same for net->sysnames in afs_lookup_atsys() - what makes sure we won't
> see "@sys" among those?

afs_proc_sysname_write() checks for it.  Note that @sys substitutions are set
locally and are not obtained remotely.

> While we are at it,
>         d = d_splice_alias(inode, dentry);
>         if (!IS_ERR_OR_NULL(d)) {
>                 d->d_fsdata = dentry->d_fsdata;
>                 trace_afs_lookup(dvnode, &d->d_name,
>                                  inode ? AFS_FS_I(inode) : NULL);
>         } else {
>                 trace_afs_lookup(dvnode, &dentry->d_name,
>                                  IS_ERR_OR_NULL(inode) ? NULL
>                                  : AFS_FS_I(inode));
>         }
> is _very_ suspicious-looking - d_splice_alias() consumes
> an inode reference, and if it ends up failing on non-ERR_PTR()
> inode, the inode will be dropped by the time it returns.
> IOW, that AFS_FS_I(inode) in the second branch can bloody
> well point to already freed memory.

Yeah, fair point.  I need to save the fid before calling d_splice_alias().

> Tracepoints: Just Say No...

You can go and argue that one with David Miller if you like.

David