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

Date:   Fri, 27 Dec 2019 00:06:34 +0000
From:   Al Viro <viro@...iv.linux.org.uk>
To:     Tiezhu Yang <yangtiezhu@...ngson.cn>
Cc:     David Howells <dhowells@...hat.com>, linux-afs@...ts.infradead.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] afs: Fix compile warning in afs_dynroot_lookup()

On Thu, Dec 19, 2019 at 09:14:51PM +0800, Tiezhu Yang wrote:
> Fix the following compile warning:
> 
>   CC      fs/afs/dynroot.o
> fs/afs/dynroot.c: In function ‘afs_dynroot_lookup’:
> fs/afs/dynroot.c:117:6: warning: ‘len’ may be used uninitialized in this function [-Wmaybe-uninitialized]
>   ret = lookup_one_len(name, dentry->d_parent, len);
>       ^
> fs/afs/dynroot.c:91:6: note: ‘len’ was declared here
>   int len;
>       ^
> 
> Signed-off-by: Tiezhu Yang <yangtiezhu@...ngson.cn>
> ---
>  fs/afs/dynroot.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/afs/dynroot.c b/fs/afs/dynroot.c
> index 7503899..303f712 100644
> --- a/fs/afs/dynroot.c
> +++ b/fs/afs/dynroot.c
> @@ -88,7 +88,7 @@ static struct dentry *afs_lookup_atcell(struct dentry *dentry)
>  	struct dentry *ret;
>  	unsigned int seq = 0;
>  	char *name;
> -	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.  It's not any better than undefined value; if we *can*
get to lookup_one_len() call without other assignments to len, we
are fucked.

As it were, it's a false positive - we have
                if (cell) {
                        len = cell->name_len;
                        memcpy(name, cell->name, len + 1);
                }
upstream of
        if (!cell)
                goto out_n;

        ret = lookup_one_len(name, dentry->d_parent, len);
so we can't reach the call of lookup_one_len() without having
hit the assignment to len.

BTW, what guarantees that cell->name won't be "@cell"?  The
things would get rather interesting in that case...  The same
for net->sysnames in afs_lookup_atsys() - what makes sure
we won't see "@sys" among those?  David?

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.  Tracepoints: Just Say No...