[PATCH] NFS: Fix potential buffer overflowin nfs_sysfs_link_rpc_client()

[PATCH] NFS: Fix potential buffer overflowin nfs_sysfs_link_rpc_client()

[Gax-c]

From: Zichen Xie <zichenxie0106@gmail.com>

name is char[64] where the size of clnt->cl_program->name remains
unknown. Invoking strcat() directly will also lead to potential buffer
overflow. Change them to strscpy() and strncat() to fix potential
issues.

Fixes: e13b549319a6 ("NFS: Add sysfs links to sunrpc clients for nfs_clients")
Signed-off-by: Zichen Xie <zichenxie0106@gmail.com>
Cc: stable@vger.kernel.org
---
 fs/nfs/sysfs.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/nfs/sysfs.c b/fs/nfs/sysfs.c
index bf378ecd5d9f..7b59a40d40c0 100644
--- a/fs/nfs/sysfs.c
+++ b/fs/nfs/sysfs.c
@@ -280,9 +280,9 @@ void nfs_sysfs_link_rpc_client(struct nfs_server *server,
 	char name[RPC_CLIENT_NAME_SIZE];
 	int ret;
 
-	strcpy(name, clnt->cl_program->name);
-	strcat(name, uniq ? uniq : "");
-	strcat(name, "_client");
+	strscpy(name, clnt->cl_program->name, sizeof(name));
+	strncat(name, uniq ? uniq : "", sizeof(name) - strlen(name) - 1);
+	strncat(name, "_client", sizeof(name) - strlen(name) - 1);
 
 	ret = sysfs_create_link_nowarn(&server->kobj,
 						&clnt->cl_sysfs->kobject, name);
-- 
2.25.1

Re: [PATCH] NFS: Fix potential buffer overflowin nfs_sysfs_link_rpc_client()

[Trond Myklebust]

On Wed, 2024-12-18 at 00:13 +0800, Gax-c wrote:
> From: Zichen Xie <zichenxie0106@gmail.com>
> 
> name is char[64] where the size of clnt->cl_program->name remains
> unknown. Invoking strcat() directly will also lead to potential
> buffer
> overflow. Change them to strscpy() and strncat() to fix potential
> issues.

What makes you think that clnt->cl_program->name is unknown?

All calls to nfs_sysfs_link_rpc_client() use well known RPC clients for
which the cl_program is pointing to one of nlm_program, nfs_program or
nfsacl_program. So we know very well the sizes of clnt->cl_program-
>name.

> 
> Fixes: e13b549319a6 ("NFS: Add sysfs links to sunrpc clients for
> nfs_clients")
> Signed-off-by: Zichen Xie <zichenxie0106@gmail.com>
> Cc: stable@vger.kernel.org
> ---
>  fs/nfs/sysfs.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/nfs/sysfs.c b/fs/nfs/sysfs.c
> index bf378ecd5d9f..7b59a40d40c0 100644
> --- a/fs/nfs/sysfs.c
> +++ b/fs/nfs/sysfs.c
> @@ -280,9 +280,9 @@ void nfs_sysfs_link_rpc_client(struct nfs_server
> *server,
>  	char name[RPC_CLIENT_NAME_SIZE];
>  	int ret;
>  
> -	strcpy(name, clnt->cl_program->name);
> -	strcat(name, uniq ? uniq : "");
> -	strcat(name, "_client");
> +	strscpy(name, clnt->cl_program->name, sizeof(name));
> +	strncat(name, uniq ? uniq : "", sizeof(name) - strlen(name)
> - 1);
> +	strncat(name, "_client", sizeof(name) - strlen(name) - 1);
>  
>  	ret = sysfs_create_link_nowarn(&server->kobj,
>  						&clnt->cl_sysfs-
> >kobject, name);

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com

Re: [PATCH] NFS: Fix potential buffer overflowin nfs_sysfs_link_rpc_client()

[Trond Myklebust]

On Tue, 2024-12-17 at 16:51 +0000, Trond Myklebust wrote:
> On Wed, 2024-12-18 at 00:13 +0800, Gax-c wrote:
> > From: Zichen Xie <zichenxie0106@gmail.com>
> > 
> > name is char[64] where the size of clnt->cl_program->name remains
> > unknown. Invoking strcat() directly will also lead to potential
> > buffer
> > overflow. Change them to strscpy() and strncat() to fix potential
> > issues.
> 
> What makes you think that clnt->cl_program->name is unknown?
> 
> All calls to nfs_sysfs_link_rpc_client() use well known RPC clients
> for
> which the cl_program is pointing to one of nlm_program, nfs_program
> or
> nfsacl_program. So we know very well the sizes of clnt->cl_program-
> > name.

Just to clarify: I'm not strongly against the patch itself. However it
would seem premature to consider this a bug, let alone a stable fix
candidate. 

Has anyone ever seen a buffer overflow here? If so, under which
circumstances?

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com

Re: [PATCH] NFS: Fix potential buffer overflowin nfs_sysfs_link_rpc_client()

[Benjamin Coddington]

On 17 Dec 2024, at 12:07, Trond Myklebust wrote:

> On Tue, 2024-12-17 at 16:51 +0000, Trond Myklebust wrote:
>> On Wed, 2024-12-18 at 00:13 +0800, Gax-c wrote:
>>> From: Zichen Xie <zichenxie0106@gmail.com>
>>>
>>> name is char[64] where the size of clnt->cl_program->name remains
>>> unknown. Invoking strcat() directly will also lead to potential
>>> buffer
>>> overflow. Change them to strscpy() and strncat() to fix potential
>>> issues.
>>
>> What makes you think that clnt->cl_program->name is unknown?
>>
>> All calls to nfs_sysfs_link_rpc_client() use well known RPC clients
>> for
>> which the cl_program is pointing to one of nlm_program, nfs_program
>> or
>> nfsacl_program. So we know very well the sizes of clnt->cl_program-
>>> name.
>
> Just to clarify: I'm not strongly against the patch itself. However it
> would seem premature to consider this a bug, let alone a stable fix
> candidate. 
>
> Has anyone ever seen a buffer overflow here? If so, under which
> circumstances?

No, there's no way in the current kernel, but I suppose both
nfs_sysfs_link_rpc_client() as well as rpc_create() are exported, so we
could end up having some other part of the kernel send a long "uniq" or
create a client with a long cl_program->name.  That change might escape our
review.

Probably not a bad idea to send it back to stable IMO, since a change like
that could get back there too.

Reviewed-by: Benjamin Coddington <bcodding@redhat.com>

Ben