* Re: [PATCH] afs: Fix compile warning in afs_dynroot_lookup()
2019-12-19 13:14 [PATCH] afs: Fix compile warning in afs_dynroot_lookup() Tiezhu Yang
@ 2019-12-27 0:06 ` Al Viro
2019-12-27 9:10 ` David Howells
1 sibling, 0 replies; 3+ messages in thread
From: Al Viro @ 2019-12-27 0:06 UTC (permalink / raw)
To: Tiezhu Yang; +Cc: David Howells, linux-afs, linux-kernel
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...
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH] afs: Fix compile warning in afs_dynroot_lookup()
2019-12-19 13:14 [PATCH] afs: Fix compile warning in afs_dynroot_lookup() Tiezhu Yang
2019-12-27 0:06 ` Al Viro
@ 2019-12-27 9:10 ` David Howells
1 sibling, 0 replies; 3+ messages in thread
From: David Howells @ 2019-12-27 9:10 UTC (permalink / raw)
To: Al Viro; +Cc: dhowells, Tiezhu Yang, linux-afs, linux-kernel
Al Viro <viro@zeniv.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
^ permalink raw reply [flat|nested] 3+ messages in thread