| Message ID | 20241217161311.28640-1-zichenxie0106@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | NFS: Fix potential buffer overflowin nfs_sysfs_link_rpc_client() | expand |
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);
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?
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
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);