tracing/uprobe: Replace strlcpy() with strscpy()
PATCH 1/1 tracing/uprobe: Replace strlcpy() with strscpy()
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]. Additionally, it returns the size of the source string, not the resulting size of the destination string. In an effort to remove strlcpy() completely[2], replace strlcpy() here with strscpy().
The negative return value is already handled by this code so no new handling is needed here.
--- kernel/trace/trace_uprobe.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
kernel/trace/trace_uprobe.c +1 -1
@@ -151,7 +151,7 @@ fetch_store_string(unsigned long addr, void *dest, void *base)return -ENOMEM ;if (addr ==FETCH_TOKEN_COMM ) - ret =strlcpy (dst, current->comm, maxlen); + ret =strscpy (dst, current->comm, maxlen);else ret =strncpy_from_user (dst, src, maxlen);if (ret >=0 ) {
Signature · 1 line
2.34.1
Masami Hiramatsu (Google)
maintainerQuoted 10 lines · On Thu, 30 Nov 2023 12:56:08 -0800 Kees Cook <[email protected]> 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]. Additionally, it returns the size of the source string, not the resulting size of the destination string. In an effort to removestrlcpy()completely[2], replacestrlcpy()here withstrscpy().The negative return value is already handled by this code so no new handling is needed here.
Quoted 6 lines
Cc Steven Rostedt <[email protected]>Cc Masami Hiramatsu <[email protected]>Signed-off-by Kees Cook <[email protected]>
Thanks for reporting! Let me pick this.
Thanks!
Quoted 20 lines
--- kernel/trace/trace_uprobe.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)kernel/trace/trace_uprobe.c +1
-1@@ -151,7 +151,7 @@ fetch_store_string(unsigned long addr, void *dest, void *base)return -ENOMEM ;if (addr ==FETCH_TOKEN_COMM ) - ret =strlcpy (dst, current->comm, maxlen); + ret =strscpy (dst, current->comm, maxlen);else ret =strncpy_from_user (dst, src, maxlen);if (ret >=0 ) {Signature · 1 line
2.34.1
Signature · 1 line
Masami Hiramatsu (Google) <[email protected]>
Quoted 10 lines · On Thu, 30 Nov 2023 12:56:08 -0800 Kees Cook <[email protected]> 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]. Additionally, it returns the size of the source string, not the resulting size of the destination string. In an effort to removestrlcpy()completely[2], replacestrlcpy()here withstrscpy().The negative return value is already handled by this code so no new handling is needed here.
Quoted 6 lines
Cc Steven Rostedt <[email protected]>Cc Masami Hiramatsu <[email protected]>Signed-off-by Kees Cook <[email protected]>
Hi Kees,
As same as sample's change, should I ask you to pick this to your tree? Since it is a kind of a part of series patch. I'm OK for it since this does not change the code so much.
In that case, please feel free to add my Ack.
Thank you,
Quoted 20 lines
--- kernel/trace/trace_uprobe.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)kernel/trace/trace_uprobe.c +1
-1@@ -151,7 +151,7 @@ fetch_store_string(unsigned long addr, void *dest, void *base)return -ENOMEM ;if (addr ==FETCH_TOKEN_COMM ) - ret =strlcpy (dst, current->comm, maxlen); + ret =strscpy (dst, current->comm, maxlen);else ret =strncpy_from_user (dst, src, maxlen);if (ret >=0 ) {Signature · 1 line
2.34.1
Signature · 1 line
Masami Hiramatsu (Google) <[email protected]>
Quoted 22 lines · Masami Hiramatsu
Quoted 10 lines · On Thu, 30 Nov 2023 12:56:08 -0800 Kees Cook <[email protected]> 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]. Additionally, it returns the size of the source string, not the resulting size of the destination string. In an effort to removestrlcpy()completely[2], replacestrlcpy()here withstrscpy().The negative return value is already handled by this code so no new handling is needed here.
Quoted 6 lines
Cc Steven Rostedt <[email protected]>Cc Masami Hiramatsu <[email protected]>Signed-off-by Kees Cook <[email protected]>Hi Kees,
Quoted 7 lines
As same as sample's change, should I ask you to pick this to your tree? Since it is a kind of a part of series patch. I'm OK for it since this does not change the code so much.
In that case, please feel free to add my Ack.
Either way is fine, but I can take it since I've got a few others already too. :)
Thanks!
-Kees
Signature · 1 line
Kees Cook
Kees Cook
AppliedQuoted 8 lines · Kees Cook
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]. Additionally, it returns the size of the source string, not the resulting size of the destination string. In an effort to removestrlcpy()completely[2], replacestrlcpy()here withstrscpy().[...]
Applied to for-next/hardening, thanks!
[1/1] tracing/uprobe: Replace strlcpy() with strscpy()
https://git.kernel.org/kees/c/8a3750ecf810
Take care,
Signature · 1 line
Kees Cook