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: <d9b026f1-6ed3-41ca-8699-914c45b0339b@mandelbit.com>
Date: Thu, 17 Jul 2025 10:01:42 +0200
From: Antonio Quartulli <antonio@...delbit.com>
To: Dan Carpenter <dan.carpenter@...aro.org>,
 Sergey Bashirov <sergeybashirov@...il.com>
Cc: 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 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@...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. */
> 

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