* [PATCH] SUNRPC: Replace strlcpy with strscpy @ 2023-06-13 0:40 Azeem Shaikh 2023-06-13 14:18 ` Chuck Lever III 0 siblings, 1 reply; 5+ messages in thread From: Azeem Shaikh @ 2023-06-13 0:40 UTC (permalink / raw) To: Chuck Lever, Jeff Layton Cc: linux-hardening, Azeem Shaikh, Neil Brown, Olga Kornievskaia, Dai Ngo, Tom Talpey, linux-nfs, linux-kernel, Trond Myklebust, Anna Schumaker, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, netdev strlcpy() reads the entire source buffer first. This read may exceed the destination size limit. This is both inefficient and can lead to linear read overflows if a source string is not NUL-terminated [1]. In an effort to remove strlcpy() completely [2], replace strlcpy() here with strscpy(). Direct replacement is safe here since the getter in kernel_params_ops handles -errorno return [3]. [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy [2] https://github.com/KSPP/linux/issues/89 [3] https://elixir.bootlin.com/linux/v6.4-rc6/source/include/linux/moduleparam.h#L52 Signed-off-by: Azeem Shaikh <azeemshaikh38@gmail.com> --- net/sunrpc/svc.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c index e6d4cec61e47..e5f379c4fdb3 100644 --- a/net/sunrpc/svc.c +++ b/net/sunrpc/svc.c @@ -109,13 +109,13 @@ param_get_pool_mode(char *buf, const struct kernel_param *kp) switch (*ip) { case SVC_POOL_AUTO: - return strlcpy(buf, "auto\n", 20); + return strscpy(buf, "auto\n", 20); case SVC_POOL_GLOBAL: - return strlcpy(buf, "global\n", 20); + return strscpy(buf, "global\n", 20); case SVC_POOL_PERCPU: - return strlcpy(buf, "percpu\n", 20); + return strscpy(buf, "percpu\n", 20); case SVC_POOL_PERNODE: - return strlcpy(buf, "pernode\n", 20); + return strscpy(buf, "pernode\n", 20); default: return sprintf(buf, "%d\n", *ip); } -- 2.41.0.162.gfafddb0af9-goog ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] SUNRPC: Replace strlcpy with strscpy 2023-06-13 0:40 [PATCH] SUNRPC: Replace strlcpy with strscpy Azeem Shaikh @ 2023-06-13 14:18 ` Chuck Lever III 2023-06-13 19:42 ` Kees Cook 0 siblings, 1 reply; 5+ messages in thread From: Chuck Lever III @ 2023-06-13 14:18 UTC (permalink / raw) To: Azeem Shaikh Cc: Jeff Layton, linux-hardening@vger.kernel.org, Neil Brown, Olga Kornievskaia, Dai Ngo, Tom Talpey, Linux NFS Mailing List, open list, Trond Myklebust, Anna Schumaker, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, netdev@vger.kernel.org > On Jun 12, 2023, at 8:40 PM, Azeem Shaikh <azeemshaikh38@gmail.com> wrote: > > strlcpy() reads the entire source buffer first. > This read may exceed the destination size limit. > This is both inefficient and can lead to linear read > overflows if a source string is not NUL-terminated [1]. > In an effort to remove strlcpy() completely [2], replace > strlcpy() here with strscpy(). Using sprintf() seems cleaner to me: it would get rid of the undocumented naked integer. Would that work for you? > Direct replacement is safe here since the getter in kernel_params_ops > handles -errorno return [3]. s/errorno/errno/ > [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy > [2] https://github.com/KSPP/linux/issues/89 > [3] https://elixir.bootlin.com/linux/v6.4-rc6/source/include/linux/moduleparam.h#L52 > > Signed-off-by: Azeem Shaikh <azeemshaikh38@gmail.com> > --- > net/sunrpc/svc.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c > index e6d4cec61e47..e5f379c4fdb3 100644 > --- a/net/sunrpc/svc.c > +++ b/net/sunrpc/svc.c > @@ -109,13 +109,13 @@ param_get_pool_mode(char *buf, const struct kernel_param *kp) > switch (*ip) > { > case SVC_POOL_AUTO: > - return strlcpy(buf, "auto\n", 20); > + return strscpy(buf, "auto\n", 20); > case SVC_POOL_GLOBAL: > - return strlcpy(buf, "global\n", 20); > + return strscpy(buf, "global\n", 20); > case SVC_POOL_PERCPU: > - return strlcpy(buf, "percpu\n", 20); > + return strscpy(buf, "percpu\n", 20); > case SVC_POOL_PERNODE: > - return strlcpy(buf, "pernode\n", 20); > + return strscpy(buf, "pernode\n", 20); > default: > return sprintf(buf, "%d\n", *ip); > } > -- > 2.41.0.162.gfafddb0af9-goog > > -- Chuck Lever ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] SUNRPC: Replace strlcpy with strscpy 2023-06-13 14:18 ` Chuck Lever III @ 2023-06-13 19:42 ` Kees Cook 2023-06-13 19:43 ` Chuck Lever III 0 siblings, 1 reply; 5+ messages in thread From: Kees Cook @ 2023-06-13 19:42 UTC (permalink / raw) To: Chuck Lever III Cc: Azeem Shaikh, Jeff Layton, linux-hardening@vger.kernel.org, Neil Brown, Olga Kornievskaia, Dai Ngo, Tom Talpey, Linux NFS Mailing List, open list, Trond Myklebust, Anna Schumaker, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, netdev@vger.kernel.org On Tue, Jun 13, 2023 at 02:18:06PM +0000, Chuck Lever III wrote: > > > > On Jun 12, 2023, at 8:40 PM, Azeem Shaikh <azeemshaikh38@gmail.com> wrote: > > > > strlcpy() reads the entire source buffer first. > > This read may exceed the destination size limit. > > This is both inefficient and can lead to linear read > > overflows if a source string is not NUL-terminated [1]. > > In an effort to remove strlcpy() completely [2], replace > > strlcpy() here with strscpy(). > > Using sprintf() seems cleaner to me: it would get rid of > the undocumented naked integer. Would that work for you? This is changing the "get" routine for reporting module parameters out of /sys. I think the right choice here is sysfs_emit(), as it performs the size tracking correctly. (Even the "default" sprintf() call should be replaced too, IMO.) > > > > Direct replacement is safe here since the getter in kernel_params_ops > > handles -errorno return [3]. > > s/errorno/errno/ > > > > [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy > > [2] https://github.com/KSPP/linux/issues/89 > > [3] https://elixir.bootlin.com/linux/v6.4-rc6/source/include/linux/moduleparam.h#L52 > > > > Signed-off-by: Azeem Shaikh <azeemshaikh38@gmail.com> > > --- > > net/sunrpc/svc.c | 8 ++++---- > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c > > index e6d4cec61e47..e5f379c4fdb3 100644 > > --- a/net/sunrpc/svc.c > > +++ b/net/sunrpc/svc.c > > @@ -109,13 +109,13 @@ param_get_pool_mode(char *buf, const struct kernel_param *kp) > > switch (*ip) > > { > > case SVC_POOL_AUTO: > > - return strlcpy(buf, "auto\n", 20); > > + return strscpy(buf, "auto\n", 20); e.g. return sysfs_emit(buf, "auto\n"); ... > > case SVC_POOL_GLOBAL: > > - return strlcpy(buf, "global\n", 20); > > + return strscpy(buf, "global\n", 20); > > case SVC_POOL_PERCPU: > > - return strlcpy(buf, "percpu\n", 20); > > + return strscpy(buf, "percpu\n", 20); > > case SVC_POOL_PERNODE: > > - return strlcpy(buf, "pernode\n", 20); > > + return strscpy(buf, "pernode\n", 20); > > default: > > return sprintf(buf, "%d\n", *ip); and: return sysfs_emit(buf, "%d\n", *ip); -Kees -- Kees Cook ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] SUNRPC: Replace strlcpy with strscpy 2023-06-13 19:42 ` Kees Cook @ 2023-06-13 19:43 ` Chuck Lever III 2023-06-13 22:55 ` Azeem Shaikh 0 siblings, 1 reply; 5+ messages in thread From: Chuck Lever III @ 2023-06-13 19:43 UTC (permalink / raw) To: Kees Cook Cc: Azeem Shaikh, Jeff Layton, linux-hardening@vger.kernel.org, Neil Brown, Olga Kornievskaia, Dai Ngo, Tom Talpey, Linux NFS Mailing List, open list, Trond Myklebust, Anna Schumaker, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, netdev@vger.kernel.org > On Jun 13, 2023, at 3:42 PM, Kees Cook <keescook@chromium.org> wrote: > > On Tue, Jun 13, 2023 at 02:18:06PM +0000, Chuck Lever III wrote: >> >> >>> On Jun 12, 2023, at 8:40 PM, Azeem Shaikh <azeemshaikh38@gmail.com> wrote: >>> >>> strlcpy() reads the entire source buffer first. >>> This read may exceed the destination size limit. >>> This is both inefficient and can lead to linear read >>> overflows if a source string is not NUL-terminated [1]. >>> In an effort to remove strlcpy() completely [2], replace >>> strlcpy() here with strscpy(). >> >> Using sprintf() seems cleaner to me: it would get rid of >> the undocumented naked integer. Would that work for you? > > This is changing the "get" routine for reporting module parameters out > of /sys. I think the right choice here is sysfs_emit(), as it performs > the size tracking correctly. (Even the "default" sprintf() call should > be replaced too, IMO.) Agreed, that's even better. >>> Direct replacement is safe here since the getter in kernel_params_ops >>> handles -errorno return [3]. >> >> s/errorno/errno/ >> >> >>> [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy >>> [2] https://github.com/KSPP/linux/issues/89 >>> [3] https://elixir.bootlin.com/linux/v6.4-rc6/source/include/linux/moduleparam.h#L52 >>> >>> Signed-off-by: Azeem Shaikh <azeemshaikh38@gmail.com> >>> --- >>> net/sunrpc/svc.c | 8 ++++---- >>> 1 file changed, 4 insertions(+), 4 deletions(-) >>> >>> diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c >>> index e6d4cec61e47..e5f379c4fdb3 100644 >>> --- a/net/sunrpc/svc.c >>> +++ b/net/sunrpc/svc.c >>> @@ -109,13 +109,13 @@ param_get_pool_mode(char *buf, const struct kernel_param *kp) >>> switch (*ip) >>> { >>> case SVC_POOL_AUTO: >>> - return strlcpy(buf, "auto\n", 20); >>> + return strscpy(buf, "auto\n", 20); > > e.g. > return sysfs_emit(buf, "auto\n"); > ... > >>> case SVC_POOL_GLOBAL: >>> - return strlcpy(buf, "global\n", 20); >>> + return strscpy(buf, "global\n", 20); >>> case SVC_POOL_PERCPU: >>> - return strlcpy(buf, "percpu\n", 20); >>> + return strscpy(buf, "percpu\n", 20); >>> case SVC_POOL_PERNODE: >>> - return strlcpy(buf, "pernode\n", 20); >>> + return strscpy(buf, "pernode\n", 20); >>> default: >>> return sprintf(buf, "%d\n", *ip); > > and: > > return sysfs_emit(buf, "%d\n", *ip); > > > -Kees > > -- > Kees Cook -- Chuck Lever ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] SUNRPC: Replace strlcpy with strscpy 2023-06-13 19:43 ` Chuck Lever III @ 2023-06-13 22:55 ` Azeem Shaikh 0 siblings, 0 replies; 5+ messages in thread From: Azeem Shaikh @ 2023-06-13 22:55 UTC (permalink / raw) To: Chuck Lever III Cc: Kees Cook, Jeff Layton, linux-hardening@vger.kernel.org, Neil Brown, Olga Kornievskaia, Dai Ngo, Tom Talpey, Linux NFS Mailing List, open list, Trond Myklebust, Anna Schumaker, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, netdev@vger.kernel.org On Tue, Jun 13, 2023 at 3:43 PM Chuck Lever III <chuck.lever@oracle.com> wrote: > > > On Jun 13, 2023, at 3:42 PM, Kees Cook <keescook@chromium.org> wrote: > > > > On Tue, Jun 13, 2023 at 02:18:06PM +0000, Chuck Lever III wrote: > >> > >> > >>> On Jun 12, 2023, at 8:40 PM, Azeem Shaikh <azeemshaikh38@gmail.com> wrote: > >>> > >>> strlcpy() reads the entire source buffer first. > >>> This read may exceed the destination size limit. > >>> This is both inefficient and can lead to linear read > >>> overflows if a source string is not NUL-terminated [1]. > >>> In an effort to remove strlcpy() completely [2], replace > >>> strlcpy() here with strscpy(). > >> > >> Using sprintf() seems cleaner to me: it would get rid of > >> the undocumented naked integer. Would that work for you? > > > > This is changing the "get" routine for reporting module parameters out > > of /sys. I think the right choice here is sysfs_emit(), as it performs > > the size tracking correctly. (Even the "default" sprintf() call should > > be replaced too, IMO.) > > Agreed, that's even better. > Thanks folks. Will send over a v2 which replaces strlcpy with sysfs_emit. ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-06-13 22:55 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-06-13 0:40 [PATCH] SUNRPC: Replace strlcpy with strscpy Azeem Shaikh 2023-06-13 14:18 ` Chuck Lever III 2023-06-13 19:42 ` Kees Cook 2023-06-13 19:43 ` Chuck Lever III 2023-06-13 22:55 ` Azeem Shaikh
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox