Re: [PATCH] afs: Fix compile warning in afs_dynroot_lookup()

Al Viro
2019-12-27T00:06:48Z

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@loongson.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...