cifs: Fix oops due to uninitialised variable

[PATCH] cifs: Fix oops due to uninitialised variable

Posted by David Howells 5 months, 3 weeks ago

Fix smb3_init_transform_rq() to initialise buffer to NULL before calling
netfs_alloc_folioq_buffer() as netfs assumes it can append to the buffer it
is given.  Setting it to NULL means it should start a fresh buffer, but the
value is currently undefined.

Fixes: a2906d3316fc ("cifs: Switch crypto buffer to use a folio_queue rather than an xarray")
Signed-off-by: David Howells <dhowells@redhat.com>
cc: Steve French <sfrench@samba.org>
cc: Paulo Alcantara <pc@manguebit.org>
cc: linux-cifs@vger.kernel.org
cc: linux-fsdevel@vger.kernel.org
---
 fs/smb/client/smb2ops.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c
index ad8947434b71..cd0c9b5a35c3 100644
--- a/fs/smb/client/smb2ops.c
+++ b/fs/smb/client/smb2ops.c
@@ -4487,7 +4487,7 @@ smb3_init_transform_rq(struct TCP_Server_Info *server, int num_rqst,
 	for (int i = 1; i < num_rqst; i++) {
 		struct smb_rqst *old = &old_rq[i - 1];
 		struct smb_rqst *new = &new_rq[i];
-		struct folio_queue *buffer;
+		struct folio_queue *buffer = NULL;
 		size_t size = iov_iter_count(&old->rq_iter);
 
 		orig_len += smb_rqst_len(server, old);

Re: [PATCH] cifs: Fix oops due to uninitialised variable

Posted by Enzo Matsumiya 4 months, 3 weeks ago

Hi David, Steve,

On 08/19, David Howells wrote:
>Fix smb3_init_transform_rq() to initialise buffer to NULL before calling
>netfs_alloc_folioq_buffer() as netfs assumes it can append to the buffer it
>is given.  Setting it to NULL means it should start a fresh buffer, but the
>value is currently undefined.

This patch was based on David's RFC series
"netfs: [WIP] Allow the use of MSG_SPLICE_PAGES and use netmem
allocator", specifically patch 15/31 "cifs: Use
netfs_alloc/free_folioq_buffer()", which were never merged.

Current code in smb3_init_transform_rq() initializes buffer with
cifs_alloc_folioq_buffer() and NULL-checked right after:

> ...
>                 struct folio_queue *buffer;
>                 size_t size = iov_iter_count(&old->rq_iter);
> 
>                 orig_len += smb_rqst_len(server, old);
>                 new->rq_iov = old->rq_iov;
>                 new->rq_nvec = old->rq_nvec;
> 
>                 if (size > 0) {
>                         buffer = cifs_alloc_folioq_buffer(size);
>                         if (!buffer)
>                                 goto err_free;
> ...

Sorry not catching this earlier, but this just got my attention because
there's now a CVE for this non-bug/vulnerability
https://nvd.nist.gov/vuln/detail/CVE-2025-38737

I don't know what exactly can/should be done, but I thought I'd let you
know.


Cheers,

Enzo

>Fixes: a2906d3316fc ("cifs: Switch crypto buffer to use a folio_queue rather than an xarray")
>Signed-off-by: David Howells <dhowells@redhat.com>
>cc: Steve French <sfrench@samba.org>
>cc: Paulo Alcantara <pc@manguebit.org>
>cc: linux-cifs@vger.kernel.org
>cc: linux-fsdevel@vger.kernel.org
>---
> fs/smb/client/smb2ops.c |    2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c
>index ad8947434b71..cd0c9b5a35c3 100644
>--- a/fs/smb/client/smb2ops.c
>+++ b/fs/smb/client/smb2ops.c
>@@ -4487,7 +4487,7 @@ smb3_init_transform_rq(struct TCP_Server_Info *server, int num_rqst,
> 	for (int i = 1; i < num_rqst; i++) {
> 		struct smb_rqst *old = &old_rq[i - 1];
> 		struct smb_rqst *new = &new_rq[i];
>-		struct folio_queue *buffer;
>+		struct folio_queue *buffer = NULL;
> 		size_t size = iov_iter_count(&old->rq_iter);
>
> 		orig_len += smb_rqst_len(server, old);
>
>

Re: [PATCH] cifs: Fix oops due to uninitialised variable

Posted by Steve French 5 months, 3 weeks ago

merged into cifs-2.6.git for-next

On Tue, Aug 19, 2025 at 10:28 AM David Howells <dhowells@redhat.com> wrote:
>
> Fix smb3_init_transform_rq() to initialise buffer to NULL before calling
> netfs_alloc_folioq_buffer() as netfs assumes it can append to the buffer it
> is given.  Setting it to NULL means it should start a fresh buffer, but the
> value is currently undefined.
>
> Fixes: a2906d3316fc ("cifs: Switch crypto buffer to use a folio_queue rather than an xarray")
> Signed-off-by: David Howells <dhowells@redhat.com>
> cc: Steve French <sfrench@samba.org>
> cc: Paulo Alcantara <pc@manguebit.org>
> cc: linux-cifs@vger.kernel.org
> cc: linux-fsdevel@vger.kernel.org
> ---
>  fs/smb/client/smb2ops.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c
> index ad8947434b71..cd0c9b5a35c3 100644
> --- a/fs/smb/client/smb2ops.c
> +++ b/fs/smb/client/smb2ops.c
> @@ -4487,7 +4487,7 @@ smb3_init_transform_rq(struct TCP_Server_Info *server, int num_rqst,
>         for (int i = 1; i < num_rqst; i++) {
>                 struct smb_rqst *old = &old_rq[i - 1];
>                 struct smb_rqst *new = &new_rq[i];
> -               struct folio_queue *buffer;
> +               struct folio_queue *buffer = NULL;
>                 size_t size = iov_iter_count(&old->rq_iter);
>
>                 orig_len += smb_rqst_len(server, old);
>
>


-- 
Thanks,

Steve