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: <h4ydkt7c23ha46j33i42wh2ecdwtcrgxnvfb6c7mo3dqc7l2kz@ng7fev5rbqmi>
Date: Thu, 17 Jul 2025 03:27:50 +0300
From: Sergey Bashirov <sergeybashirov@...il.com>
To: Antonio Quartulli <antonio@...delbit.com>, 
	Dan Carpenter <dan.carpenter@...aro.org>, linux-nfs@...r.kernel.org
Cc: Trond Myklebust <trondmy@...nel.org>, Anna Schumaker <anna@...nel.org>, 
	Sergey Bashirov <sergeybashirov@...il.com>, Konstantin Evtushenko <koevtushenko@...dex.com>, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] pNFS: fix uninitialized pointer access

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.

diff --git a/fs/nfs/blocklayout/extent_tree.c b/fs/nfs/blocklayout/extent_tree.c
index 315949a7e92d..e80f2f82378f 100644
--- a/fs/nfs/blocklayout/extent_tree.c
+++ b/fs/nfs/blocklayout/extent_tree.c
@@ -588,6 +588,12 @@ ext_tree_encode_commit(struct pnfs_block_layout *bl, __be32 *p,
        struct pnfs_block_extent *be, *be_prev;
        int ret = 0;

+       if (ext_tree_layoutupdate_size(bl, 1) > buffer_size) {
+               *count = 0;
+               *lastbyte = U64_MAX;
+               return -ENOSPC;
+       }
+
        spin_lock(&bl->bl_ext_lock);
        for (be = ext_tree_first(&bl->bl_ext_rw); be; be = ext_tree_next(be)) {
                if (be->be_state != PNFS_BLOCK_INVALID_DATA ||


--
Sergey Bashirov