Re: [PATCH] pNFS: fix uninitialized pointer access
From: Antonio Quartulli
Date: Thu Jul 17 2025 - 04:01:59 EST
On 17/07/2025 06:56, Dan Carpenter wrote:
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.
Ok. It wasn't clear to me that
657 buffer_size = NFS_SERVER(arg->inode)->wsize;
would always set buffer_size to something large enough to encode at
least one block extent.
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@xxxxxxxxxxxxx>
---
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. */
I agree a comment would help.
Another option would be to initialize the be_prev to NULL. This will
silence the uninitialized variable warning.
But will likely trigger a potential NULL-ptr-deref, because the static
analyzer believes we can get there with count==0.
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.
Thanks for the hint about setting CONFIG_INIT_STACK_ALL_PATTERN=y.
I'll add it to our test system as well.
Regards,
--
Antonio Quartulli
CEO and Co-Founder
Mandelbit Srl
https://www.mandelbit.com