linux-kernel - Re: [PATCH] pNFS: fix uninitialized pointer access

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: <b927d3dd-a4ed-46d7-b129-59eaf60305c7@suswa.mountain>
Date: Thu, 17 Jul 2025 07:56:01 +0300
From: Dan Carpenter <dan.carpenter@...aro.org>
To: Sergey Bashirov <sergeybashirov@...il.com>
Cc: Antonio Quartulli <antonio@...delbit.com>, linux-nfs@...r.kernel.org,
	Trond Myklebust <trondmy@...nel.org>,
	Anna Schumaker <anna@...nel.org>,
	Konstantin Evtushenko <koevtushenko@...dex.com>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] pNFS: fix uninitialized pointer access

On Thu, Jul 17, 2025 at 03:27:50AM +0300, Sergey Bashirov wrote:
> On Wed, Jul 16, 2025 at 04:38:48PM +0200, Antonio Quartulli wrote:
> > In ext_tree_encode_commit() if no block extent is encoded due to lack
> > of buffer space, ret is set to -ENOSPC and we end up accessing be_prev
> > despite it being uninitialized.
> 
> This static check warning appears to be a false positive. This is an
> internal static function that is not exported outside the module via
> an interface or API. Inside the module we always use a buffer size
> that is a multiple of PAGE_SIZE, so at least one page is provided.
> The block extent size does not exceed 44 bytes, so we can always
> encode at least one extent. Thus, we never fail on the first iteration.
> Either ret is zero, or ret is nonzero and at least one extent is encoded.
> 
> > Fix this behaviour by bailing out right away when no extent is encoded.
> >
> > Fixes: d84c4754f874 ("pNFS: Fix extent encoding in block/scsi layout")
> > Addresses-Coverity-ID: 1647611 ("Memory - illegal accesses  (UNINIT)")
> > Signed-off-by: Antonio Quartulli <antonio@...delbit.com>
> > ---
> >  fs/nfs/blocklayout/extent_tree.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/fs/nfs/blocklayout/extent_tree.c b/fs/nfs/blocklayout/extent_tree.c
> > index 315949a7e92d..82e19205f425 100644
> > --- a/fs/nfs/blocklayout/extent_tree.c
> > +++ b/fs/nfs/blocklayout/extent_tree.c
> > @@ -598,6 +598,11 @@ ext_tree_encode_commit(struct pnfs_block_layout *bl, __be32 *p,
> >  		if (ext_tree_layoutupdate_size(bl, *count) > buffer_size) {
> >  			(*count)--;
> >  			ret = -ENOSPC;
> > +			/* bail out right away if no extent was encoded */
> > +			if (!*count) {
> 
> We can't exit here without setting the value of lastbyte, which is one
> of the function outputs. Please set it to U64_MAX to let upper layer
> logic handle it properly. Or, see the alternative solution at the end.
>   +				*lastbyte = U64_MAX;
> 
> > +				spin_unlock(&bl->bl_ext_lock);
> > +				return ret;
> > +			}
> >  			break;
> >  		}
> >
> 
> If we need to fix this, I'd rather add an early check whether the buffer
> size is large enough to encode at least one extent at the beginning of
> the function. Before spinlock is acquired and ext_tree traversed. This
> looks more natural to me. But I'm not sure if this will satisfy the
> static checker.
> 

No, it won't.  I feel like the code is confusing enough that maybe a
comment is warranted.  /* We always iterate through the loop at least
once so be_prev is correct. */

Another option would be to initialize the be_prev to NULL.  This will
silence the uninitialized variable warning.  And everyone sensible runs
with CONFIG_INIT_STACK_ALL_ZERO set in production so it doesn't affect
run time at all.  Btw, we changed our test systems to set
CONFIG_INIT_STACK_ALL_PATTERN=y a few months ago and immediately ran
into an uninitialized variable bug.  So I've heard there are a couple
distros which don't set CONFIG_INIT_STACK_ALL_ZERO which is very daring
when every single developer is testing with uninitialized variables
defaulting to zero.

regards,
dan carpenter