-
Notifications
You must be signed in to change notification settings - Fork 5
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove all strncpy() uses #90
Comments
Simplify the code by using kstrndup instead of kzalloc and strncpy in smk_parse_smack(), which meanwhile remove strncpy as [1] suggests. [1]: KSPP#90 Signed-off-by: GONG, Ruiqi <gongruiqi1@huawei.com>
Simplify the code by using kstrndup instead of kzalloc and strncpy in smk_parse_smack(), which meanwhile remove strncpy as [1] suggests. [1]: KSPP/linux#90 Signed-off-by: GONG, Ruiqi <gongruiqi1@huawei.com> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
Simplify the code by using kstrndup instead of kzalloc and strncpy in smk_parse_smack(), which meanwhile remove strncpy as [1] suggests. [1]: KSPP/linux#90 Signed-off-by: GONG, Ruiqi <gongruiqi1@huawei.com> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
One of the "legitimate" uses of strncpy() is copying a NUL-terminated string into a fixed-size non-NUL-terminated character array. To avoid the weaknesses and ambiguity of intent when using strncpy(), provide replacement functions that explicitly distinguish between trailing padding and not, and require the destination buffer size be discoverable by the compiler. For example: struct obj { int foo; char small[4] __nonstring; char big[8] __nonstring; int bar; }; struct obj p; /* This will truncate to 4 chars with no trailing NUL */ strncpy(p.small, "hello", sizeof(p.small)); /* p.small contains 'h', 'e', 'l', 'l' */ /* This will NUL pad to 8 chars. */ strncpy(p.big, "hello", sizeof(p.big)); /* p.big contains 'h', 'e', 'l', 'l', 'o', '\0', '\0', '\0' */ When the "__nonstring" attributes are missing, the intent of the programmer becomes ambiguous for whether the lack of a trailing NUL in the p.small copy is a bug. Additionally, it's not clear whether the trailing padding in the p.big copy is _needed_. Both cases become unambiguous with: strtomem(p.small, "hello"); strtomem_pad(p.big, "hello"); See also KSPP#90 Expand the memcpy KUnit tests to include these functions. Cc: Wolfram Sang <wsa+renesas@sang-engineering.com> Cc: Nick Desaulniers <ndesaulniers@google.com> Cc: Geert Uytterhoeven <geert@linux-m68k.org> Cc: Guenter Roeck <linux@roeck-us.net> Signed-off-by: Kees Cook <keescook@chromium.org>
One of the "legitimate" uses of strncpy() is copying a NUL-terminated string into a fixed-size non-NUL-terminated character array. To avoid the weaknesses and ambiguity of intent when using strncpy(), provide replacement functions that explicitly distinguish between trailing padding and not, and require the destination buffer size be discoverable by the compiler. For example: struct obj { int foo; char small[4] __nonstring; char big[8] __nonstring; int bar; }; struct obj p; /* This will truncate to 4 chars with no trailing NUL */ strncpy(p.small, "hello", sizeof(p.small)); /* p.small contains 'h', 'e', 'l', 'l' */ /* This will NUL pad to 8 chars. */ strncpy(p.big, "hello", sizeof(p.big)); /* p.big contains 'h', 'e', 'l', 'l', 'o', '\0', '\0', '\0' */ When the "__nonstring" attributes are missing, the intent of the programmer becomes ambiguous for whether the lack of a trailing NUL in the p.small copy is a bug. Additionally, it's not clear whether the trailing padding in the p.big copy is _needed_. Both cases become unambiguous with: strtomem(p.small, "hello"); strtomem_pad(p.big, "hello", 0); See also KSPP#90 Expand the memcpy KUnit tests to include these functions. Cc: Wolfram Sang <wsa+renesas@sang-engineering.com> Cc: Nick Desaulniers <ndesaulniers@google.com> Cc: Geert Uytterhoeven <geert@linux-m68k.org> Cc: Guenter Roeck <linux@roeck-us.net> Signed-off-by: Kees Cook <keescook@chromium.org>
One of the "legitimate" uses of strncpy() is copying a NUL-terminated string into a fixed-size non-NUL-terminated character array. To avoid the weaknesses and ambiguity of intent when using strncpy(), provide replacement functions that explicitly distinguish between trailing padding and not, and require the destination buffer size be discoverable by the compiler. For example: struct obj { int foo; char small[4] __nonstring; char big[8] __nonstring; int bar; }; struct obj p; /* This will truncate to 4 chars with no trailing NUL */ strncpy(p.small, "hello", sizeof(p.small)); /* p.small contains 'h', 'e', 'l', 'l' */ /* This will NUL pad to 8 chars. */ strncpy(p.big, "hello", sizeof(p.big)); /* p.big contains 'h', 'e', 'l', 'l', 'o', '\0', '\0', '\0' */ When the "__nonstring" attributes are missing, the intent of the programmer becomes ambiguous for whether the lack of a trailing NUL in the p.small copy is a bug. Additionally, it's not clear whether the trailing padding in the p.big copy is _needed_. Both cases become unambiguous with: strtomem(p.small, "hello"); strtomem_pad(p.big, "hello", 0); See also KSPP#90 Expand the memcpy KUnit tests to include these functions. Cc: Wolfram Sang <wsa+renesas@sang-engineering.com> Cc: Nick Desaulniers <ndesaulniers@google.com> Cc: Geert Uytterhoeven <geert@linux-m68k.org> Cc: Guenter Roeck <linux@roeck-us.net> Cc: Bagas Sanjaya <bagasdotme@gmail.com> Signed-off-by: Kees Cook <keescook@chromium.org>
One of the "legitimate" uses of strncpy() is copying a NUL-terminated string into a fixed-size non-NUL-terminated character array. To avoid the weaknesses and ambiguity of intent when using strncpy(), provide replacement functions that explicitly distinguish between trailing padding and not, and require the destination buffer size be discoverable by the compiler. For example: struct obj { int foo; char small[4] __nonstring; char big[8] __nonstring; int bar; }; struct obj p; /* This will truncate to 4 chars with no trailing NUL */ strncpy(p.small, "hello", sizeof(p.small)); /* p.small contains 'h', 'e', 'l', 'l' */ /* This will NUL pad to 8 chars. */ strncpy(p.big, "hello", sizeof(p.big)); /* p.big contains 'h', 'e', 'l', 'l', 'o', '\0', '\0', '\0' */ When the "__nonstring" attributes are missing, the intent of the programmer becomes ambiguous for whether the lack of a trailing NUL in the p.small copy is a bug. Additionally, it's not clear whether the trailing padding in the p.big copy is _needed_. Both cases become unambiguous with: strtomem(p.small, "hello"); strtomem_pad(p.big, "hello", 0); See also KSPP#90 Expand the memcpy KUnit tests to include these functions. Cc: Wolfram Sang <wsa+renesas@sang-engineering.com> Cc: Nick Desaulniers <ndesaulniers@google.com> Cc: Geert Uytterhoeven <geert@linux-m68k.org> Cc: Guenter Roeck <linux@roeck-us.net> Signed-off-by: Kees Cook <keescook@chromium.org>
One of the "legitimate" uses of strncpy() is copying a NUL-terminated string into a fixed-size non-NUL-terminated character array. To avoid the weaknesses and ambiguity of intent when using strncpy(), provide replacement functions that explicitly distinguish between trailing padding and not, and require the destination buffer size be discoverable by the compiler. For example: struct obj { int foo; char small[4] __nonstring; char big[8] __nonstring; int bar; }; struct obj p; /* This will truncate to 4 chars with no trailing NUL */ strncpy(p.small, "hello", sizeof(p.small)); /* p.small contains 'h', 'e', 'l', 'l' */ /* This will NUL pad to 8 chars. */ strncpy(p.big, "hello", sizeof(p.big)); /* p.big contains 'h', 'e', 'l', 'l', 'o', '\0', '\0', '\0' */ When the "__nonstring" attributes are missing, the intent of the programmer becomes ambiguous for whether the lack of a trailing NUL in the p.small copy is a bug. Additionally, it's not clear whether the trailing padding in the p.big copy is _needed_. Both cases become unambiguous with: strtomem(p.small, "hello"); strtomem_pad(p.big, "hello", 0); See also KSPP#90 Expand the memcpy KUnit tests to include these functions. Cc: Wolfram Sang <wsa+renesas@sang-engineering.com> Cc: Nick Desaulniers <ndesaulniers@google.com> Cc: Geert Uytterhoeven <geert@linux-m68k.org> Cc: Guenter Roeck <linux@roeck-us.net> Signed-off-by: Kees Cook <keescook@chromium.org>
What is to be done about cases where the destination buffer is length-bounded (not NUL-terminated) and its size is not known by the compiler (i.e: the macros There are some pathological cases in If Take this code, for example, from the case mentioned above: static void dsa_slave_get_strings(struct net_device *dev,
uint32_t stringset, uint8_t *data)
...
int len = 32;
strncpy(data, "tx_packets", len);
strncpy(data + len, "tx_bytes", len);
strncpy(data + 2 * len, "rx_packets", len);
strncpy(data + 3 * len, "rx_bytes", len);
... Simply swapping these calls to the appropriate tl;dr: based on the decision tree provided by OP there exists some cases (mentioned above) where the destination has both qualities 1) length-bounded and 2) impossible for the compiler to determine the size of
Somewhat Reviewed by: @nickdesaulniers |
After some tinkering, I think I've found a viable solution. If there existed some #define strntomem_pad(dest, dest_len, src, pad) \
memcpy_and_pad(dest, dest_len, src, strnlen(src, dest_len), pad); Here's how it would look in action tackling static void dsa_slave_get_strings(struct net_device *dev,
uint32_t stringset, uint8_t *data)
...
int len = 32;
strncpy(data, "tx_packets", len);
strncpy(data + len, "tx_bytes", len);
strncpy(data + 2 * len, "rx_packets", len);
strncpy(data + 3 * len, "rx_bytes", len);
... turns into static void dsa_slave_get_strings(struct net_device *dev,
uint32_t stringset, uint8_t *data)
...
int len = 32;
strntomem_pad(data, len, "tx_packets", 0);
strntomem_pad(data + len, len, "tx_bytes", 0);
strntomem_pad(data + 2 * len, len, "rx_packets", 0);
strntomem_pad(data + 3 * len, len, "rx_bytes", 0);
... Testing reveals these are exactly equivalent (in my isolated godbolt testing) and thus the new "Remove all strncpy() uses" decision tree looks as follows:
Is this a viable solution? |
Depending on the human to get the length argument correct is a new foot-gun that we should work very hard to avoid. Yeah, this is a new ugly case you've found (unknown length and not %NUL terminated). A few thoughts about the specific example:
This internal API looks very fragile. The Anyway, I suspect the best fix here is to teach the handlers the size of the buffers in some way. |
Given that the strings in question will not be truncated and other |
@kees Thoughts on this article: tl;dr: Linus doesn't want large swaths of
|
https://lore.kernel.org/lkml/202307121703.D2BE6DFEE@keescook/ tl;dr: we won't do treewide changes (instead we do it individually ) and we need to do conversions with a very regular process for performing and validating the transformations. (This is how we've approached the flexible array transformations as well.) |
`strncpy` is deprecated for use on NUL-terminated destination strings [1]. Even call sites utilizing length-bounded destination buffers should switch over to using `strtomem` or `strtomem_pad`. In this case, however, the compiler is unable to determine the size of the `data` buffer which renders `strtomem` unusable. Due to this, `strscpy` should be used. It should be noted that most call sites already zero-initialize the destination buffer. However, I've opted to use `strscpy_pad` to maintain the same exact behavior that `strncpy` produced (zero-padded tail up to `len`). Also see [3]. [1]: www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [2]: elixir.bootlin.com/linux/v6.3/source/net/ethtool/ioctl.c#L1944 [3]: manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html Link: KSPP#90 Signed-off-by: Justin Stitt <justinstitt@google.com>
`strncpy` is deprecated for use on NUL-terminated destination strings [1]. Even call sites utilizing length-bounded destination buffers should switch over to using `strtomem` or `strtomem_pad`. In this case, however, the compiler is unable to determine the size of the `data` buffer which renders `strtomem` unusable. Due to this, `strscpy` should be used. It should be noted that most call sites already zero-initialize the destination buffer. However, I've opted to use `strscpy_pad` to maintain the same exact behavior that `strncpy` produced (zero-padded tail up to `len`). Also see [3]. [1]: www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [2]: elixir.bootlin.com/linux/v6.3/source/net/ethtool/ioctl.c#L1944 [3]: manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html Link: KSPP#90 Reviewed-by: Nick Desaulniers <ndesaulniers@google.com> Reviewed-by: Kees Cook <keescook@chromium.org> Signed-off-by: Justin Stitt <justinstitt@google.com>
`strncpy` is deprecated for use on NUL-terminated destination strings [1]. A suitable replacement is `strscpy` [2] due to the fact that it guarantees NUL-termination on its destination buffer argument which is _not_ the case for `strncpy`! It was pretty difficult, in this case, to try and figure out whether or not the destination buffer was zero-initialized. If it is and this behavior is relied on then perhaps `strscpy_pad` is the preferred option here. Kees was able to help me out and identify the following code snippet which seems to show that the destination buffer is zero-initialized. | skl = devm_kzalloc(&pci->dev, sizeof(*skl), GFP_KERNEL); With this information, I opted for `strscpy` since padding is seemingly not required. Also within this patch is a change to an instance of `x > y - 1` to `x >= y` which tends to be more robust and readable. Consider, for instance, if `y` was somehow `INT_MIN`. [1]: www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [2]: manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html Link: KSPP#90 Suggested-by: Kees Cook <keescook@chromium.org> Signed-off-by: Justin Stitt <justinstitt@google.com>
`strncpy` is deprecated for use on NUL-terminated destination strings [1]. A suitable replacement is `strscpy` [2]. There are some hopes that someday the `strncpy` api could be ripped out due to the vast number of suitable replacements (strscpy, strscpy_pad, strtomem, strtomem_pad, strlcpy) [1]. [1]: www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [2]: manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html Link: KSPP#90 Signed-off-by: Justin Stitt <justinstitt@google.com> Link: https://lore.kernel.org/r/20230725-sound-soc-intel-avs-remove-deprecated-strncpy-v1-1-6357a1f8e9cf@google.com Signed-off-by: Mark Brown <broonie@kernel.org>
`strncpy` is deprecated for use on NUL-terminated destination strings [1]. A suitable replacement is `strscpy` [2] due to the fact that it guarantees NUL-termination on its destination buffer argument which is _not_ the case for `strncpy`! It was pretty difficult, in this case, to try and figure out whether or not the destination buffer was zero-initialized. If it is and this behavior is relied on then perhaps `strscpy_pad` is the preferred option here. Kees was able to help me out and identify the following code snippet which seems to show that the destination buffer is zero-initialized. | skl = devm_kzalloc(&pci->dev, sizeof(*skl), GFP_KERNEL); With this information, I opted for `strscpy` since padding is seemingly not required. [1]: www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [2]: manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html Link: KSPP#90 Suggested-by: Kees Cook <keescook@chromium.org> Signed-off-by: Justin Stitt <justinstitt@google.com>
`strncpy` is deprecated for use on NUL-terminated destination strings [1]. A suitable replacement is `strscpy` [2] due to the fact that it guarantees NUL-termination on its destination buffer argument which is _not_ always the case for `strncpy`! It should be noted that, in this case, the destination buffer has a length strictly greater than the source string. Moreover, the source string is NUL-terminated (and so is the destination) which means there was no real bug happening here. Nonetheless, this patch would get us one step closer to eliminating the `strncpy` API in the kernel, as its use is too ambiguous. We need to favor less ambiguous replacements such as: strscpy, strscpy_pad, strtomem and strtomem_pad (amongst others). Technically, my patch yields subtly different behavior. The original implementation with `strncpy` would fill the entire destination buffer with null bytes [3] while `strscpy` will leave the junk, uninitialized bytes trailing after the _mandatory_ NUL-termination. So, if somehow `pcm->name` or `card->driver/shortname/longname` require this NUL-padding behavior then `strscpy_pad` should be used. My interpretation, though, is that the aforementioned fields are just fine as NUL-terminated strings. Please correct my assumptions if needed and I'll send in a v2. [1]: www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [2]: manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html [3]: https://linux.die.net/man/3/strncpy Link: KSPP#90 Signed-off-by: Justin Stitt <justinstitt@google.com>
`strncpy` is deprecated for use on NUL-terminated destination strings [1]. A suitable replacement is `strscpy` [2] due to the fact that it guarantees NUL-termination on its destination buffer argument which is _not_ always the case for `strncpy`! It should be noted that, in this case, the destination buffer has a length strictly greater than the source string. Moreover, the source string is NUL-terminated (and so is the destination) which means there was no real bug happening here. Nonetheless, this patch would get us one step closer to eliminating the `strncpy` API in the kernel, as its use is too ambiguous. We need to favor less ambiguous replacements such as: strscpy, strscpy_pad, strtomem and strtomem_pad (amongst others). Technically, my patch yields subtly different behavior. The original implementation with `strncpy` would fill the entire destination buffer with null bytes [3] while `strscpy` will leave the junk, uninitialized bytes trailing after the _mandatory_ NUL-termination. So, if somehow `card->driver` or `card->shortname` require this NUL-padding behavior then `strscpy_pad` should be used. My interpretation, though, is that the aforementioned fields are just fine as NUL-terminated strings. Please correct my assumptions if needed and I'll send in a v2. [1]: www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [2]: manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html [3]: https://linux.die.net/man/3/strncpy Link: KSPP#90 Link: https://lore.kernel.org/r/20230727-sound-xen-v1-1-89dd161351f1@google.com (related ALSA patch) Signed-off-by: Justin Stitt <justinstitt@google.com>
`strncpy` is deprecated for use on NUL-terminated destination strings [1]. A suitable replacement is `strscpy` [2] due to the fact that it guarantees NUL-termination on its destination buffer argument which is _not_ always the case for `strncpy`! In this case, though, there was great care taken to ensure that the destination buffer would be NUL-terminated through the use of `len - 1` ensuring that the previously zero-initialized buffer would not overwrite the last NUL byte. This means that there's no bug here. However, `strscpy` will add a mandatory NUL byte to the destination buffer as promised by the following `strscpy` implementation [3]: | /* Hit buffer length without finding a NUL; force NUL-termination. */ | if (res) | dest[res-1] = '\0'; This means we can lose the `- 1` which clears up whats happening here. All the while, we get one step closer to eliminating the ambiguous `strncpy` api in favor of its less ambiguous replacement like `strscpy`, `strscpy_pad`, `strtomem` and `strtomem_pad` amongst others. [1]: www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [2]: manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html [3]: https://elixir.bootlin.com/linux/v6.3/source/lib/string.c#L183 Link: KSPP#90 Signed-off-by: Justin Stitt <justinstitt@google.com>
`strncpy` is deprecated for use on NUL-terminated destination strings [1]. A suitable replacement is `strscpy` [2] due to the fact that it guarantees NUL-termination on its destination buffer argument which is _not_ always the case for `strncpy`! In this case, though, there was care taken to ensure that the destination buffer would be NUL-terminated. The destination buffer is zero-initialized and each `pm860x->name[i]` has a size of `MAX_NAME_LENGTH + 1`. This means that there is unlikely to be a bug here. However, in an attempt to eliminate the usage of the `strncpy` API as well as disambiguate implementations, replacements such as: `strscpy`, `strscpy_pad`, `strtomem` and `strtomem_pad` should be preferred. We are able to eliminate the need for `len + 1` since `strscpy` guarantees NUL-termination for its destination buffer as per its implementation [3]: | /* Hit buffer length without finding a NUL; force NUL-termination. */ | if (res) | dest[res-1] = '\0'; [1]: www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [2]: manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html [3]: https://elixir.bootlin.com/linux/v6.3/source/lib/string.c#L183 Link: KSPP#90 Signed-off-by: Justin Stitt <justinstitt@google.com>
`strncpy` is deprecated for use on NUL-terminated destination strings [1]. A suitable replacement is `strscpy` [2] due to the fact that it guarantees NUL-termination on its destination buffer argument which is _not_ always the case for `strncpy`! It should be noted that, in this case, the destination buffer has a length strictly greater than the source string. Moreover, the source string is NUL-terminated (and so is the destination) which means there was no real bug happening here. Nonetheless, this patch would get us one step closer to eliminating the `strncpy` API in the kernel, as its use is too ambiguous. We need to favor less ambiguous replacements such as: strscpy, strscpy_pad, strtomem and strtomem_pad (amongst others). Technically, my patch yields subtly different behavior. The original implementation with `strncpy` would fill the entire destination buffer with null bytes [3] while `strscpy` will leave the junk, uninitialized bytes trailing after the _mandatory_ NUL-termination. So, if somehow `pcm->name` or `card->driver/shortname/longname` require this NUL-padding behavior then `strscpy_pad` should be used. My interpretation, though, is that the aforementioned fields are just fine as NUL-terminated strings. Please correct my assumptions if needed and I'll send in a v2. [1]: www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [2]: manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html [3]: https://linux.die.net/man/3/strncpy Link: KSPP#90 Signed-off-by: Justin Stitt <justinstitt@google.com> Reviewed-by: Kees Cook <keescook@chromium.org> Link: https://lore.kernel.org/r/20230727-sound-xen-v1-1-89dd161351f1@google.com Signed-off-by: Takashi Iwai <tiwai@suse.de>
`strncpy` is deprecated for use on NUL-terminated destination strings [1]. A suitable replacement is `strscpy` [2] due to the fact that it guarantees NUL-termination on its destination buffer argument which is _not_ always the case for `strncpy`! It should be noted that, in this case, the destination buffer has a length strictly greater than the source string. Moreover, the source string is NUL-terminated (and so is the destination) which means there was no real bug happening here. Nonetheless, this patch would get us one step closer to eliminating the `strncpy` API in the kernel, as its use is too ambiguous. We need to favor less ambiguous replacements such as: strscpy, strscpy_pad, strtomem and strtomem_pad (amongst others). Technically, my patch yields subtly different behavior. The original implementation with `strncpy` would fill the entire destination buffer with null bytes [3] while `strscpy` will leave the junk, uninitialized bytes trailing after the _mandatory_ NUL-termination. So, if somehow `card->driver` or `card->shortname` require this NUL-padding behavior then `strscpy_pad` should be used. My interpretation, though, is that the aforementioned fields are just fine as NUL-terminated strings. Please correct my assumptions if needed and I'll send in a v2. [1]: www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [2]: manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html [3]: https://linux.die.net/man/3/strncpy Link: KSPP#90 Link: https://lore.kernel.org/r/20230727-sound-xen-v1-1-89dd161351f1@google.com (related ALSA patch) Signed-off-by: Justin Stitt <justinstitt@google.com> Link: https://lore.kernel.org/r/20230727-sound-usb-bcd2000-v1-1-0dc73684b2f0@google.com Signed-off-by: Takashi Iwai <tiwai@suse.de>
`strncpy` is deprecated for use on NUL-terminated destination strings [1]. A suitable replacement is `strscpy` [2] due to the fact that it guarantees NUL-termination on its destination buffer argument which is _not_ always the case for `strncpy`! In this case, though, there was care taken to ensure that the destination buffer would be NUL-terminated. The destination buffer is zero-initialized and each `pm860x->name[i]` has a size of `MAX_NAME_LENGTH + 1`. This means that there is unlikely to be a bug here. However, in an attempt to eliminate the usage of the `strncpy` API as well as disambiguate implementations, replacements such as: `strscpy`, `strscpy_pad`, `strtomem` and `strtomem_pad` should be preferred. We are able to eliminate the need for `len + 1` since `strscpy` guarantees NUL-termination for its destination buffer as per its implementation [3]: | /* Hit buffer length without finding a NUL; force NUL-termination. */ | if (res) | dest[res-1] = '\0'; [1]: www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [2]: manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html [3]: https://elixir.bootlin.com/linux/v6.3/source/lib/string.c#L183 Link: KSPP#90 Signed-off-by: Justin Stitt <justinstitt@google.com> Link: https://lore.kernel.org/r/20230727-sound-soc-codecs-v1-1-562fa2836bf4@google.com Signed-off-by: Mark Brown <broonie@kernel.org>
`strncpy` is deprecated for use on NUL-terminated destination strings [1]. A suitable replacement is `strscpy` [2] due to the fact that it guarantees NUL-termination on its destination buffer argument which is _not_ always the case for `strncpy`! In this case, though, there was great care taken to ensure that the destination buffer would be NUL-terminated through the use of `len - 1` ensuring that the previously zero-initialized buffer would not overwrite the last NUL byte. This means that there's no bug here. However, `strscpy` will add a mandatory NUL byte to the destination buffer as promised by the following `strscpy` implementation [3]: | /* Hit buffer length without finding a NUL; force NUL-termination. */ | if (res) | dest[res-1] = '\0'; This means we can lose the `- 1` which clears up whats happening here. All the while, we get one step closer to eliminating the ambiguous `strncpy` api in favor of its less ambiguous replacement like `strscpy`, `strscpy_pad`, `strtomem` and `strtomem_pad` amongst others. [1]: www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [2]: manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html [3]: https://elixir.bootlin.com/linux/v6.3/source/lib/string.c#L183 Link: KSPP#90 Signed-off-by: Justin Stitt <justinstitt@google.com> Acked-by: Shengjiu Wang <shengjiu.wang@gmail.com> Link: https://lore.kernel.org/r/20230727-sound-soc-fsl-v1-1-4fc0ed7e0366@google.com Signed-off-by: Mark Brown <broonie@kernel.org>
… entry Starting with commit c0247d2 ("btrfs: send: annotate struct name_cache_entry with __counted_by()") we annotated the variable length array "name" from the name_cache_entry structure with __counted_by() to improve overflow detection. However that alone was not correct, because the length of that array does not match the "name_len" field - it matches that plus 1 to include the nul string terminator, so that makes a fortified kernel think there's an overflow and report a splat like this: Sep 15 23:32:17 sdslinux1 kernel: ------------[ cut here ]------------ Sep 15 23:32:17 sdslinux1 kernel: strcpy: detected buffer overflow: 20 byte write of buffer size 19 Sep 15 23:32:17 sdslinux1 kernel: WARNING: CPU: 3 PID: 3310 at __fortify_report+0x45/0x50 Sep 15 23:32:17 sdslinux1 kernel: Modules linked in: nfsd auth_rpcgss lockd grace nfs_acl bridge stp llc bonding tls vfat fat binfmt_misc snd_hda_codec_hdmi intel_rapl_msr intel_rapl_common x8 6_pkg_temp_thermal intel_powerclamp kvm_intel iTCO_wdt intel_pmc_bxt spi_intel_platform kvm at24 mei_wdt spi_intel mei_pxp iTCO_vendor_support mei_hdcp btusb snd_hda_codec_realtek btbcm btinte l snd_hda_scodec_component i915 rapl iwlwifi snd_hda_codec_generic btrtl intel_cstate btmtk cec snd_hda_intel intel_uncore cfg80211 snd_intel_dspcfg drm_buddy coretemp snd_intel_sdw_acpi bluet ooth ttm pcspkr snd_hda_codec rfkill snd_hda_core snd_hwdep intel_vbtn snd_pcm mei_me drm_display_helper snd_timer sparse_keymap i2c_i801 mei snd i2c_smbus lpc_ich soundcore cdc_mbim cdc_wdm cdc_ncm cdc_ether usbnet crct10dif_pclmul crc32_pclmul crc32c_intel polyval_clmulni polyval_generic ghash_clmulni_intel sha512_ssse3 sha256_ssse3 sha1_ssse3 igb r8152 serio_raw i2c_algo_bit mii dca e1000e video wmi sunrpc Sep 15 23:32:17 sdslinux1 kernel: CPU: 3 UID: 0 PID: 3310 Comm: btrfs Not tainted 6.11.0-prnet #1 Sep 15 23:32:17 sdslinux1 kernel: Hardware name: CompuLab Ltd. sbc-ihsw/Intense-PC2 (IPC2), BIOS IPC2_3.330.7 X64 03/15/2018 Sep 15 23:32:17 sdslinux1 kernel: RIP: 0010:__fortify_report+0x45/0x50 Sep 15 23:32:17 sdslinux1 kernel: Code: 48 8b 34 (...) Sep 15 23:32:17 sdslinux1 kernel: RSP: 0018:ffff97ebc0d6f650 EFLAGS: 00010246 Sep 15 23:32:17 sdslinux1 kernel: RAX: 7749924ef60fa600 RBX: ffff8bf5446a521a RCX: 0000000000000027 Sep 15 23:32:17 sdslinux1 kernel: RDX: 00000000ffffdfff RSI: ffff97ebc0d6f548 RDI: ffff8bf84e7a1cc8 Sep 15 23:32:17 sdslinux1 kernel: RBP: ffff8bf548574080 R08: ffffffffa8c40e10 R09: 0000000000005ffd Sep 15 23:32:17 sdslinux1 kernel: R10: 0000000000000004 R11: ffffffffa8c70e10 R12: ffff8bf551eef400 Sep 15 23:32:17 sdslinux1 kernel: R13: 0000000000000000 R14: 0000000000000013 R15: 00000000000003a8 Sep 15 23:32:17 sdslinux1 kernel: FS: 00007fae144de8c0(0000) GS:ffff8bf84e780000(0000) knlGS:0000000000000000 Sep 15 23:32:17 sdslinux1 kernel: CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 Sep 15 23:32:17 sdslinux1 kernel: CR2: 00007fae14691690 CR3: 00000001027a2003 CR4: 00000000001706f0 Sep 15 23:32:17 sdslinux1 kernel: Call Trace: Sep 15 23:32:17 sdslinux1 kernel: <TASK> Sep 15 23:32:17 sdslinux1 kernel: ? __warn+0x12a/0x1d0 Sep 15 23:32:17 sdslinux1 kernel: ? __fortify_report+0x45/0x50 Sep 15 23:32:17 sdslinux1 kernel: ? report_bug+0x154/0x1c0 Sep 15 23:32:17 sdslinux1 kernel: ? handle_bug+0x42/0x70 Sep 15 23:32:17 sdslinux1 kernel: ? exc_invalid_op+0x1a/0x50 Sep 15 23:32:17 sdslinux1 kernel: ? asm_exc_invalid_op+0x1a/0x20 Sep 15 23:32:17 sdslinux1 kernel: ? __fortify_report+0x45/0x50 Sep 15 23:32:17 sdslinux1 kernel: __fortify_panic+0x9/0x10 Sep 15 23:32:17 sdslinux1 kernel: __get_cur_name_and_parent+0x3bc/0x3c0 Sep 15 23:32:17 sdslinux1 kernel: get_cur_path+0x207/0x3b0 Sep 15 23:32:17 sdslinux1 kernel: send_extent_data+0x709/0x10d0 Sep 15 23:32:17 sdslinux1 kernel: ? find_parent_nodes+0x22df/0x25d0 Sep 15 23:32:17 sdslinux1 kernel: ? mas_nomem+0x13/0x90 Sep 15 23:32:17 sdslinux1 kernel: ? mtree_insert_range+0xa5/0x110 Sep 15 23:32:17 sdslinux1 kernel: ? btrfs_lru_cache_store+0x5f/0x1e0 Sep 15 23:32:17 sdslinux1 kernel: ? iterate_extent_inodes+0x52d/0x5a0 Sep 15 23:32:17 sdslinux1 kernel: process_extent+0xa96/0x11a0 Sep 15 23:32:17 sdslinux1 kernel: ? __pfx_lookup_backref_cache+0x10/0x10 Sep 15 23:32:17 sdslinux1 kernel: ? __pfx_store_backref_cache+0x10/0x10 Sep 15 23:32:17 sdslinux1 kernel: ? __pfx_iterate_backrefs+0x10/0x10 Sep 15 23:32:17 sdslinux1 kernel: ? __pfx_check_extent_item+0x10/0x10 Sep 15 23:32:17 sdslinux1 kernel: changed_cb+0x6fa/0x930 Sep 15 23:32:17 sdslinux1 kernel: ? tree_advance+0x362/0x390 Sep 15 23:32:17 sdslinux1 kernel: ? memcmp_extent_buffer+0xd7/0x160 Sep 15 23:32:17 sdslinux1 kernel: send_subvol+0xf0a/0x1520 Sep 15 23:32:17 sdslinux1 kernel: btrfs_ioctl_send+0x106b/0x11d0 Sep 15 23:32:17 sdslinux1 kernel: ? __pfx___clone_root_cmp_sort+0x10/0x10 Sep 15 23:32:17 sdslinux1 kernel: _btrfs_ioctl_send+0x1ac/0x240 Sep 15 23:32:17 sdslinux1 kernel: btrfs_ioctl+0x75b/0x850 Sep 15 23:32:17 sdslinux1 kernel: __se_sys_ioctl+0xca/0x150 Sep 15 23:32:17 sdslinux1 kernel: do_syscall_64+0x85/0x160 Sep 15 23:32:17 sdslinux1 kernel: ? __count_memcg_events+0x69/0x100 Sep 15 23:32:17 sdslinux1 kernel: ? handle_mm_fault+0x1327/0x15c0 Sep 15 23:32:17 sdslinux1 kernel: ? __se_sys_rt_sigprocmask+0xf1/0x180 Sep 15 23:32:17 sdslinux1 kernel: ? syscall_exit_to_user_mode+0x75/0xa0 Sep 15 23:32:17 sdslinux1 kernel: ? do_syscall_64+0x91/0x160 Sep 15 23:32:17 sdslinux1 kernel: ? do_user_addr_fault+0x21d/0x630 Sep 15 23:32:17 sdslinux1 kernel: entry_SYSCALL_64_after_hwframe+0x76/0x7e Sep 15 23:32:17 sdslinux1 kernel: RIP: 0033:0x7fae145eeb4f Sep 15 23:32:17 sdslinux1 kernel: Code: 00 48 89 (...) Sep 15 23:32:17 sdslinux1 kernel: RSP: 002b:00007ffdf1cb09b0 EFLAGS: 00000246 ORIG_RAX: 0000000000000010 Sep 15 23:32:17 sdslinux1 kernel: RAX: ffffffffffffffda RBX: 0000000000000004 RCX: 00007fae145eeb4f Sep 15 23:32:17 sdslinux1 kernel: RDX: 00007ffdf1cb0ad0 RSI: 0000000040489426 RDI: 0000000000000004 Sep 15 23:32:17 sdslinux1 kernel: RBP: 00000000000078fe R08: 00007fae144006c0 R09: 00007ffdf1cb0927 Sep 15 23:32:17 sdslinux1 kernel: R10: 0000000000000008 R11: 0000000000000246 R12: 00007ffdf1cb1ce8 Sep 15 23:32:17 sdslinux1 kernel: R13: 0000000000000003 R14: 000055c499fab2e0 R15: 0000000000000004 Sep 15 23:32:17 sdslinux1 kernel: </TASK> Sep 15 23:32:17 sdslinux1 kernel: ---[ end trace 0000000000000000 ]--- Sep 15 23:32:17 sdslinux1 kernel: ------------[ cut here ]------------ Fix this by not storing the nul string terminator since we don't actually need it for name cache entries, this way "name_len" corresponds to the actual size of the "name" array. This requires marking the "name" array field with __nonstring and using memcpy() instead of strcpy() as recommended by the guidelines at: KSPP#90 Fixes: c0247d2 ("btrfs: send: annotate struct name_cache_entry with __counted_by()") Reported-by: David Arendt <admin@prnet.org> Link: https://lore.kernel.org/linux-btrfs/cee4591a-3088-49ba-99b8-d86b4242b8bd@prnet.org/ Tested-by: David Arendt <admin@prnet.org> Reviewed-by: Josef Bacik <josef@toxicpanda.com> Reviewed-by: Qu Wenruo <wqu@suse.com> Signed-off-by: Filipe Manana <fdmanana@suse.com> Signed-off-by: David Sterba <dsterba@suse.com>
… entry Starting with commit c0247d2 ("btrfs: send: annotate struct name_cache_entry with __counted_by()") we annotated the variable length array "name" from the name_cache_entry structure with __counted_by() to improve overflow detection. However that alone was not correct, because the length of that array does not match the "name_len" field - it matches that plus 1 to include the NUL string terminator, so that makes a fortified kernel think there's an overflow and report a splat like this: strcpy: detected buffer overflow: 20 byte write of buffer size 19 WARNING: CPU: 3 PID: 3310 at __fortify_report+0x45/0x50 CPU: 3 UID: 0 PID: 3310 Comm: btrfs Not tainted 6.11.0-prnet #1 Hardware name: CompuLab Ltd. sbc-ihsw/Intense-PC2 (IPC2), BIOS IPC2_3.330.7 X64 03/15/2018 RIP: 0010:__fortify_report+0x45/0x50 Code: 48 8b 34 (...) RSP: 0018:ffff97ebc0d6f650 EFLAGS: 00010246 RAX: 7749924ef60fa600 RBX: ffff8bf5446a521a RCX: 0000000000000027 RDX: 00000000ffffdfff RSI: ffff97ebc0d6f548 RDI: ffff8bf84e7a1cc8 RBP: ffff8bf548574080 R08: ffffffffa8c40e10 R09: 0000000000005ffd R10: 0000000000000004 R11: ffffffffa8c70e10 R12: ffff8bf551eef400 R13: 0000000000000000 R14: 0000000000000013 R15: 00000000000003a8 FS: 00007fae144de8c0(0000) GS:ffff8bf84e780000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: 00007fae14691690 CR3: 00000001027a2003 CR4: 00000000001706f0 Call Trace: <TASK> ? __warn+0x12a/0x1d0 ? __fortify_report+0x45/0x50 ? report_bug+0x154/0x1c0 ? handle_bug+0x42/0x70 ? exc_invalid_op+0x1a/0x50 ? asm_exc_invalid_op+0x1a/0x20 ? __fortify_report+0x45/0x50 __fortify_panic+0x9/0x10 __get_cur_name_and_parent+0x3bc/0x3c0 get_cur_path+0x207/0x3b0 send_extent_data+0x709/0x10d0 ? find_parent_nodes+0x22df/0x25d0 ? mas_nomem+0x13/0x90 ? mtree_insert_range+0xa5/0x110 ? btrfs_lru_cache_store+0x5f/0x1e0 ? iterate_extent_inodes+0x52d/0x5a0 process_extent+0xa96/0x11a0 ? __pfx_lookup_backref_cache+0x10/0x10 ? __pfx_store_backref_cache+0x10/0x10 ? __pfx_iterate_backrefs+0x10/0x10 ? __pfx_check_extent_item+0x10/0x10 changed_cb+0x6fa/0x930 ? tree_advance+0x362/0x390 ? memcmp_extent_buffer+0xd7/0x160 send_subvol+0xf0a/0x1520 btrfs_ioctl_send+0x106b/0x11d0 ? __pfx___clone_root_cmp_sort+0x10/0x10 _btrfs_ioctl_send+0x1ac/0x240 btrfs_ioctl+0x75b/0x850 __se_sys_ioctl+0xca/0x150 do_syscall_64+0x85/0x160 ? __count_memcg_events+0x69/0x100 ? handle_mm_fault+0x1327/0x15c0 ? __se_sys_rt_sigprocmask+0xf1/0x180 ? syscall_exit_to_user_mode+0x75/0xa0 ? do_syscall_64+0x91/0x160 ? do_user_addr_fault+0x21d/0x630 entry_SYSCALL_64_after_hwframe+0x76/0x7e RIP: 0033:0x7fae145eeb4f Code: 00 48 89 (...) RSP: 002b:00007ffdf1cb09b0 EFLAGS: 00000246 ORIG_RAX: 0000000000000010 RAX: ffffffffffffffda RBX: 0000000000000004 RCX: 00007fae145eeb4f RDX: 00007ffdf1cb0ad0 RSI: 0000000040489426 RDI: 0000000000000004 RBP: 00000000000078fe R08: 00007fae144006c0 R09: 00007ffdf1cb0927 R10: 0000000000000008 R11: 0000000000000246 R12: 00007ffdf1cb1ce8 R13: 0000000000000003 R14: 000055c499fab2e0 R15: 0000000000000004 </TASK> Fix this by not storing the NUL string terminator since we don't actually need it for name cache entries, this way "name_len" corresponds to the actual size of the "name" array. This requires marking the "name" array field with __nonstring and using memcpy() instead of strcpy() as recommended by the guidelines at: KSPP#90 Reported-by: David Arendt <admin@prnet.org> Link: https://lore.kernel.org/linux-btrfs/cee4591a-3088-49ba-99b8-d86b4242b8bd@prnet.org/ Fixes: c0247d2 ("btrfs: send: annotate struct name_cache_entry with __counted_by()") CC: stable@vger.kernel.org # 6.11 Tested-by: David Arendt <admin@prnet.org> Reviewed-by: Josef Bacik <josef@toxicpanda.com> Reviewed-by: Qu Wenruo <wqu@suse.com> Signed-off-by: Filipe Manana <fdmanana@suse.com> Reviewed-by: David Sterba <dsterba@suse.com> Signed-off-by: David Sterba <dsterba@suse.com>
… entry Starting with commit c0247d2 ("btrfs: send: annotate struct name_cache_entry with __counted_by()") we annotated the variable length array "name" from the name_cache_entry structure with __counted_by() to improve overflow detection. However that alone was not correct, because the length of that array does not match the "name_len" field - it matches that plus 1 to include the nul string terminador, so that makes a fortified kernel think there's an overflow and report a splat like this: Sep 15 23:32:17 sdslinux1 kernel: ------------[ cut here ]------------ Sep 15 23:32:17 sdslinux1 kernel: strcpy: detected buffer overflow: 20 byte write of buffer size 19 Sep 15 23:32:17 sdslinux1 kernel: WARNING: CPU: 3 PID: 3310 at __fortify_report+0x45/0x50 Sep 15 23:32:17 sdslinux1 kernel: Modules linked in: nfsd auth_rpcgss lockd grace nfs_acl bridge stp llc bonding tls vfat fat binfmt_misc snd_hda_codec_hdmi intel_rapl_msr intel_rapl_common x8 6_pkg_temp_thermal intel_powerclamp kvm_intel iTCO_wdt intel_pmc_bxt spi_intel_platform kvm at24 mei_wdt spi_intel mei_pxp iTCO_vendor_support mei_hdcp btusb snd_hda_codec_realtek btbcm btinte l snd_hda_scodec_component i915 rapl iwlwifi snd_hda_codec_generic btrtl intel_cstate btmtk cec snd_hda_intel intel_uncore cfg80211 snd_intel_dspcfg drm_buddy coretemp snd_intel_sdw_acpi bluet ooth ttm pcspkr snd_hda_codec rfkill snd_hda_core snd_hwdep intel_vbtn snd_pcm mei_me drm_display_helper snd_timer sparse_keymap i2c_i801 mei snd i2c_smbus lpc_ich soundcore cdc_mbim cdc_wdm cdc_ncm cdc_ether usbnet crct10dif_pclmul crc32_pclmul crc32c_intel polyval_clmulni polyval_generic ghash_clmulni_intel sha512_ssse3 sha256_ssse3 sha1_ssse3 igb r8152 serio_raw i2c_algo_bit mii dca e1000e video wmi sunrpc Sep 15 23:32:17 sdslinux1 kernel: CPU: 3 UID: 0 PID: 3310 Comm: btrfs Not tainted 6.11.0-prnet #1 Sep 15 23:32:17 sdslinux1 kernel: Hardware name: CompuLab Ltd. sbc-ihsw/Intense-PC2 (IPC2), BIOS IPC2_3.330.7 X64 03/15/2018 Sep 15 23:32:17 sdslinux1 kernel: RIP: 0010:__fortify_report+0x45/0x50 Sep 15 23:32:17 sdslinux1 kernel: Code: 48 8b 34 (...) Sep 15 23:32:17 sdslinux1 kernel: RSP: 0018:ffff97ebc0d6f650 EFLAGS: 00010246 Sep 15 23:32:17 sdslinux1 kernel: RAX: 7749924ef60fa600 RBX: ffff8bf5446a521a RCX: 0000000000000027 Sep 15 23:32:17 sdslinux1 kernel: RDX: 00000000ffffdfff RSI: ffff97ebc0d6f548 RDI: ffff8bf84e7a1cc8 Sep 15 23:32:17 sdslinux1 kernel: RBP: ffff8bf548574080 R08: ffffffffa8c40e10 R09: 0000000000005ffd Sep 15 23:32:17 sdslinux1 kernel: R10: 0000000000000004 R11: ffffffffa8c70e10 R12: ffff8bf551eef400 Sep 15 23:32:17 sdslinux1 kernel: R13: 0000000000000000 R14: 0000000000000013 R15: 00000000000003a8 Sep 15 23:32:17 sdslinux1 kernel: FS: 00007fae144de8c0(0000) GS:ffff8bf84e780000(0000) knlGS:0000000000000000 Sep 15 23:32:17 sdslinux1 kernel: CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 Sep 15 23:32:17 sdslinux1 kernel: CR2: 00007fae14691690 CR3: 00000001027a2003 CR4: 00000000001706f0 Sep 15 23:32:17 sdslinux1 kernel: Call Trace: Sep 15 23:32:17 sdslinux1 kernel: <TASK> Sep 15 23:32:17 sdslinux1 kernel: ? __warn+0x12a/0x1d0 Sep 15 23:32:17 sdslinux1 kernel: ? __fortify_report+0x45/0x50 Sep 15 23:32:17 sdslinux1 kernel: ? report_bug+0x154/0x1c0 Sep 15 23:32:17 sdslinux1 kernel: ? handle_bug+0x42/0x70 Sep 15 23:32:17 sdslinux1 kernel: ? exc_invalid_op+0x1a/0x50 Sep 15 23:32:17 sdslinux1 kernel: ? asm_exc_invalid_op+0x1a/0x20 Sep 15 23:32:17 sdslinux1 kernel: ? __fortify_report+0x45/0x50 Sep 15 23:32:17 sdslinux1 kernel: __fortify_panic+0x9/0x10 Sep 15 23:32:17 sdslinux1 kernel: __get_cur_name_and_parent+0x3bc/0x3c0 Sep 15 23:32:17 sdslinux1 kernel: get_cur_path+0x207/0x3b0 Sep 15 23:32:17 sdslinux1 kernel: send_extent_data+0x709/0x10d0 Sep 15 23:32:17 sdslinux1 kernel: ? find_parent_nodes+0x22df/0x25d0 Sep 15 23:32:17 sdslinux1 kernel: ? mas_nomem+0x13/0x90 Sep 15 23:32:17 sdslinux1 kernel: ? mtree_insert_range+0xa5/0x110 Sep 15 23:32:17 sdslinux1 kernel: ? btrfs_lru_cache_store+0x5f/0x1e0 Sep 15 23:32:17 sdslinux1 kernel: ? iterate_extent_inodes+0x52d/0x5a0 Sep 15 23:32:17 sdslinux1 kernel: process_extent+0xa96/0x11a0 Sep 15 23:32:17 sdslinux1 kernel: ? __pfx_lookup_backref_cache+0x10/0x10 Sep 15 23:32:17 sdslinux1 kernel: ? __pfx_store_backref_cache+0x10/0x10 Sep 15 23:32:17 sdslinux1 kernel: ? __pfx_iterate_backrefs+0x10/0x10 Sep 15 23:32:17 sdslinux1 kernel: ? __pfx_check_extent_item+0x10/0x10 Sep 15 23:32:17 sdslinux1 kernel: changed_cb+0x6fa/0x930 Sep 15 23:32:17 sdslinux1 kernel: ? tree_advance+0x362/0x390 Sep 15 23:32:17 sdslinux1 kernel: ? memcmp_extent_buffer+0xd7/0x160 Sep 15 23:32:17 sdslinux1 kernel: send_subvol+0xf0a/0x1520 Sep 15 23:32:17 sdslinux1 kernel: btrfs_ioctl_send+0x106b/0x11d0 Sep 15 23:32:17 sdslinux1 kernel: ? __pfx___clone_root_cmp_sort+0x10/0x10 Sep 15 23:32:17 sdslinux1 kernel: _btrfs_ioctl_send+0x1ac/0x240 Sep 15 23:32:17 sdslinux1 kernel: btrfs_ioctl+0x75b/0x850 Sep 15 23:32:17 sdslinux1 kernel: __se_sys_ioctl+0xca/0x150 Sep 15 23:32:17 sdslinux1 kernel: do_syscall_64+0x85/0x160 Sep 15 23:32:17 sdslinux1 kernel: ? __count_memcg_events+0x69/0x100 Sep 15 23:32:17 sdslinux1 kernel: ? handle_mm_fault+0x1327/0x15c0 Sep 15 23:32:17 sdslinux1 kernel: ? __se_sys_rt_sigprocmask+0xf1/0x180 Sep 15 23:32:17 sdslinux1 kernel: ? syscall_exit_to_user_mode+0x75/0xa0 Sep 15 23:32:17 sdslinux1 kernel: ? do_syscall_64+0x91/0x160 Sep 15 23:32:17 sdslinux1 kernel: ? do_user_addr_fault+0x21d/0x630 Sep 15 23:32:17 sdslinux1 kernel: entry_SYSCALL_64_after_hwframe+0x76/0x7e Sep 15 23:32:17 sdslinux1 kernel: RIP: 0033:0x7fae145eeb4f Sep 15 23:32:17 sdslinux1 kernel: Code: 00 48 89 (...) Sep 15 23:32:17 sdslinux1 kernel: RSP: 002b:00007ffdf1cb09b0 EFLAGS: 00000246 ORIG_RAX: 0000000000000010 Sep 15 23:32:17 sdslinux1 kernel: RAX: ffffffffffffffda RBX: 0000000000000004 RCX: 00007fae145eeb4f Sep 15 23:32:17 sdslinux1 kernel: RDX: 00007ffdf1cb0ad0 RSI: 0000000040489426 RDI: 0000000000000004 Sep 15 23:32:17 sdslinux1 kernel: RBP: 00000000000078fe R08: 00007fae144006c0 R09: 00007ffdf1cb0927 Sep 15 23:32:17 sdslinux1 kernel: R10: 0000000000000008 R11: 0000000000000246 R12: 00007ffdf1cb1ce8 Sep 15 23:32:17 sdslinux1 kernel: R13: 0000000000000003 R14: 000055c499fab2e0 R15: 0000000000000004 Sep 15 23:32:17 sdslinux1 kernel: </TASK> Sep 15 23:32:17 sdslinux1 kernel: ---[ end trace 0000000000000000 ]--- Sep 15 23:32:17 sdslinux1 kernel: ------------[ cut here ]------------ Fix this by not storing the nul string terminator since we don't actually need it for name cache entries, this way "name_len" corresponds to the actual size of the "name" array. This requires marking the "name" array field with __nonstring and using memcpy() instead of strcpy() as recommended by the guidelines at: KSPP#90 Fixes: c0247d2 ("btrfs: send: annotate struct name_cache_entry with __counted_by()") Reported-by: David Arendt <admin@prnet.org> Link: https://lore.kernel.org/linux-btrfs/cee4591a-3088-49ba-99b8-d86b4242b8bd@prnet.org/ Tested-by: David Arendt <admin@prnet.org> Signed-off-by: Filipe Manana <fdmanana@suse.com>
… entry Starting with commit c0247d2 ("btrfs: send: annotate struct name_cache_entry with __counted_by()") we annotated the variable length array "name" from the name_cache_entry structure with __counted_by() to improve overflow detection. However that alone was not correct, because the length of that array does not match the "name_len" field - it matches that plus 1 to include the nul string terminador, so that makes a fortified kernel think there's an overflow and report a splat like this: Sep 15 23:32:17 sdslinux1 kernel: ------------[ cut here ]------------ Sep 15 23:32:17 sdslinux1 kernel: strcpy: detected buffer overflow: 20 byte write of buffer size 19 Sep 15 23:32:17 sdslinux1 kernel: WARNING: CPU: 3 PID: 3310 at __fortify_report+0x45/0x50 Sep 15 23:32:17 sdslinux1 kernel: Modules linked in: nfsd auth_rpcgss lockd grace nfs_acl bridge stp llc bonding tls vfat fat binfmt_misc snd_hda_codec_hdmi intel_rapl_msr intel_rapl_common x8 6_pkg_temp_thermal intel_powerclamp kvm_intel iTCO_wdt intel_pmc_bxt spi_intel_platform kvm at24 mei_wdt spi_intel mei_pxp iTCO_vendor_support mei_hdcp btusb snd_hda_codec_realtek btbcm btinte l snd_hda_scodec_component i915 rapl iwlwifi snd_hda_codec_generic btrtl intel_cstate btmtk cec snd_hda_intel intel_uncore cfg80211 snd_intel_dspcfg drm_buddy coretemp snd_intel_sdw_acpi bluet ooth ttm pcspkr snd_hda_codec rfkill snd_hda_core snd_hwdep intel_vbtn snd_pcm mei_me drm_display_helper snd_timer sparse_keymap i2c_i801 mei snd i2c_smbus lpc_ich soundcore cdc_mbim cdc_wdm cdc_ncm cdc_ether usbnet crct10dif_pclmul crc32_pclmul crc32c_intel polyval_clmulni polyval_generic ghash_clmulni_intel sha512_ssse3 sha256_ssse3 sha1_ssse3 igb r8152 serio_raw i2c_algo_bit mii dca e1000e video wmi sunrpc Sep 15 23:32:17 sdslinux1 kernel: CPU: 3 UID: 0 PID: 3310 Comm: btrfs Not tainted 6.11.0-prnet #1 Sep 15 23:32:17 sdslinux1 kernel: Hardware name: CompuLab Ltd. sbc-ihsw/Intense-PC2 (IPC2), BIOS IPC2_3.330.7 X64 03/15/2018 Sep 15 23:32:17 sdslinux1 kernel: RIP: 0010:__fortify_report+0x45/0x50 Sep 15 23:32:17 sdslinux1 kernel: Code: 48 8b 34 (...) Sep 15 23:32:17 sdslinux1 kernel: RSP: 0018:ffff97ebc0d6f650 EFLAGS: 00010246 Sep 15 23:32:17 sdslinux1 kernel: RAX: 7749924ef60fa600 RBX: ffff8bf5446a521a RCX: 0000000000000027 Sep 15 23:32:17 sdslinux1 kernel: RDX: 00000000ffffdfff RSI: ffff97ebc0d6f548 RDI: ffff8bf84e7a1cc8 Sep 15 23:32:17 sdslinux1 kernel: RBP: ffff8bf548574080 R08: ffffffffa8c40e10 R09: 0000000000005ffd Sep 15 23:32:17 sdslinux1 kernel: R10: 0000000000000004 R11: ffffffffa8c70e10 R12: ffff8bf551eef400 Sep 15 23:32:17 sdslinux1 kernel: R13: 0000000000000000 R14: 0000000000000013 R15: 00000000000003a8 Sep 15 23:32:17 sdslinux1 kernel: FS: 00007fae144de8c0(0000) GS:ffff8bf84e780000(0000) knlGS:0000000000000000 Sep 15 23:32:17 sdslinux1 kernel: CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 Sep 15 23:32:17 sdslinux1 kernel: CR2: 00007fae14691690 CR3: 00000001027a2003 CR4: 00000000001706f0 Sep 15 23:32:17 sdslinux1 kernel: Call Trace: Sep 15 23:32:17 sdslinux1 kernel: <TASK> Sep 15 23:32:17 sdslinux1 kernel: ? __warn+0x12a/0x1d0 Sep 15 23:32:17 sdslinux1 kernel: ? __fortify_report+0x45/0x50 Sep 15 23:32:17 sdslinux1 kernel: ? report_bug+0x154/0x1c0 Sep 15 23:32:17 sdslinux1 kernel: ? handle_bug+0x42/0x70 Sep 15 23:32:17 sdslinux1 kernel: ? exc_invalid_op+0x1a/0x50 Sep 15 23:32:17 sdslinux1 kernel: ? asm_exc_invalid_op+0x1a/0x20 Sep 15 23:32:17 sdslinux1 kernel: ? __fortify_report+0x45/0x50 Sep 15 23:32:17 sdslinux1 kernel: __fortify_panic+0x9/0x10 Sep 15 23:32:17 sdslinux1 kernel: __get_cur_name_and_parent+0x3bc/0x3c0 Sep 15 23:32:17 sdslinux1 kernel: get_cur_path+0x207/0x3b0 Sep 15 23:32:17 sdslinux1 kernel: send_extent_data+0x709/0x10d0 Sep 15 23:32:17 sdslinux1 kernel: ? find_parent_nodes+0x22df/0x25d0 Sep 15 23:32:17 sdslinux1 kernel: ? mas_nomem+0x13/0x90 Sep 15 23:32:17 sdslinux1 kernel: ? mtree_insert_range+0xa5/0x110 Sep 15 23:32:17 sdslinux1 kernel: ? btrfs_lru_cache_store+0x5f/0x1e0 Sep 15 23:32:17 sdslinux1 kernel: ? iterate_extent_inodes+0x52d/0x5a0 Sep 15 23:32:17 sdslinux1 kernel: process_extent+0xa96/0x11a0 Sep 15 23:32:17 sdslinux1 kernel: ? __pfx_lookup_backref_cache+0x10/0x10 Sep 15 23:32:17 sdslinux1 kernel: ? __pfx_store_backref_cache+0x10/0x10 Sep 15 23:32:17 sdslinux1 kernel: ? __pfx_iterate_backrefs+0x10/0x10 Sep 15 23:32:17 sdslinux1 kernel: ? __pfx_check_extent_item+0x10/0x10 Sep 15 23:32:17 sdslinux1 kernel: changed_cb+0x6fa/0x930 Sep 15 23:32:17 sdslinux1 kernel: ? tree_advance+0x362/0x390 Sep 15 23:32:17 sdslinux1 kernel: ? memcmp_extent_buffer+0xd7/0x160 Sep 15 23:32:17 sdslinux1 kernel: send_subvol+0xf0a/0x1520 Sep 15 23:32:17 sdslinux1 kernel: btrfs_ioctl_send+0x106b/0x11d0 Sep 15 23:32:17 sdslinux1 kernel: ? __pfx___clone_root_cmp_sort+0x10/0x10 Sep 15 23:32:17 sdslinux1 kernel: _btrfs_ioctl_send+0x1ac/0x240 Sep 15 23:32:17 sdslinux1 kernel: btrfs_ioctl+0x75b/0x850 Sep 15 23:32:17 sdslinux1 kernel: __se_sys_ioctl+0xca/0x150 Sep 15 23:32:17 sdslinux1 kernel: do_syscall_64+0x85/0x160 Sep 15 23:32:17 sdslinux1 kernel: ? __count_memcg_events+0x69/0x100 Sep 15 23:32:17 sdslinux1 kernel: ? handle_mm_fault+0x1327/0x15c0 Sep 15 23:32:17 sdslinux1 kernel: ? __se_sys_rt_sigprocmask+0xf1/0x180 Sep 15 23:32:17 sdslinux1 kernel: ? syscall_exit_to_user_mode+0x75/0xa0 Sep 15 23:32:17 sdslinux1 kernel: ? do_syscall_64+0x91/0x160 Sep 15 23:32:17 sdslinux1 kernel: ? do_user_addr_fault+0x21d/0x630 Sep 15 23:32:17 sdslinux1 kernel: entry_SYSCALL_64_after_hwframe+0x76/0x7e Sep 15 23:32:17 sdslinux1 kernel: RIP: 0033:0x7fae145eeb4f Sep 15 23:32:17 sdslinux1 kernel: Code: 00 48 89 (...) Sep 15 23:32:17 sdslinux1 kernel: RSP: 002b:00007ffdf1cb09b0 EFLAGS: 00000246 ORIG_RAX: 0000000000000010 Sep 15 23:32:17 sdslinux1 kernel: RAX: ffffffffffffffda RBX: 0000000000000004 RCX: 00007fae145eeb4f Sep 15 23:32:17 sdslinux1 kernel: RDX: 00007ffdf1cb0ad0 RSI: 0000000040489426 RDI: 0000000000000004 Sep 15 23:32:17 sdslinux1 kernel: RBP: 00000000000078fe R08: 00007fae144006c0 R09: 00007ffdf1cb0927 Sep 15 23:32:17 sdslinux1 kernel: R10: 0000000000000008 R11: 0000000000000246 R12: 00007ffdf1cb1ce8 Sep 15 23:32:17 sdslinux1 kernel: R13: 0000000000000003 R14: 000055c499fab2e0 R15: 0000000000000004 Sep 15 23:32:17 sdslinux1 kernel: </TASK> Sep 15 23:32:17 sdslinux1 kernel: ---[ end trace 0000000000000000 ]--- Sep 15 23:32:17 sdslinux1 kernel: ------------[ cut here ]------------ Fix this by not storing the nul string terminator since we don't actually need it for name cache entries, this way "name_len" corresponds to the actual size of the "name" array. This requires marking the "name" array field with __nonstring and using memcpy() instead of strcpy() as recommended by the guidelines at: KSPP#90 Fixes: c0247d2 ("btrfs: send: annotate struct name_cache_entry with __counted_by()") Reported-by: David Arendt <admin@prnet.org> Link: https://lore.kernel.org/linux-btrfs/cee4591a-3088-49ba-99b8-d86b4242b8bd@prnet.org/ Tested-by: David Arendt <admin@prnet.org> Signed-off-by: Filipe Manana <fdmanana@suse.com>
… entry Starting with commit c0247d2 ("btrfs: send: annotate struct name_cache_entry with __counted_by()") we annotated the variable length array "name" from the name_cache_entry structure with __counted_by() to improve overflow detection. However that alone was not correct, because the length of that array does not match the "name_len" field - it matches that plus 1 to include the NUL string terminator, so that makes a fortified kernel think there's an overflow and report a splat like this: strcpy: detected buffer overflow: 20 byte write of buffer size 19 WARNING: CPU: 3 PID: 3310 at __fortify_report+0x45/0x50 CPU: 3 UID: 0 PID: 3310 Comm: btrfs Not tainted 6.11.0-prnet #1 Hardware name: CompuLab Ltd. sbc-ihsw/Intense-PC2 (IPC2), BIOS IPC2_3.330.7 X64 03/15/2018 RIP: 0010:__fortify_report+0x45/0x50 Code: 48 8b 34 (...) RSP: 0018:ffff97ebc0d6f650 EFLAGS: 00010246 RAX: 7749924ef60fa600 RBX: ffff8bf5446a521a RCX: 0000000000000027 RDX: 00000000ffffdfff RSI: ffff97ebc0d6f548 RDI: ffff8bf84e7a1cc8 RBP: ffff8bf548574080 R08: ffffffffa8c40e10 R09: 0000000000005ffd R10: 0000000000000004 R11: ffffffffa8c70e10 R12: ffff8bf551eef400 R13: 0000000000000000 R14: 0000000000000013 R15: 00000000000003a8 FS: 00007fae144de8c0(0000) GS:ffff8bf84e780000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: 00007fae14691690 CR3: 00000001027a2003 CR4: 00000000001706f0 Call Trace: <TASK> ? __warn+0x12a/0x1d0 ? __fortify_report+0x45/0x50 ? report_bug+0x154/0x1c0 ? handle_bug+0x42/0x70 ? exc_invalid_op+0x1a/0x50 ? asm_exc_invalid_op+0x1a/0x20 ? __fortify_report+0x45/0x50 __fortify_panic+0x9/0x10 __get_cur_name_and_parent+0x3bc/0x3c0 get_cur_path+0x207/0x3b0 send_extent_data+0x709/0x10d0 ? find_parent_nodes+0x22df/0x25d0 ? mas_nomem+0x13/0x90 ? mtree_insert_range+0xa5/0x110 ? btrfs_lru_cache_store+0x5f/0x1e0 ? iterate_extent_inodes+0x52d/0x5a0 process_extent+0xa96/0x11a0 ? __pfx_lookup_backref_cache+0x10/0x10 ? __pfx_store_backref_cache+0x10/0x10 ? __pfx_iterate_backrefs+0x10/0x10 ? __pfx_check_extent_item+0x10/0x10 changed_cb+0x6fa/0x930 ? tree_advance+0x362/0x390 ? memcmp_extent_buffer+0xd7/0x160 send_subvol+0xf0a/0x1520 btrfs_ioctl_send+0x106b/0x11d0 ? __pfx___clone_root_cmp_sort+0x10/0x10 _btrfs_ioctl_send+0x1ac/0x240 btrfs_ioctl+0x75b/0x850 __se_sys_ioctl+0xca/0x150 do_syscall_64+0x85/0x160 ? __count_memcg_events+0x69/0x100 ? handle_mm_fault+0x1327/0x15c0 ? __se_sys_rt_sigprocmask+0xf1/0x180 ? syscall_exit_to_user_mode+0x75/0xa0 ? do_syscall_64+0x91/0x160 ? do_user_addr_fault+0x21d/0x630 entry_SYSCALL_64_after_hwframe+0x76/0x7e RIP: 0033:0x7fae145eeb4f Code: 00 48 89 (...) RSP: 002b:00007ffdf1cb09b0 EFLAGS: 00000246 ORIG_RAX: 0000000000000010 RAX: ffffffffffffffda RBX: 0000000000000004 RCX: 00007fae145eeb4f RDX: 00007ffdf1cb0ad0 RSI: 0000000040489426 RDI: 0000000000000004 RBP: 00000000000078fe R08: 00007fae144006c0 R09: 00007ffdf1cb0927 R10: 0000000000000008 R11: 0000000000000246 R12: 00007ffdf1cb1ce8 R13: 0000000000000003 R14: 000055c499fab2e0 R15: 0000000000000004 </TASK> Fix this by not storing the NUL string terminator since we don't actually need it for name cache entries, this way "name_len" corresponds to the actual size of the "name" array. This requires marking the "name" array field with __nonstring and using memcpy() instead of strcpy() as recommended by the guidelines at: KSPP/linux#90 Reported-by: David Arendt <admin@prnet.org> Link: https://lore.kernel.org/linux-btrfs/cee4591a-3088-49ba-99b8-d86b4242b8bd@prnet.org/ Fixes: c0247d2 ("btrfs: send: annotate struct name_cache_entry with __counted_by()") CC: stable@vger.kernel.org # 6.11 Tested-by: David Arendt <admin@prnet.org> Reviewed-by: Josef Bacik <josef@toxicpanda.com> Reviewed-by: Qu Wenruo <wqu@suse.com> Signed-off-by: Filipe Manana <fdmanana@suse.com> Reviewed-by: David Sterba <dsterba@suse.com> Signed-off-by: David Sterba <dsterba@suse.com>
BugLink: https://bugs.launchpad.net/bugs/2078428 [ Upstream commit b0009b8 ] strncpy() is deprecated for use on NUL-terminated destination strings [1] and as such we should prefer more robust and less ambiguous string interfaces. We expect bdi->model_name to be NUL-terminated based on its usage with sysfs_emit and format strings: val->strval is assigned to bdi->model_name in bq24190_charger_get_property(): 1186 | val->strval = bdi->model_name; ... then in power_supply_sysfs.c we use value.strval with a format string: 311 | ret = sysfs_emit(buf, "%s\n", value.strval); we assigned value.strval via: 285 | ret = power_supply_get_property(psy, psp, &value); ... which invokes psy->desc->get_property(): 1210 | return psy->desc->get_property(psy, psp, val); with bq24190_charger_get_property(): 1320 | static const struct power_supply_desc bq24190_charger_desc = { ... 1325 | .get_property = bq24190_charger_get_property, Moreover, no NUL-padding is required as bdi is zero-allocated in bq24190_charger.c: 1798 | bdi = devm_kzalloc(dev, sizeof(*bdi), GFP_KERNEL); Considering the above, a suitable replacement is `strscpy` [2] due to the fact that it guarantees NUL-termination on the destination buffer without unnecessarily NUL-padding. Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1] Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html [2] Link: KSPP/linux#90 Cc: linux-hardening@vger.kernel.org Signed-off-by: Justin Stitt <justinstitt@google.com> Reviewed-by: Kees Cook <keescook@chromium.org> Link: https://lore.kernel.org/r/20231020-strncpy-drivers-power-supply-bq24190_charger-c-v1-1-e896223cb795@google.com Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com> Signed-off-by: Sasha Levin <sashal@kernel.org> Signed-off-by: Koichiro Den <koichiro.den@canonical.com> Signed-off-by: Stefan Bader <stefan.bader@canonical.com>
… entry commit 96c6ca7 upstream. Starting with commit c0247d2 ("btrfs: send: annotate struct name_cache_entry with __counted_by()") we annotated the variable length array "name" from the name_cache_entry structure with __counted_by() to improve overflow detection. However that alone was not correct, because the length of that array does not match the "name_len" field - it matches that plus 1 to include the NUL string terminator, so that makes a fortified kernel think there's an overflow and report a splat like this: strcpy: detected buffer overflow: 20 byte write of buffer size 19 WARNING: CPU: 3 PID: 3310 at __fortify_report+0x45/0x50 CPU: 3 UID: 0 PID: 3310 Comm: btrfs Not tainted 6.11.0-prnet #1 Hardware name: CompuLab Ltd. sbc-ihsw/Intense-PC2 (IPC2), BIOS IPC2_3.330.7 X64 03/15/2018 RIP: 0010:__fortify_report+0x45/0x50 Code: 48 8b 34 (...) RSP: 0018:ffff97ebc0d6f650 EFLAGS: 00010246 RAX: 7749924ef60fa600 RBX: ffff8bf5446a521a RCX: 0000000000000027 RDX: 00000000ffffdfff RSI: ffff97ebc0d6f548 RDI: ffff8bf84e7a1cc8 RBP: ffff8bf548574080 R08: ffffffffa8c40e10 R09: 0000000000005ffd R10: 0000000000000004 R11: ffffffffa8c70e10 R12: ffff8bf551eef400 R13: 0000000000000000 R14: 0000000000000013 R15: 00000000000003a8 FS: 00007fae144de8c0(0000) GS:ffff8bf84e780000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: 00007fae14691690 CR3: 00000001027a2003 CR4: 00000000001706f0 Call Trace: <TASK> ? __warn+0x12a/0x1d0 ? __fortify_report+0x45/0x50 ? report_bug+0x154/0x1c0 ? handle_bug+0x42/0x70 ? exc_invalid_op+0x1a/0x50 ? asm_exc_invalid_op+0x1a/0x20 ? __fortify_report+0x45/0x50 __fortify_panic+0x9/0x10 __get_cur_name_and_parent+0x3bc/0x3c0 get_cur_path+0x207/0x3b0 send_extent_data+0x709/0x10d0 ? find_parent_nodes+0x22df/0x25d0 ? mas_nomem+0x13/0x90 ? mtree_insert_range+0xa5/0x110 ? btrfs_lru_cache_store+0x5f/0x1e0 ? iterate_extent_inodes+0x52d/0x5a0 process_extent+0xa96/0x11a0 ? __pfx_lookup_backref_cache+0x10/0x10 ? __pfx_store_backref_cache+0x10/0x10 ? __pfx_iterate_backrefs+0x10/0x10 ? __pfx_check_extent_item+0x10/0x10 changed_cb+0x6fa/0x930 ? tree_advance+0x362/0x390 ? memcmp_extent_buffer+0xd7/0x160 send_subvol+0xf0a/0x1520 btrfs_ioctl_send+0x106b/0x11d0 ? __pfx___clone_root_cmp_sort+0x10/0x10 _btrfs_ioctl_send+0x1ac/0x240 btrfs_ioctl+0x75b/0x850 __se_sys_ioctl+0xca/0x150 do_syscall_64+0x85/0x160 ? __count_memcg_events+0x69/0x100 ? handle_mm_fault+0x1327/0x15c0 ? __se_sys_rt_sigprocmask+0xf1/0x180 ? syscall_exit_to_user_mode+0x75/0xa0 ? do_syscall_64+0x91/0x160 ? do_user_addr_fault+0x21d/0x630 entry_SYSCALL_64_after_hwframe+0x76/0x7e RIP: 0033:0x7fae145eeb4f Code: 00 48 89 (...) RSP: 002b:00007ffdf1cb09b0 EFLAGS: 00000246 ORIG_RAX: 0000000000000010 RAX: ffffffffffffffda RBX: 0000000000000004 RCX: 00007fae145eeb4f RDX: 00007ffdf1cb0ad0 RSI: 0000000040489426 RDI: 0000000000000004 RBP: 00000000000078fe R08: 00007fae144006c0 R09: 00007ffdf1cb0927 R10: 0000000000000008 R11: 0000000000000246 R12: 00007ffdf1cb1ce8 R13: 0000000000000003 R14: 000055c499fab2e0 R15: 0000000000000004 </TASK> Fix this by not storing the NUL string terminator since we don't actually need it for name cache entries, this way "name_len" corresponds to the actual size of the "name" array. This requires marking the "name" array field with __nonstring and using memcpy() instead of strcpy() as recommended by the guidelines at: KSPP/linux#90 Reported-by: David Arendt <admin@prnet.org> Link: https://lore.kernel.org/linux-btrfs/cee4591a-3088-49ba-99b8-d86b4242b8bd@prnet.org/ Fixes: c0247d2 ("btrfs: send: annotate struct name_cache_entry with __counted_by()") CC: stable@vger.kernel.org # 6.11 Tested-by: David Arendt <admin@prnet.org> Reviewed-by: Josef Bacik <josef@toxicpanda.com> Reviewed-by: Qu Wenruo <wqu@suse.com> Signed-off-by: Filipe Manana <fdmanana@suse.com> Reviewed-by: David Sterba <dsterba@suse.com> Signed-off-by: David Sterba <dsterba@suse.com> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
… entry commit 96c6ca7 upstream. Starting with commit c0247d2 ("btrfs: send: annotate struct name_cache_entry with __counted_by()") we annotated the variable length array "name" from the name_cache_entry structure with __counted_by() to improve overflow detection. However that alone was not correct, because the length of that array does not match the "name_len" field - it matches that plus 1 to include the NUL string terminator, so that makes a fortified kernel think there's an overflow and report a splat like this: strcpy: detected buffer overflow: 20 byte write of buffer size 19 WARNING: CPU: 3 PID: 3310 at __fortify_report+0x45/0x50 CPU: 3 UID: 0 PID: 3310 Comm: btrfs Not tainted 6.11.0-prnet #1 Hardware name: CompuLab Ltd. sbc-ihsw/Intense-PC2 (IPC2), BIOS IPC2_3.330.7 X64 03/15/2018 RIP: 0010:__fortify_report+0x45/0x50 Code: 48 8b 34 (...) RSP: 0018:ffff97ebc0d6f650 EFLAGS: 00010246 RAX: 7749924ef60fa600 RBX: ffff8bf5446a521a RCX: 0000000000000027 RDX: 00000000ffffdfff RSI: ffff97ebc0d6f548 RDI: ffff8bf84e7a1cc8 RBP: ffff8bf548574080 R08: ffffffffa8c40e10 R09: 0000000000005ffd R10: 0000000000000004 R11: ffffffffa8c70e10 R12: ffff8bf551eef400 R13: 0000000000000000 R14: 0000000000000013 R15: 00000000000003a8 FS: 00007fae144de8c0(0000) GS:ffff8bf84e780000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: 00007fae14691690 CR3: 00000001027a2003 CR4: 00000000001706f0 Call Trace: <TASK> ? __warn+0x12a/0x1d0 ? __fortify_report+0x45/0x50 ? report_bug+0x154/0x1c0 ? handle_bug+0x42/0x70 ? exc_invalid_op+0x1a/0x50 ? asm_exc_invalid_op+0x1a/0x20 ? __fortify_report+0x45/0x50 __fortify_panic+0x9/0x10 __get_cur_name_and_parent+0x3bc/0x3c0 get_cur_path+0x207/0x3b0 send_extent_data+0x709/0x10d0 ? find_parent_nodes+0x22df/0x25d0 ? mas_nomem+0x13/0x90 ? mtree_insert_range+0xa5/0x110 ? btrfs_lru_cache_store+0x5f/0x1e0 ? iterate_extent_inodes+0x52d/0x5a0 process_extent+0xa96/0x11a0 ? __pfx_lookup_backref_cache+0x10/0x10 ? __pfx_store_backref_cache+0x10/0x10 ? __pfx_iterate_backrefs+0x10/0x10 ? __pfx_check_extent_item+0x10/0x10 changed_cb+0x6fa/0x930 ? tree_advance+0x362/0x390 ? memcmp_extent_buffer+0xd7/0x160 send_subvol+0xf0a/0x1520 btrfs_ioctl_send+0x106b/0x11d0 ? __pfx___clone_root_cmp_sort+0x10/0x10 _btrfs_ioctl_send+0x1ac/0x240 btrfs_ioctl+0x75b/0x850 __se_sys_ioctl+0xca/0x150 do_syscall_64+0x85/0x160 ? __count_memcg_events+0x69/0x100 ? handle_mm_fault+0x1327/0x15c0 ? __se_sys_rt_sigprocmask+0xf1/0x180 ? syscall_exit_to_user_mode+0x75/0xa0 ? do_syscall_64+0x91/0x160 ? do_user_addr_fault+0x21d/0x630 entry_SYSCALL_64_after_hwframe+0x76/0x7e RIP: 0033:0x7fae145eeb4f Code: 00 48 89 (...) RSP: 002b:00007ffdf1cb09b0 EFLAGS: 00000246 ORIG_RAX: 0000000000000010 RAX: ffffffffffffffda RBX: 0000000000000004 RCX: 00007fae145eeb4f RDX: 00007ffdf1cb0ad0 RSI: 0000000040489426 RDI: 0000000000000004 RBP: 00000000000078fe R08: 00007fae144006c0 R09: 00007ffdf1cb0927 R10: 0000000000000008 R11: 0000000000000246 R12: 00007ffdf1cb1ce8 R13: 0000000000000003 R14: 000055c499fab2e0 R15: 0000000000000004 </TASK> Fix this by not storing the NUL string terminator since we don't actually need it for name cache entries, this way "name_len" corresponds to the actual size of the "name" array. This requires marking the "name" array field with __nonstring and using memcpy() instead of strcpy() as recommended by the guidelines at: KSPP/linux#90 Reported-by: David Arendt <admin@prnet.org> Link: https://lore.kernel.org/linux-btrfs/cee4591a-3088-49ba-99b8-d86b4242b8bd@prnet.org/ Fixes: c0247d2 ("btrfs: send: annotate struct name_cache_entry with __counted_by()") CC: stable@vger.kernel.org # 6.11 Tested-by: David Arendt <admin@prnet.org> Reviewed-by: Josef Bacik <josef@toxicpanda.com> Reviewed-by: Qu Wenruo <wqu@suse.com> Signed-off-by: Filipe Manana <fdmanana@suse.com> Reviewed-by: David Sterba <dsterba@suse.com> Signed-off-by: David Sterba <dsterba@suse.com> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Patch series "Improve the copy of task comm", v8. Using {memcpy,strncpy,strcpy,kstrdup} to copy the task comm relies on the length of task comm. Changes in the task comm could result in a destination string that is overflow. Therefore, we should explicitly ensure the destination string is always NUL-terminated, regardless of the task comm. This approach will facilitate future extensions to the task comm. As suggested by Linus [0], we can identify all relevant code with the following git grep command: git grep 'memcpy.*->comm\>' git grep 'kstrdup.*->comm\>' git grep 'strncpy.*->comm\>' git grep 'strcpy.*->comm\>' PATCH #2~#4: memcpy PATCH #5~torvalds#6: kstrdup PATCH torvalds#7: strcpy Please note that strncpy() is not included in this series as it is being tracked by another effort. [1] This patch (of 7): We want to eliminate the use of __get_task_comm() for the following reasons: - The task_lock() is unnecessary Quoted from Linus [0]: : Since user space can randomly change their names anyway, using locking : was always wrong for readers (for writers it probably does make sense : to have some lock - although practically speaking nobody cares there : either, but at least for a writer some kind of race could have : long-term mixed results Link: https://lkml.kernel.org/r/20241007144911.27693-1-laoar.shao@gmail.com Link: https://lkml.kernel.org/r/20241007144911.27693-2-laoar.shao@gmail.com Link: https://lore.kernel.org/all/CAHk-=wivfrF0_zvf+oj6==Sh=-npJooP8chLPEfaFV0oNYTTBA@mail.gmail.com [0] Link: https://lore.kernel.org/all/CAHk-=whWtUC-AjmGJveAETKOMeMFSTwKwu99v7+b6AyHMmaDFA@mail.gmail.com/ Link: https://lore.kernel.org/all/CAHk-=wjAmmHUg6vho1KjzQi2=psR30+CogFd4aXrThr2gsiS4g@mail.gmail.com/ [0] Link: KSPP#90 [1] Signed-off-by: Yafang Shao <laoar.shao@gmail.com> Suggested-by: Linus Torvalds <torvalds@linux-foundation.org> Cc: Alexander Viro <viro@zeniv.linux.org.uk> Cc: Christian Brauner <brauner@kernel.org> Cc: Jan Kara <jack@suse.cz> Cc: Eric Biederman <ebiederm@xmission.com> Cc: Kees Cook <keescook@chromium.org> Cc: Alexei Starovoitov <alexei.starovoitov@gmail.com> Cc: Matus Jokay <matus.jokay@stuba.sk> Cc: Alejandro Colomar <alx@kernel.org> Cc: "Serge E. Hallyn" <serge@hallyn.com> Cc: Catalin Marinas <catalin.marinas@arm.com> Cc: Justin Stitt <justinstitt@google.com> Cc: Steven Rostedt (Google) <rostedt@goodmis.org> Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> Cc: Andy Shevchenko <andy.shevchenko@gmail.com> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> Cc: David Airlie <airlied@gmail.com> Cc: Eric Paris <eparis@redhat.com> Cc: James Morris <jmorris@namei.org> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Cc: Matthew Wilcox <willy@infradead.org> Cc: Maxime Ripard <mripard@kernel.org> Cc: Ondrej Mosnacek <omosnace@redhat.com> Cc: Paul Moore <paul@paul-moore.com> Cc: Quentin Monnet <qmo@kernel.org> Cc: Simon Horman <horms@kernel.org> Cc: Stephen Smalley <stephen.smalley.work@gmail.com> Cc: Thomas Zimmermann <tzimmermann@suse.de> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
… entry commit 96c6ca7 upstream. Starting with commit c0247d2 ("btrfs: send: annotate struct name_cache_entry with __counted_by()") we annotated the variable length array "name" from the name_cache_entry structure with __counted_by() to improve overflow detection. However that alone was not correct, because the length of that array does not match the "name_len" field - it matches that plus 1 to include the NUL string terminator, so that makes a fortified kernel think there's an overflow and report a splat like this: strcpy: detected buffer overflow: 20 byte write of buffer size 19 WARNING: CPU: 3 PID: 3310 at __fortify_report+0x45/0x50 CPU: 3 UID: 0 PID: 3310 Comm: btrfs Not tainted 6.11.0-prnet #1 Hardware name: CompuLab Ltd. sbc-ihsw/Intense-PC2 (IPC2), BIOS IPC2_3.330.7 X64 03/15/2018 RIP: 0010:__fortify_report+0x45/0x50 Code: 48 8b 34 (...) RSP: 0018:ffff97ebc0d6f650 EFLAGS: 00010246 RAX: 7749924ef60fa600 RBX: ffff8bf5446a521a RCX: 0000000000000027 RDX: 00000000ffffdfff RSI: ffff97ebc0d6f548 RDI: ffff8bf84e7a1cc8 RBP: ffff8bf548574080 R08: ffffffffa8c40e10 R09: 0000000000005ffd R10: 0000000000000004 R11: ffffffffa8c70e10 R12: ffff8bf551eef400 R13: 0000000000000000 R14: 0000000000000013 R15: 00000000000003a8 FS: 00007fae144de8c0(0000) GS:ffff8bf84e780000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: 00007fae14691690 CR3: 00000001027a2003 CR4: 00000000001706f0 Call Trace: <TASK> ? __warn+0x12a/0x1d0 ? __fortify_report+0x45/0x50 ? report_bug+0x154/0x1c0 ? handle_bug+0x42/0x70 ? exc_invalid_op+0x1a/0x50 ? asm_exc_invalid_op+0x1a/0x20 ? __fortify_report+0x45/0x50 __fortify_panic+0x9/0x10 __get_cur_name_and_parent+0x3bc/0x3c0 get_cur_path+0x207/0x3b0 send_extent_data+0x709/0x10d0 ? find_parent_nodes+0x22df/0x25d0 ? mas_nomem+0x13/0x90 ? mtree_insert_range+0xa5/0x110 ? btrfs_lru_cache_store+0x5f/0x1e0 ? iterate_extent_inodes+0x52d/0x5a0 process_extent+0xa96/0x11a0 ? __pfx_lookup_backref_cache+0x10/0x10 ? __pfx_store_backref_cache+0x10/0x10 ? __pfx_iterate_backrefs+0x10/0x10 ? __pfx_check_extent_item+0x10/0x10 changed_cb+0x6fa/0x930 ? tree_advance+0x362/0x390 ? memcmp_extent_buffer+0xd7/0x160 send_subvol+0xf0a/0x1520 btrfs_ioctl_send+0x106b/0x11d0 ? __pfx___clone_root_cmp_sort+0x10/0x10 _btrfs_ioctl_send+0x1ac/0x240 btrfs_ioctl+0x75b/0x850 __se_sys_ioctl+0xca/0x150 do_syscall_64+0x85/0x160 ? __count_memcg_events+0x69/0x100 ? handle_mm_fault+0x1327/0x15c0 ? __se_sys_rt_sigprocmask+0xf1/0x180 ? syscall_exit_to_user_mode+0x75/0xa0 ? do_syscall_64+0x91/0x160 ? do_user_addr_fault+0x21d/0x630 entry_SYSCALL_64_after_hwframe+0x76/0x7e RIP: 0033:0x7fae145eeb4f Code: 00 48 89 (...) RSP: 002b:00007ffdf1cb09b0 EFLAGS: 00000246 ORIG_RAX: 0000000000000010 RAX: ffffffffffffffda RBX: 0000000000000004 RCX: 00007fae145eeb4f RDX: 00007ffdf1cb0ad0 RSI: 0000000040489426 RDI: 0000000000000004 RBP: 00000000000078fe R08: 00007fae144006c0 R09: 00007ffdf1cb0927 R10: 0000000000000008 R11: 0000000000000246 R12: 00007ffdf1cb1ce8 R13: 0000000000000003 R14: 000055c499fab2e0 R15: 0000000000000004 </TASK> Fix this by not storing the NUL string terminator since we don't actually need it for name cache entries, this way "name_len" corresponds to the actual size of the "name" array. This requires marking the "name" array field with __nonstring and using memcpy() instead of strcpy() as recommended by the guidelines at: KSPP#90 Reported-by: David Arendt <admin@prnet.org> Link: https://lore.kernel.org/linux-btrfs/cee4591a-3088-49ba-99b8-d86b4242b8bd@prnet.org/ Fixes: c0247d2 ("btrfs: send: annotate struct name_cache_entry with __counted_by()") CC: stable@vger.kernel.org # 6.11 Tested-by: David Arendt <admin@prnet.org> Reviewed-by: Josef Bacik <josef@toxicpanda.com> Reviewed-by: Qu Wenruo <wqu@suse.com> Signed-off-by: Filipe Manana <fdmanana@suse.com> Reviewed-by: David Sterba <dsterba@suse.com> Signed-off-by: David Sterba <dsterba@suse.com> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
strncpy() is deprecated for use on NUL-terminated destination strings [1] and as such we should prefer more robust and less ambiguous string interfaces. Both of these fields want to be NUL-terminated as per their use in printk: F_printk("%u:%s:%s (%u)%s", __entry->line, __entry->func, __entry->file, __entry->correct, __entry->constant ? " CONSTANT" : "") Use strscpy() as it NUL-terminates the destination buffer, so it doesn't have to be done manually. Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1] Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html Link: KSPP/linux#90 Cc: linux-hardening@vger.kernel.org Cc: Masami Hiramatsu <mhiramat@kernel.org> Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> Cc: Kees Cook <kees@kernel.org> Link: https://lore.kernel.org/20240826-strncpy-kernel-trace-trace_branch-c-v1-1-b2c14f2e9e84@google.com Signed-off-by: Justin Stitt <justinstitt@google.com> Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
strncpy() is deprecated for use on NUL-terminated destination strings [1] and as such we should prefer more robust and less ambiguous string interfaces. We expect the @pattern and @num_buf strings to be NUL-terminated, as evidenced by their manual NUL-byte assignments immediately following each copy. Switch to using strscpy which guarantees NUL-termination for the destination buffer -- eschewing manual NUL-byte assignments. strscpy does not NUL-pad so to keep this behavior zero-allocate @num_buf. @pred is already zero-allocated before the copies. pred = kzalloc(sizeof(*pred), GFP_KERNEL); This should result in no behavioral changes whilst helping towards the goal of [2] -- with the ultimate goal of removing strncpy in favor of less ambiguous and more robust alternatives. Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1] Link: KSPP/linux#90 [2] Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html Cc: Kees Cook <keescook@chromium.org> Cc: Masami Hiramatsu <mhiramat@kernel.org> Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> Cc: Kees Cook <kees@kernel.org> Cc: linux-hardening@vger.kernel.org Link: https://lore.kernel.org/20240930-strncpy-kernel-trace-trace_events_filter-c-v1-1-feed30820b83@google.com Signed-off-by: Justin Stitt <justinstitt@google.com> Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
Patch series "Improve the copy of task comm", v8. Using {memcpy,strncpy,strcpy,kstrdup} to copy the task comm relies on the length of task comm. Changes in the task comm could result in a destination string that is overflow. Therefore, we should explicitly ensure the destination string is always NUL-terminated, regardless of the task comm. This approach will facilitate future extensions to the task comm. As suggested by Linus [0], we can identify all relevant code with the following git grep command: git grep 'memcpy.*->comm\>' git grep 'kstrdup.*->comm\>' git grep 'strncpy.*->comm\>' git grep 'strcpy.*->comm\>' PATCH #2~#4: memcpy PATCH #5~torvalds#6: kstrdup PATCH torvalds#7: strcpy Please note that strncpy() is not included in this series as it is being tracked by another effort. [1] This patch (of 7): We want to eliminate the use of __get_task_comm() for the following reasons: - The task_lock() is unnecessary Quoted from Linus [0]: : Since user space can randomly change their names anyway, using locking : was always wrong for readers (for writers it probably does make sense : to have some lock - although practically speaking nobody cares there : either, but at least for a writer some kind of race could have : long-term mixed results Link: https://lkml.kernel.org/r/20241007144911.27693-1-laoar.shao@gmail.com Link: https://lkml.kernel.org/r/20241007144911.27693-2-laoar.shao@gmail.com Link: https://lore.kernel.org/all/CAHk-=wivfrF0_zvf+oj6==Sh=-npJooP8chLPEfaFV0oNYTTBA@mail.gmail.com [0] Link: https://lore.kernel.org/all/CAHk-=whWtUC-AjmGJveAETKOMeMFSTwKwu99v7+b6AyHMmaDFA@mail.gmail.com/ Link: https://lore.kernel.org/all/CAHk-=wjAmmHUg6vho1KjzQi2=psR30+CogFd4aXrThr2gsiS4g@mail.gmail.com/ [0] Link: KSPP#90 [1] Signed-off-by: Yafang Shao <laoar.shao@gmail.com> Suggested-by: Linus Torvalds <torvalds@linux-foundation.org> Cc: Alexander Viro <viro@zeniv.linux.org.uk> Cc: Christian Brauner <brauner@kernel.org> Cc: Jan Kara <jack@suse.cz> Cc: Eric Biederman <ebiederm@xmission.com> Cc: Kees Cook <keescook@chromium.org> Cc: Alexei Starovoitov <alexei.starovoitov@gmail.com> Cc: Matus Jokay <matus.jokay@stuba.sk> Cc: Alejandro Colomar <alx@kernel.org> Cc: "Serge E. Hallyn" <serge@hallyn.com> Cc: Catalin Marinas <catalin.marinas@arm.com> Cc: Justin Stitt <justinstitt@google.com> Cc: Steven Rostedt (Google) <rostedt@goodmis.org> Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> Cc: Andy Shevchenko <andy.shevchenko@gmail.com> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> Cc: David Airlie <airlied@gmail.com> Cc: Eric Paris <eparis@redhat.com> Cc: James Morris <jmorris@namei.org> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Cc: Matthew Wilcox <willy@infradead.org> Cc: Maxime Ripard <mripard@kernel.org> Cc: Ondrej Mosnacek <omosnace@redhat.com> Cc: Paul Moore <paul@paul-moore.com> Cc: Quentin Monnet <qmo@kernel.org> Cc: Simon Horman <horms@kernel.org> Cc: Stephen Smalley <stephen.smalley.work@gmail.com> Cc: Thomas Zimmermann <tzimmermann@suse.de> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
… entry commit 96c6ca7 upstream. Starting with commit c0247d2 ("btrfs: send: annotate struct name_cache_entry with __counted_by()") we annotated the variable length array "name" from the name_cache_entry structure with __counted_by() to improve overflow detection. However that alone was not correct, because the length of that array does not match the "name_len" field - it matches that plus 1 to include the NUL string terminator, so that makes a fortified kernel think there's an overflow and report a splat like this: strcpy: detected buffer overflow: 20 byte write of buffer size 19 WARNING: CPU: 3 PID: 3310 at __fortify_report+0x45/0x50 CPU: 3 UID: 0 PID: 3310 Comm: btrfs Not tainted 6.11.0-prnet #1 Hardware name: CompuLab Ltd. sbc-ihsw/Intense-PC2 (IPC2), BIOS IPC2_3.330.7 X64 03/15/2018 RIP: 0010:__fortify_report+0x45/0x50 Code: 48 8b 34 (...) RSP: 0018:ffff97ebc0d6f650 EFLAGS: 00010246 RAX: 7749924ef60fa600 RBX: ffff8bf5446a521a RCX: 0000000000000027 RDX: 00000000ffffdfff RSI: ffff97ebc0d6f548 RDI: ffff8bf84e7a1cc8 RBP: ffff8bf548574080 R08: ffffffffa8c40e10 R09: 0000000000005ffd R10: 0000000000000004 R11: ffffffffa8c70e10 R12: ffff8bf551eef400 R13: 0000000000000000 R14: 0000000000000013 R15: 00000000000003a8 FS: 00007fae144de8c0(0000) GS:ffff8bf84e780000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: 00007fae14691690 CR3: 00000001027a2003 CR4: 00000000001706f0 Call Trace: <TASK> ? __warn+0x12a/0x1d0 ? __fortify_report+0x45/0x50 ? report_bug+0x154/0x1c0 ? handle_bug+0x42/0x70 ? exc_invalid_op+0x1a/0x50 ? asm_exc_invalid_op+0x1a/0x20 ? __fortify_report+0x45/0x50 __fortify_panic+0x9/0x10 __get_cur_name_and_parent+0x3bc/0x3c0 get_cur_path+0x207/0x3b0 send_extent_data+0x709/0x10d0 ? find_parent_nodes+0x22df/0x25d0 ? mas_nomem+0x13/0x90 ? mtree_insert_range+0xa5/0x110 ? btrfs_lru_cache_store+0x5f/0x1e0 ? iterate_extent_inodes+0x52d/0x5a0 process_extent+0xa96/0x11a0 ? __pfx_lookup_backref_cache+0x10/0x10 ? __pfx_store_backref_cache+0x10/0x10 ? __pfx_iterate_backrefs+0x10/0x10 ? __pfx_check_extent_item+0x10/0x10 changed_cb+0x6fa/0x930 ? tree_advance+0x362/0x390 ? memcmp_extent_buffer+0xd7/0x160 send_subvol+0xf0a/0x1520 btrfs_ioctl_send+0x106b/0x11d0 ? __pfx___clone_root_cmp_sort+0x10/0x10 _btrfs_ioctl_send+0x1ac/0x240 btrfs_ioctl+0x75b/0x850 __se_sys_ioctl+0xca/0x150 do_syscall_64+0x85/0x160 ? __count_memcg_events+0x69/0x100 ? handle_mm_fault+0x1327/0x15c0 ? __se_sys_rt_sigprocmask+0xf1/0x180 ? syscall_exit_to_user_mode+0x75/0xa0 ? do_syscall_64+0x91/0x160 ? do_user_addr_fault+0x21d/0x630 entry_SYSCALL_64_after_hwframe+0x76/0x7e RIP: 0033:0x7fae145eeb4f Code: 00 48 89 (...) RSP: 002b:00007ffdf1cb09b0 EFLAGS: 00000246 ORIG_RAX: 0000000000000010 RAX: ffffffffffffffda RBX: 0000000000000004 RCX: 00007fae145eeb4f RDX: 00007ffdf1cb0ad0 RSI: 0000000040489426 RDI: 0000000000000004 RBP: 00000000000078fe R08: 00007fae144006c0 R09: 00007ffdf1cb0927 R10: 0000000000000008 R11: 0000000000000246 R12: 00007ffdf1cb1ce8 R13: 0000000000000003 R14: 000055c499fab2e0 R15: 0000000000000004 </TASK> Fix this by not storing the NUL string terminator since we don't actually need it for name cache entries, this way "name_len" corresponds to the actual size of the "name" array. This requires marking the "name" array field with __nonstring and using memcpy() instead of strcpy() as recommended by the guidelines at: KSPP/linux#90 Reported-by: David Arendt <admin@prnet.org> Link: https://lore.kernel.org/linux-btrfs/cee4591a-3088-49ba-99b8-d86b4242b8bd@prnet.org/ Fixes: c0247d2 ("btrfs: send: annotate struct name_cache_entry with __counted_by()") CC: stable@vger.kernel.org # 6.11 Tested-by: David Arendt <admin@prnet.org> Reviewed-by: Josef Bacik <josef@toxicpanda.com> Reviewed-by: Qu Wenruo <wqu@suse.com> Signed-off-by: Filipe Manana <fdmanana@suse.com> Reviewed-by: David Sterba <dsterba@suse.com> Signed-off-by: David Sterba <dsterba@suse.com> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Patch series "Improve the copy of task comm", v8. Using {memcpy,strncpy,strcpy,kstrdup} to copy the task comm relies on the length of task comm. Changes in the task comm could result in a destination string that is overflow. Therefore, we should explicitly ensure the destination string is always NUL-terminated, regardless of the task comm. This approach will facilitate future extensions to the task comm. As suggested by Linus [0], we can identify all relevant code with the following git grep command: git grep 'memcpy.*->comm\>' git grep 'kstrdup.*->comm\>' git grep 'strncpy.*->comm\>' git grep 'strcpy.*->comm\>' PATCH #2~#4: memcpy PATCH #5~torvalds#6: kstrdup PATCH torvalds#7: strcpy Please note that strncpy() is not included in this series as it is being tracked by another effort. [1] This patch (of 7): We want to eliminate the use of __get_task_comm() for the following reasons: - The task_lock() is unnecessary Quoted from Linus [0]: : Since user space can randomly change their names anyway, using locking : was always wrong for readers (for writers it probably does make sense : to have some lock - although practically speaking nobody cares there : either, but at least for a writer some kind of race could have : long-term mixed results Link: https://lkml.kernel.org/r/20241007144911.27693-1-laoar.shao@gmail.com Link: https://lkml.kernel.org/r/20241007144911.27693-2-laoar.shao@gmail.com Link: https://lore.kernel.org/all/CAHk-=wivfrF0_zvf+oj6==Sh=-npJooP8chLPEfaFV0oNYTTBA@mail.gmail.com [0] Link: https://lore.kernel.org/all/CAHk-=whWtUC-AjmGJveAETKOMeMFSTwKwu99v7+b6AyHMmaDFA@mail.gmail.com/ Link: https://lore.kernel.org/all/CAHk-=wjAmmHUg6vho1KjzQi2=psR30+CogFd4aXrThr2gsiS4g@mail.gmail.com/ [0] Link: KSPP#90 [1] Signed-off-by: Yafang Shao <laoar.shao@gmail.com> Suggested-by: Linus Torvalds <torvalds@linux-foundation.org> Cc: Alexander Viro <viro@zeniv.linux.org.uk> Cc: Christian Brauner <brauner@kernel.org> Cc: Jan Kara <jack@suse.cz> Cc: Eric Biederman <ebiederm@xmission.com> Cc: Kees Cook <keescook@chromium.org> Cc: Alexei Starovoitov <alexei.starovoitov@gmail.com> Cc: Matus Jokay <matus.jokay@stuba.sk> Cc: Alejandro Colomar <alx@kernel.org> Cc: "Serge E. Hallyn" <serge@hallyn.com> Cc: Catalin Marinas <catalin.marinas@arm.com> Cc: Justin Stitt <justinstitt@google.com> Cc: Steven Rostedt (Google) <rostedt@goodmis.org> Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> Cc: Andy Shevchenko <andy.shevchenko@gmail.com> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> Cc: David Airlie <airlied@gmail.com> Cc: Eric Paris <eparis@redhat.com> Cc: James Morris <jmorris@namei.org> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Cc: Matthew Wilcox <willy@infradead.org> Cc: Maxime Ripard <mripard@kernel.org> Cc: Ondrej Mosnacek <omosnace@redhat.com> Cc: Paul Moore <paul@paul-moore.com> Cc: Quentin Monnet <qmo@kernel.org> Cc: Simon Horman <horms@kernel.org> Cc: Stephen Smalley <stephen.smalley.work@gmail.com> Cc: Thomas Zimmermann <tzimmermann@suse.de> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
… entry commit 96c6ca7 upstream. Starting with commit c0247d2 ("btrfs: send: annotate struct name_cache_entry with __counted_by()") we annotated the variable length array "name" from the name_cache_entry structure with __counted_by() to improve overflow detection. However that alone was not correct, because the length of that array does not match the "name_len" field - it matches that plus 1 to include the NUL string terminator, so that makes a fortified kernel think there's an overflow and report a splat like this: strcpy: detected buffer overflow: 20 byte write of buffer size 19 WARNING: CPU: 3 PID: 3310 at __fortify_report+0x45/0x50 CPU: 3 UID: 0 PID: 3310 Comm: btrfs Not tainted 6.11.0-prnet #1 Hardware name: CompuLab Ltd. sbc-ihsw/Intense-PC2 (IPC2), BIOS IPC2_3.330.7 X64 03/15/2018 RIP: 0010:__fortify_report+0x45/0x50 Code: 48 8b 34 (...) RSP: 0018:ffff97ebc0d6f650 EFLAGS: 00010246 RAX: 7749924ef60fa600 RBX: ffff8bf5446a521a RCX: 0000000000000027 RDX: 00000000ffffdfff RSI: ffff97ebc0d6f548 RDI: ffff8bf84e7a1cc8 RBP: ffff8bf548574080 R08: ffffffffa8c40e10 R09: 0000000000005ffd R10: 0000000000000004 R11: ffffffffa8c70e10 R12: ffff8bf551eef400 R13: 0000000000000000 R14: 0000000000000013 R15: 00000000000003a8 FS: 00007fae144de8c0(0000) GS:ffff8bf84e780000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: 00007fae14691690 CR3: 00000001027a2003 CR4: 00000000001706f0 Call Trace: <TASK> ? __warn+0x12a/0x1d0 ? __fortify_report+0x45/0x50 ? report_bug+0x154/0x1c0 ? handle_bug+0x42/0x70 ? exc_invalid_op+0x1a/0x50 ? asm_exc_invalid_op+0x1a/0x20 ? __fortify_report+0x45/0x50 __fortify_panic+0x9/0x10 __get_cur_name_and_parent+0x3bc/0x3c0 get_cur_path+0x207/0x3b0 send_extent_data+0x709/0x10d0 ? find_parent_nodes+0x22df/0x25d0 ? mas_nomem+0x13/0x90 ? mtree_insert_range+0xa5/0x110 ? btrfs_lru_cache_store+0x5f/0x1e0 ? iterate_extent_inodes+0x52d/0x5a0 process_extent+0xa96/0x11a0 ? __pfx_lookup_backref_cache+0x10/0x10 ? __pfx_store_backref_cache+0x10/0x10 ? __pfx_iterate_backrefs+0x10/0x10 ? __pfx_check_extent_item+0x10/0x10 changed_cb+0x6fa/0x930 ? tree_advance+0x362/0x390 ? memcmp_extent_buffer+0xd7/0x160 send_subvol+0xf0a/0x1520 btrfs_ioctl_send+0x106b/0x11d0 ? __pfx___clone_root_cmp_sort+0x10/0x10 _btrfs_ioctl_send+0x1ac/0x240 btrfs_ioctl+0x75b/0x850 __se_sys_ioctl+0xca/0x150 do_syscall_64+0x85/0x160 ? __count_memcg_events+0x69/0x100 ? handle_mm_fault+0x1327/0x15c0 ? __se_sys_rt_sigprocmask+0xf1/0x180 ? syscall_exit_to_user_mode+0x75/0xa0 ? do_syscall_64+0x91/0x160 ? do_user_addr_fault+0x21d/0x630 entry_SYSCALL_64_after_hwframe+0x76/0x7e RIP: 0033:0x7fae145eeb4f Code: 00 48 89 (...) RSP: 002b:00007ffdf1cb09b0 EFLAGS: 00000246 ORIG_RAX: 0000000000000010 RAX: ffffffffffffffda RBX: 0000000000000004 RCX: 00007fae145eeb4f RDX: 00007ffdf1cb0ad0 RSI: 0000000040489426 RDI: 0000000000000004 RBP: 00000000000078fe R08: 00007fae144006c0 R09: 00007ffdf1cb0927 R10: 0000000000000008 R11: 0000000000000246 R12: 00007ffdf1cb1ce8 R13: 0000000000000003 R14: 000055c499fab2e0 R15: 0000000000000004 </TASK> Fix this by not storing the NUL string terminator since we don't actually need it for name cache entries, this way "name_len" corresponds to the actual size of the "name" array. This requires marking the "name" array field with __nonstring and using memcpy() instead of strcpy() as recommended by the guidelines at: KSPP/linux#90 Reported-by: David Arendt <admin@prnet.org> Link: https://lore.kernel.org/linux-btrfs/cee4591a-3088-49ba-99b8-d86b4242b8bd@prnet.org/ Fixes: c0247d2 ("btrfs: send: annotate struct name_cache_entry with __counted_by()") CC: stable@vger.kernel.org # 6.11 Tested-by: David Arendt <admin@prnet.org> Reviewed-by: Josef Bacik <josef@toxicpanda.com> Reviewed-by: Qu Wenruo <wqu@suse.com> Signed-off-by: Filipe Manana <fdmanana@suse.com> Reviewed-by: David Sterba <dsterba@suse.com> Signed-off-by: David Sterba <dsterba@suse.com> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
… entry commit 96c6ca7 upstream. Starting with commit c0247d2 ("btrfs: send: annotate struct name_cache_entry with __counted_by()") we annotated the variable length array "name" from the name_cache_entry structure with __counted_by() to improve overflow detection. However that alone was not correct, because the length of that array does not match the "name_len" field - it matches that plus 1 to include the NUL string terminator, so that makes a fortified kernel think there's an overflow and report a splat like this: strcpy: detected buffer overflow: 20 byte write of buffer size 19 WARNING: CPU: 3 PID: 3310 at __fortify_report+0x45/0x50 CPU: 3 UID: 0 PID: 3310 Comm: btrfs Not tainted 6.11.0-prnet #1 Hardware name: CompuLab Ltd. sbc-ihsw/Intense-PC2 (IPC2), BIOS IPC2_3.330.7 X64 03/15/2018 RIP: 0010:__fortify_report+0x45/0x50 Code: 48 8b 34 (...) RSP: 0018:ffff97ebc0d6f650 EFLAGS: 00010246 RAX: 7749924ef60fa600 RBX: ffff8bf5446a521a RCX: 0000000000000027 RDX: 00000000ffffdfff RSI: ffff97ebc0d6f548 RDI: ffff8bf84e7a1cc8 RBP: ffff8bf548574080 R08: ffffffffa8c40e10 R09: 0000000000005ffd R10: 0000000000000004 R11: ffffffffa8c70e10 R12: ffff8bf551eef400 R13: 0000000000000000 R14: 0000000000000013 R15: 00000000000003a8 FS: 00007fae144de8c0(0000) GS:ffff8bf84e780000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: 00007fae14691690 CR3: 00000001027a2003 CR4: 00000000001706f0 Call Trace: <TASK> ? __warn+0x12a/0x1d0 ? __fortify_report+0x45/0x50 ? report_bug+0x154/0x1c0 ? handle_bug+0x42/0x70 ? exc_invalid_op+0x1a/0x50 ? asm_exc_invalid_op+0x1a/0x20 ? __fortify_report+0x45/0x50 __fortify_panic+0x9/0x10 __get_cur_name_and_parent+0x3bc/0x3c0 get_cur_path+0x207/0x3b0 send_extent_data+0x709/0x10d0 ? find_parent_nodes+0x22df/0x25d0 ? mas_nomem+0x13/0x90 ? mtree_insert_range+0xa5/0x110 ? btrfs_lru_cache_store+0x5f/0x1e0 ? iterate_extent_inodes+0x52d/0x5a0 process_extent+0xa96/0x11a0 ? __pfx_lookup_backref_cache+0x10/0x10 ? __pfx_store_backref_cache+0x10/0x10 ? __pfx_iterate_backrefs+0x10/0x10 ? __pfx_check_extent_item+0x10/0x10 changed_cb+0x6fa/0x930 ? tree_advance+0x362/0x390 ? memcmp_extent_buffer+0xd7/0x160 send_subvol+0xf0a/0x1520 btrfs_ioctl_send+0x106b/0x11d0 ? __pfx___clone_root_cmp_sort+0x10/0x10 _btrfs_ioctl_send+0x1ac/0x240 btrfs_ioctl+0x75b/0x850 __se_sys_ioctl+0xca/0x150 do_syscall_64+0x85/0x160 ? __count_memcg_events+0x69/0x100 ? handle_mm_fault+0x1327/0x15c0 ? __se_sys_rt_sigprocmask+0xf1/0x180 ? syscall_exit_to_user_mode+0x75/0xa0 ? do_syscall_64+0x91/0x160 ? do_user_addr_fault+0x21d/0x630 entry_SYSCALL_64_after_hwframe+0x76/0x7e RIP: 0033:0x7fae145eeb4f Code: 00 48 89 (...) RSP: 002b:00007ffdf1cb09b0 EFLAGS: 00000246 ORIG_RAX: 0000000000000010 RAX: ffffffffffffffda RBX: 0000000000000004 RCX: 00007fae145eeb4f RDX: 00007ffdf1cb0ad0 RSI: 0000000040489426 RDI: 0000000000000004 RBP: 00000000000078fe R08: 00007fae144006c0 R09: 00007ffdf1cb0927 R10: 0000000000000008 R11: 0000000000000246 R12: 00007ffdf1cb1ce8 R13: 0000000000000003 R14: 000055c499fab2e0 R15: 0000000000000004 </TASK> Fix this by not storing the NUL string terminator since we don't actually need it for name cache entries, this way "name_len" corresponds to the actual size of the "name" array. This requires marking the "name" array field with __nonstring and using memcpy() instead of strcpy() as recommended by the guidelines at: KSPP/linux#90 Reported-by: David Arendt <admin@prnet.org> Link: https://lore.kernel.org/linux-btrfs/cee4591a-3088-49ba-99b8-d86b4242b8bd@prnet.org/ Fixes: c0247d2 ("btrfs: send: annotate struct name_cache_entry with __counted_by()") CC: stable@vger.kernel.org # 6.11 Tested-by: David Arendt <admin@prnet.org> Reviewed-by: Josef Bacik <josef@toxicpanda.com> Reviewed-by: Qu Wenruo <wqu@suse.com> Signed-off-by: Filipe Manana <fdmanana@suse.com> Reviewed-by: David Sterba <dsterba@suse.com> Signed-off-by: David Sterba <dsterba@suse.com> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Patch series "Improve the copy of task comm", v8. Using {memcpy,strncpy,strcpy,kstrdup} to copy the task comm relies on the length of task comm. Changes in the task comm could result in a destination string that is overflow. Therefore, we should explicitly ensure the destination string is always NUL-terminated, regardless of the task comm. This approach will facilitate future extensions to the task comm. As suggested by Linus [0], we can identify all relevant code with the following git grep command: git grep 'memcpy.*->comm\>' git grep 'kstrdup.*->comm\>' git grep 'strncpy.*->comm\>' git grep 'strcpy.*->comm\>' PATCH #2~#4: memcpy PATCH #5~torvalds#6: kstrdup PATCH torvalds#7: strcpy Please note that strncpy() is not included in this series as it is being tracked by another effort. [1] This patch (of 7): We want to eliminate the use of __get_task_comm() for the following reasons: - The task_lock() is unnecessary Quoted from Linus [0]: : Since user space can randomly change their names anyway, using locking : was always wrong for readers (for writers it probably does make sense : to have some lock - although practically speaking nobody cares there : either, but at least for a writer some kind of race could have : long-term mixed results Link: https://lkml.kernel.org/r/20241007144911.27693-1-laoar.shao@gmail.com Link: https://lkml.kernel.org/r/20241007144911.27693-2-laoar.shao@gmail.com Link: https://lore.kernel.org/all/CAHk-=wivfrF0_zvf+oj6==Sh=-npJooP8chLPEfaFV0oNYTTBA@mail.gmail.com [0] Link: https://lore.kernel.org/all/CAHk-=whWtUC-AjmGJveAETKOMeMFSTwKwu99v7+b6AyHMmaDFA@mail.gmail.com/ Link: https://lore.kernel.org/all/CAHk-=wjAmmHUg6vho1KjzQi2=psR30+CogFd4aXrThr2gsiS4g@mail.gmail.com/ [0] Link: KSPP#90 [1] Signed-off-by: Yafang Shao <laoar.shao@gmail.com> Suggested-by: Linus Torvalds <torvalds@linux-foundation.org> Cc: Alexander Viro <viro@zeniv.linux.org.uk> Cc: Christian Brauner <brauner@kernel.org> Cc: Jan Kara <jack@suse.cz> Cc: Eric Biederman <ebiederm@xmission.com> Cc: Kees Cook <keescook@chromium.org> Cc: Alexei Starovoitov <alexei.starovoitov@gmail.com> Cc: Matus Jokay <matus.jokay@stuba.sk> Cc: Alejandro Colomar <alx@kernel.org> Cc: "Serge E. Hallyn" <serge@hallyn.com> Cc: Catalin Marinas <catalin.marinas@arm.com> Cc: Justin Stitt <justinstitt@google.com> Cc: Steven Rostedt (Google) <rostedt@goodmis.org> Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> Cc: Andy Shevchenko <andy.shevchenko@gmail.com> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> Cc: David Airlie <airlied@gmail.com> Cc: Eric Paris <eparis@redhat.com> Cc: James Morris <jmorris@namei.org> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Cc: Matthew Wilcox <willy@infradead.org> Cc: Maxime Ripard <mripard@kernel.org> Cc: Ondrej Mosnacek <omosnace@redhat.com> Cc: Paul Moore <paul@paul-moore.com> Cc: Quentin Monnet <qmo@kernel.org> Cc: Simon Horman <horms@kernel.org> Cc: Stephen Smalley <stephen.smalley.work@gmail.com> Cc: Thomas Zimmermann <tzimmermann@suse.de> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
Patch series "Improve the copy of task comm", v8. Using {memcpy,strncpy,strcpy,kstrdup} to copy the task comm relies on the length of task comm. Changes in the task comm could result in a destination string that is overflow. Therefore, we should explicitly ensure the destination string is always NUL-terminated, regardless of the task comm. This approach will facilitate future extensions to the task comm. As suggested by Linus [0], we can identify all relevant code with the following git grep command: git grep 'memcpy.*->comm\>' git grep 'kstrdup.*->comm\>' git grep 'strncpy.*->comm\>' git grep 'strcpy.*->comm\>' PATCH #2~#4: memcpy PATCH #5~torvalds#6: kstrdup PATCH torvalds#7: strcpy Please note that strncpy() is not included in this series as it is being tracked by another effort. [1] This patch (of 7): We want to eliminate the use of __get_task_comm() for the following reasons: - The task_lock() is unnecessary Quoted from Linus [0]: : Since user space can randomly change their names anyway, using locking : was always wrong for readers (for writers it probably does make sense : to have some lock - although practically speaking nobody cares there : either, but at least for a writer some kind of race could have : long-term mixed results Link: https://lkml.kernel.org/r/20241007144911.27693-1-laoar.shao@gmail.com Link: https://lkml.kernel.org/r/20241007144911.27693-2-laoar.shao@gmail.com Link: https://lore.kernel.org/all/CAHk-=wivfrF0_zvf+oj6==Sh=-npJooP8chLPEfaFV0oNYTTBA@mail.gmail.com [0] Link: https://lore.kernel.org/all/CAHk-=whWtUC-AjmGJveAETKOMeMFSTwKwu99v7+b6AyHMmaDFA@mail.gmail.com/ Link: https://lore.kernel.org/all/CAHk-=wjAmmHUg6vho1KjzQi2=psR30+CogFd4aXrThr2gsiS4g@mail.gmail.com/ [0] Link: KSPP#90 [1] Signed-off-by: Yafang Shao <laoar.shao@gmail.com> Suggested-by: Linus Torvalds <torvalds@linux-foundation.org> Cc: Alexander Viro <viro@zeniv.linux.org.uk> Cc: Christian Brauner <brauner@kernel.org> Cc: Jan Kara <jack@suse.cz> Cc: Eric Biederman <ebiederm@xmission.com> Cc: Kees Cook <keescook@chromium.org> Cc: Alexei Starovoitov <alexei.starovoitov@gmail.com> Cc: Matus Jokay <matus.jokay@stuba.sk> Cc: Alejandro Colomar <alx@kernel.org> Cc: "Serge E. Hallyn" <serge@hallyn.com> Cc: Catalin Marinas <catalin.marinas@arm.com> Cc: Justin Stitt <justinstitt@google.com> Cc: Steven Rostedt (Google) <rostedt@goodmis.org> Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> Cc: Andy Shevchenko <andy.shevchenko@gmail.com> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> Cc: David Airlie <airlied@gmail.com> Cc: Eric Paris <eparis@redhat.com> Cc: James Morris <jmorris@namei.org> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Cc: Matthew Wilcox <willy@infradead.org> Cc: Maxime Ripard <mripard@kernel.org> Cc: Ondrej Mosnacek <omosnace@redhat.com> Cc: Paul Moore <paul@paul-moore.com> Cc: Quentin Monnet <qmo@kernel.org> Cc: Simon Horman <horms@kernel.org> Cc: Stephen Smalley <stephen.smalley.work@gmail.com> Cc: Thomas Zimmermann <tzimmermann@suse.de> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
Patch series "Improve the copy of task comm", v8. Using {memcpy,strncpy,strcpy,kstrdup} to copy the task comm relies on the length of task comm. Changes in the task comm could result in a destination string that is overflow. Therefore, we should explicitly ensure the destination string is always NUL-terminated, regardless of the task comm. This approach will facilitate future extensions to the task comm. As suggested by Linus [0], we can identify all relevant code with the following git grep command: git grep 'memcpy.*->comm\>' git grep 'kstrdup.*->comm\>' git grep 'strncpy.*->comm\>' git grep 'strcpy.*->comm\>' PATCH #2~#4: memcpy PATCH #5~torvalds#6: kstrdup PATCH torvalds#7: strcpy Please note that strncpy() is not included in this series as it is being tracked by another effort. [1] This patch (of 7): We want to eliminate the use of __get_task_comm() for the following reasons: - The task_lock() is unnecessary Quoted from Linus [0]: : Since user space can randomly change their names anyway, using locking : was always wrong for readers (for writers it probably does make sense : to have some lock - although practically speaking nobody cares there : either, but at least for a writer some kind of race could have : long-term mixed results Link: https://lkml.kernel.org/r/20241007144911.27693-1-laoar.shao@gmail.com Link: https://lkml.kernel.org/r/20241007144911.27693-2-laoar.shao@gmail.com Link: https://lore.kernel.org/all/CAHk-=wivfrF0_zvf+oj6==Sh=-npJooP8chLPEfaFV0oNYTTBA@mail.gmail.com [0] Link: https://lore.kernel.org/all/CAHk-=whWtUC-AjmGJveAETKOMeMFSTwKwu99v7+b6AyHMmaDFA@mail.gmail.com/ Link: https://lore.kernel.org/all/CAHk-=wjAmmHUg6vho1KjzQi2=psR30+CogFd4aXrThr2gsiS4g@mail.gmail.com/ [0] Link: KSPP#90 [1] Signed-off-by: Yafang Shao <laoar.shao@gmail.com> Suggested-by: Linus Torvalds <torvalds@linux-foundation.org> Cc: Alexander Viro <viro@zeniv.linux.org.uk> Cc: Christian Brauner <brauner@kernel.org> Cc: Jan Kara <jack@suse.cz> Cc: Eric Biederman <ebiederm@xmission.com> Cc: Kees Cook <keescook@chromium.org> Cc: Alexei Starovoitov <alexei.starovoitov@gmail.com> Cc: Matus Jokay <matus.jokay@stuba.sk> Cc: Alejandro Colomar <alx@kernel.org> Cc: "Serge E. Hallyn" <serge@hallyn.com> Cc: Catalin Marinas <catalin.marinas@arm.com> Cc: Justin Stitt <justinstitt@google.com> Cc: Steven Rostedt (Google) <rostedt@goodmis.org> Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> Cc: Andy Shevchenko <andy.shevchenko@gmail.com> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> Cc: David Airlie <airlied@gmail.com> Cc: Eric Paris <eparis@redhat.com> Cc: James Morris <jmorris@namei.org> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Cc: Matthew Wilcox <willy@infradead.org> Cc: Maxime Ripard <mripard@kernel.org> Cc: Ondrej Mosnacek <omosnace@redhat.com> Cc: Paul Moore <paul@paul-moore.com> Cc: Quentin Monnet <qmo@kernel.org> Cc: Simon Horman <horms@kernel.org> Cc: Stephen Smalley <stephen.smalley.work@gmail.com> Cc: Thomas Zimmermann <tzimmermann@suse.de> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
strncpy() is deprecated for use on NUL-terminated destination strings [1] and as such we should prefer more robust and less ambiguous string interfaces. In this particular instance, the usage of strncpy() is fine and works as expected. However, towards the goal of [2], we should consider replacing it with an alternative as many instances of strncpy() are bug-prone. Its removal from the kernel promotes better long term health for the codebase. The current usage of strncpy() likely just wants the NUL-padding behavior offered by strncpy() and doesn't care about the NUL-termination. Since the compiler doesn't know the size of @DesT, we can't use strtomem_pad(). Instead, use strscpy_pad() which behaves functionally the same as strncpy() in this context -- as we expect br_dev->name to be NUL-terminated itself. Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1] Link: KSPP#90 [2] Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html Cc: Kees Cook <keescook@chromium.org> Cc: linux-hardening@vger.kernel.org Signed-off-by: Justin Stitt <justinstitt@google.com> Reviewed-by: Simon Horman <horms@kernel.org> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org> Signed-off-by: NipaLocal <nipa@local>
strncpy() is deprecated for use on NUL-terminated destination strings [1] and as such we should prefer more robust and less ambiguous string interfaces. String copy operations involving manual pointer offset and length calculations followed by explicit NUL-byte assignments are best changed to either strscpy or memcpy. strscpy is not a drop-in replacement as @len would need a one subtracted from it to avoid truncating the source string. To not sabotage readability of the current code, use memcpy (retaining the manual NUL assignment) as this unambiguously describes the desired behavior. Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1] Link: KSPP#90 [2] Cc: Kees Cook <keescook@chromium.org> Cc: linux-hardening@vger.kernel.org Signed-off-by: Justin Stitt <justinstitt@google.com>
Patch series "Improve the copy of task comm", v8. Using {memcpy,strncpy,strcpy,kstrdup} to copy the task comm relies on the length of task comm. Changes in the task comm could result in a destination string that is overflow. Therefore, we should explicitly ensure the destination string is always NUL-terminated, regardless of the task comm. This approach will facilitate future extensions to the task comm. As suggested by Linus [0], we can identify all relevant code with the following git grep command: git grep 'memcpy.*->comm\>' git grep 'kstrdup.*->comm\>' git grep 'strncpy.*->comm\>' git grep 'strcpy.*->comm\>' PATCH #2~#4: memcpy PATCH #5~torvalds#6: kstrdup PATCH torvalds#7: strcpy Please note that strncpy() is not included in this series as it is being tracked by another effort. [1] This patch (of 7): We want to eliminate the use of __get_task_comm() for the following reasons: - The task_lock() is unnecessary Quoted from Linus [0]: : Since user space can randomly change their names anyway, using locking : was always wrong for readers (for writers it probably does make sense : to have some lock - although practically speaking nobody cares there : either, but at least for a writer some kind of race could have : long-term mixed results Link: https://lkml.kernel.org/r/20241007144911.27693-1-laoar.shao@gmail.com Link: https://lkml.kernel.org/r/20241007144911.27693-2-laoar.shao@gmail.com Link: https://lore.kernel.org/all/CAHk-=wivfrF0_zvf+oj6==Sh=-npJooP8chLPEfaFV0oNYTTBA@mail.gmail.com [0] Link: https://lore.kernel.org/all/CAHk-=whWtUC-AjmGJveAETKOMeMFSTwKwu99v7+b6AyHMmaDFA@mail.gmail.com/ Link: https://lore.kernel.org/all/CAHk-=wjAmmHUg6vho1KjzQi2=psR30+CogFd4aXrThr2gsiS4g@mail.gmail.com/ [0] Link: KSPP#90 [1] Signed-off-by: Yafang Shao <laoar.shao@gmail.com> Suggested-by: Linus Torvalds <torvalds@linux-foundation.org> Cc: Alexander Viro <viro@zeniv.linux.org.uk> Cc: Christian Brauner <brauner@kernel.org> Cc: Jan Kara <jack@suse.cz> Cc: Eric Biederman <ebiederm@xmission.com> Cc: Kees Cook <keescook@chromium.org> Cc: Alexei Starovoitov <alexei.starovoitov@gmail.com> Cc: Matus Jokay <matus.jokay@stuba.sk> Cc: Alejandro Colomar <alx@kernel.org> Cc: "Serge E. Hallyn" <serge@hallyn.com> Cc: Catalin Marinas <catalin.marinas@arm.com> Cc: Justin Stitt <justinstitt@google.com> Cc: Steven Rostedt (Google) <rostedt@goodmis.org> Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> Cc: Andy Shevchenko <andy.shevchenko@gmail.com> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> Cc: David Airlie <airlied@gmail.com> Cc: Eric Paris <eparis@redhat.com> Cc: James Morris <jmorris@namei.org> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Cc: Matthew Wilcox <willy@infradead.org> Cc: Maxime Ripard <mripard@kernel.org> Cc: Ondrej Mosnacek <omosnace@redhat.com> Cc: Paul Moore <paul@paul-moore.com> Cc: Quentin Monnet <qmo@kernel.org> Cc: Simon Horman <horms@kernel.org> Cc: Stephen Smalley <stephen.smalley.work@gmail.com> Cc: Thomas Zimmermann <tzimmermann@suse.de> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
Patch series "Improve the copy of task comm", v8. Using {memcpy,strncpy,strcpy,kstrdup} to copy the task comm relies on the length of task comm. Changes in the task comm could result in a destination string that is overflow. Therefore, we should explicitly ensure the destination string is always NUL-terminated, regardless of the task comm. This approach will facilitate future extensions to the task comm. As suggested by Linus [0], we can identify all relevant code with the following git grep command: git grep 'memcpy.*->comm\>' git grep 'kstrdup.*->comm\>' git grep 'strncpy.*->comm\>' git grep 'strcpy.*->comm\>' PATCH #2~#4: memcpy PATCH #5~torvalds#6: kstrdup PATCH torvalds#7: strcpy Please note that strncpy() is not included in this series as it is being tracked by another effort. [1] This patch (of 7): We want to eliminate the use of __get_task_comm() for the following reasons: - The task_lock() is unnecessary Quoted from Linus [0]: : Since user space can randomly change their names anyway, using locking : was always wrong for readers (for writers it probably does make sense : to have some lock - although practically speaking nobody cares there : either, but at least for a writer some kind of race could have : long-term mixed results Link: https://lkml.kernel.org/r/20241007144911.27693-1-laoar.shao@gmail.com Link: https://lkml.kernel.org/r/20241007144911.27693-2-laoar.shao@gmail.com Link: https://lore.kernel.org/all/CAHk-=wivfrF0_zvf+oj6==Sh=-npJooP8chLPEfaFV0oNYTTBA@mail.gmail.com [0] Link: https://lore.kernel.org/all/CAHk-=whWtUC-AjmGJveAETKOMeMFSTwKwu99v7+b6AyHMmaDFA@mail.gmail.com/ Link: https://lore.kernel.org/all/CAHk-=wjAmmHUg6vho1KjzQi2=psR30+CogFd4aXrThr2gsiS4g@mail.gmail.com/ [0] Link: KSPP#90 [1] Signed-off-by: Yafang Shao <laoar.shao@gmail.com> Suggested-by: Linus Torvalds <torvalds@linux-foundation.org> Cc: Alexander Viro <viro@zeniv.linux.org.uk> Cc: Christian Brauner <brauner@kernel.org> Cc: Jan Kara <jack@suse.cz> Cc: Eric Biederman <ebiederm@xmission.com> Cc: Kees Cook <keescook@chromium.org> Cc: Alexei Starovoitov <alexei.starovoitov@gmail.com> Cc: Matus Jokay <matus.jokay@stuba.sk> Cc: Alejandro Colomar <alx@kernel.org> Cc: "Serge E. Hallyn" <serge@hallyn.com> Cc: Catalin Marinas <catalin.marinas@arm.com> Cc: Justin Stitt <justinstitt@google.com> Cc: Steven Rostedt (Google) <rostedt@goodmis.org> Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> Cc: Andy Shevchenko <andy.shevchenko@gmail.com> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> Cc: David Airlie <airlied@gmail.com> Cc: Eric Paris <eparis@redhat.com> Cc: James Morris <jmorris@namei.org> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Cc: Matthew Wilcox <willy@infradead.org> Cc: Maxime Ripard <mripard@kernel.org> Cc: Ondrej Mosnacek <omosnace@redhat.com> Cc: Paul Moore <paul@paul-moore.com> Cc: Quentin Monnet <qmo@kernel.org> Cc: Simon Horman <horms@kernel.org> Cc: Stephen Smalley <stephen.smalley.work@gmail.com> Cc: Thomas Zimmermann <tzimmermann@suse.de> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
Patch series "Improve the copy of task comm", v8. Using {memcpy,strncpy,strcpy,kstrdup} to copy the task comm relies on the length of task comm. Changes in the task comm could result in a destination string that is overflow. Therefore, we should explicitly ensure the destination string is always NUL-terminated, regardless of the task comm. This approach will facilitate future extensions to the task comm. As suggested by Linus [0], we can identify all relevant code with the following git grep command: git grep 'memcpy.*->comm\>' git grep 'kstrdup.*->comm\>' git grep 'strncpy.*->comm\>' git grep 'strcpy.*->comm\>' PATCH #2~#4: memcpy PATCH #5~torvalds#6: kstrdup PATCH torvalds#7: strcpy Please note that strncpy() is not included in this series as it is being tracked by another effort. [1] This patch (of 7): We want to eliminate the use of __get_task_comm() for the following reasons: - The task_lock() is unnecessary Quoted from Linus [0]: : Since user space can randomly change their names anyway, using locking : was always wrong for readers (for writers it probably does make sense : to have some lock - although practically speaking nobody cares there : either, but at least for a writer some kind of race could have : long-term mixed results Link: https://lkml.kernel.org/r/20241007144911.27693-1-laoar.shao@gmail.com Link: https://lkml.kernel.org/r/20241007144911.27693-2-laoar.shao@gmail.com Link: https://lore.kernel.org/all/CAHk-=wivfrF0_zvf+oj6==Sh=-npJooP8chLPEfaFV0oNYTTBA@mail.gmail.com [0] Link: https://lore.kernel.org/all/CAHk-=whWtUC-AjmGJveAETKOMeMFSTwKwu99v7+b6AyHMmaDFA@mail.gmail.com/ Link: https://lore.kernel.org/all/CAHk-=wjAmmHUg6vho1KjzQi2=psR30+CogFd4aXrThr2gsiS4g@mail.gmail.com/ [0] Link: KSPP#90 [1] Signed-off-by: Yafang Shao <laoar.shao@gmail.com> Suggested-by: Linus Torvalds <torvalds@linux-foundation.org> Cc: Alexander Viro <viro@zeniv.linux.org.uk> Cc: Christian Brauner <brauner@kernel.org> Cc: Jan Kara <jack@suse.cz> Cc: Eric Biederman <ebiederm@xmission.com> Cc: Kees Cook <keescook@chromium.org> Cc: Alexei Starovoitov <alexei.starovoitov@gmail.com> Cc: Matus Jokay <matus.jokay@stuba.sk> Cc: Alejandro Colomar <alx@kernel.org> Cc: "Serge E. Hallyn" <serge@hallyn.com> Cc: Catalin Marinas <catalin.marinas@arm.com> Cc: Justin Stitt <justinstitt@google.com> Cc: Steven Rostedt (Google) <rostedt@goodmis.org> Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> Cc: Andy Shevchenko <andy.shevchenko@gmail.com> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> Cc: David Airlie <airlied@gmail.com> Cc: Eric Paris <eparis@redhat.com> Cc: James Morris <jmorris@namei.org> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Cc: Matthew Wilcox <willy@infradead.org> Cc: Maxime Ripard <mripard@kernel.org> Cc: Ondrej Mosnacek <omosnace@redhat.com> Cc: Paul Moore <paul@paul-moore.com> Cc: Quentin Monnet <qmo@kernel.org> Cc: Simon Horman <horms@kernel.org> Cc: Stephen Smalley <stephen.smalley.work@gmail.com> Cc: Thomas Zimmermann <tzimmermann@suse.de> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
strncpy() is deprecated for use on NUL-terminated destination strings [1] and as such we should prefer more robust and less ambiguous string interfaces. In this particular instance, the usage of strncpy() is fine and works as expected. However, towards the goal of [2], we should consider replacing it with an alternative as many instances of strncpy() are bug-prone. Its removal from the kernel promotes better long term health for the codebase. The current usage of strncpy() likely just wants the NUL-padding behavior offered by strncpy() and doesn't care about the NUL-termination. Since the compiler doesn't know the size of @DesT, we can't use strtomem_pad(). Instead, use strscpy_pad() which behaves functionally the same as strncpy() in this context -- as we expect br_dev->name to be NUL-terminated itself. Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1] Link: KSPP#90 [2] Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html Cc: Kees Cook <keescook@chromium.org> Cc: linux-hardening@vger.kernel.org Signed-off-by: Justin Stitt <justinstitt@google.com> Reviewed-by: Simon Horman <horms@kernel.org> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
Patch series "Improve the copy of task comm", v8. Using {memcpy,strncpy,strcpy,kstrdup} to copy the task comm relies on the length of task comm. Changes in the task comm could result in a destination string that is overflow. Therefore, we should explicitly ensure the destination string is always NUL-terminated, regardless of the task comm. This approach will facilitate future extensions to the task comm. As suggested by Linus [0], we can identify all relevant code with the following git grep command: git grep 'memcpy.*->comm\>' git grep 'kstrdup.*->comm\>' git grep 'strncpy.*->comm\>' git grep 'strcpy.*->comm\>' PATCH #2~#4: memcpy PATCH #5~torvalds#6: kstrdup PATCH torvalds#7: strcpy Please note that strncpy() is not included in this series as it is being tracked by another effort. [1] This patch (of 7): We want to eliminate the use of __get_task_comm() for the following reasons: - The task_lock() is unnecessary Quoted from Linus [0]: : Since user space can randomly change their names anyway, using locking : was always wrong for readers (for writers it probably does make sense : to have some lock - although practically speaking nobody cares there : either, but at least for a writer some kind of race could have : long-term mixed results Link: https://lkml.kernel.org/r/20241007144911.27693-1-laoar.shao@gmail.com Link: https://lkml.kernel.org/r/20241007144911.27693-2-laoar.shao@gmail.com Link: https://lore.kernel.org/all/CAHk-=wivfrF0_zvf+oj6==Sh=-npJooP8chLPEfaFV0oNYTTBA@mail.gmail.com [0] Link: https://lore.kernel.org/all/CAHk-=whWtUC-AjmGJveAETKOMeMFSTwKwu99v7+b6AyHMmaDFA@mail.gmail.com/ Link: https://lore.kernel.org/all/CAHk-=wjAmmHUg6vho1KjzQi2=psR30+CogFd4aXrThr2gsiS4g@mail.gmail.com/ [0] Link: KSPP#90 [1] Signed-off-by: Yafang Shao <laoar.shao@gmail.com> Suggested-by: Linus Torvalds <torvalds@linux-foundation.org> Cc: Alexander Viro <viro@zeniv.linux.org.uk> Cc: Christian Brauner <brauner@kernel.org> Cc: Jan Kara <jack@suse.cz> Cc: Eric Biederman <ebiederm@xmission.com> Cc: Kees Cook <keescook@chromium.org> Cc: Alexei Starovoitov <alexei.starovoitov@gmail.com> Cc: Matus Jokay <matus.jokay@stuba.sk> Cc: Alejandro Colomar <alx@kernel.org> Cc: "Serge E. Hallyn" <serge@hallyn.com> Cc: Catalin Marinas <catalin.marinas@arm.com> Cc: Justin Stitt <justinstitt@google.com> Cc: Steven Rostedt (Google) <rostedt@goodmis.org> Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> Cc: Andy Shevchenko <andy.shevchenko@gmail.com> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> Cc: David Airlie <airlied@gmail.com> Cc: Eric Paris <eparis@redhat.com> Cc: James Morris <jmorris@namei.org> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Cc: Matthew Wilcox <willy@infradead.org> Cc: Maxime Ripard <mripard@kernel.org> Cc: Ondrej Mosnacek <omosnace@redhat.com> Cc: Paul Moore <paul@paul-moore.com> Cc: Quentin Monnet <qmo@kernel.org> Cc: Simon Horman <horms@kernel.org> Cc: Stephen Smalley <stephen.smalley.work@gmail.com> Cc: Thomas Zimmermann <tzimmermann@suse.de> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
Patch series "Improve the copy of task comm", v8. Using {memcpy,strncpy,strcpy,kstrdup} to copy the task comm relies on the length of task comm. Changes in the task comm could result in a destination string that is overflow. Therefore, we should explicitly ensure the destination string is always NUL-terminated, regardless of the task comm. This approach will facilitate future extensions to the task comm. As suggested by Linus [0], we can identify all relevant code with the following git grep command: git grep 'memcpy.*->comm\>' git grep 'kstrdup.*->comm\>' git grep 'strncpy.*->comm\>' git grep 'strcpy.*->comm\>' PATCH #2~#4: memcpy PATCH #5~torvalds#6: kstrdup PATCH torvalds#7: strcpy Please note that strncpy() is not included in this series as it is being tracked by another effort. [1] This patch (of 7): We want to eliminate the use of __get_task_comm() for the following reasons: - The task_lock() is unnecessary Quoted from Linus [0]: : Since user space can randomly change their names anyway, using locking : was always wrong for readers (for writers it probably does make sense : to have some lock - although practically speaking nobody cares there : either, but at least for a writer some kind of race could have : long-term mixed results Link: https://lkml.kernel.org/r/20241007144911.27693-1-laoar.shao@gmail.com Link: https://lkml.kernel.org/r/20241007144911.27693-2-laoar.shao@gmail.com Link: https://lore.kernel.org/all/CAHk-=wivfrF0_zvf+oj6==Sh=-npJooP8chLPEfaFV0oNYTTBA@mail.gmail.com [0] Link: https://lore.kernel.org/all/CAHk-=whWtUC-AjmGJveAETKOMeMFSTwKwu99v7+b6AyHMmaDFA@mail.gmail.com/ Link: https://lore.kernel.org/all/CAHk-=wjAmmHUg6vho1KjzQi2=psR30+CogFd4aXrThr2gsiS4g@mail.gmail.com/ [0] Link: KSPP#90 [1] Signed-off-by: Yafang Shao <laoar.shao@gmail.com> Suggested-by: Linus Torvalds <torvalds@linux-foundation.org> Cc: Alexander Viro <viro@zeniv.linux.org.uk> Cc: Christian Brauner <brauner@kernel.org> Cc: Jan Kara <jack@suse.cz> Cc: Eric Biederman <ebiederm@xmission.com> Cc: Kees Cook <keescook@chromium.org> Cc: Alexei Starovoitov <alexei.starovoitov@gmail.com> Cc: Matus Jokay <matus.jokay@stuba.sk> Cc: Alejandro Colomar <alx@kernel.org> Cc: "Serge E. Hallyn" <serge@hallyn.com> Cc: Catalin Marinas <catalin.marinas@arm.com> Cc: Justin Stitt <justinstitt@google.com> Cc: Steven Rostedt (Google) <rostedt@goodmis.org> Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> Cc: Andy Shevchenko <andy.shevchenko@gmail.com> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> Cc: David Airlie <airlied@gmail.com> Cc: Eric Paris <eparis@redhat.com> Cc: James Morris <jmorris@namei.org> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Cc: Matthew Wilcox <willy@infradead.org> Cc: Maxime Ripard <mripard@kernel.org> Cc: Ondrej Mosnacek <omosnace@redhat.com> Cc: Paul Moore <paul@paul-moore.com> Cc: Quentin Monnet <qmo@kernel.org> Cc: Simon Horman <horms@kernel.org> Cc: Stephen Smalley <stephen.smalley.work@gmail.com> Cc: Thomas Zimmermann <tzimmermann@suse.de> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
Patch series "Improve the copy of task comm", v8. Using {memcpy,strncpy,strcpy,kstrdup} to copy the task comm relies on the length of task comm. Changes in the task comm could result in a destination string that is overflow. Therefore, we should explicitly ensure the destination string is always NUL-terminated, regardless of the task comm. This approach will facilitate future extensions to the task comm. As suggested by Linus [0], we can identify all relevant code with the following git grep command: git grep 'memcpy.*->comm\>' git grep 'kstrdup.*->comm\>' git grep 'strncpy.*->comm\>' git grep 'strcpy.*->comm\>' PATCH #2~#4: memcpy PATCH #5~torvalds#6: kstrdup PATCH torvalds#7: strcpy Please note that strncpy() is not included in this series as it is being tracked by another effort. [1] This patch (of 7): We want to eliminate the use of __get_task_comm() for the following reasons: - The task_lock() is unnecessary Quoted from Linus [0]: : Since user space can randomly change their names anyway, using locking : was always wrong for readers (for writers it probably does make sense : to have some lock - although practically speaking nobody cares there : either, but at least for a writer some kind of race could have : long-term mixed results Link: https://lkml.kernel.org/r/20241007144911.27693-1-laoar.shao@gmail.com Link: https://lkml.kernel.org/r/20241007144911.27693-2-laoar.shao@gmail.com Link: https://lore.kernel.org/all/CAHk-=wivfrF0_zvf+oj6==Sh=-npJooP8chLPEfaFV0oNYTTBA@mail.gmail.com [0] Link: https://lore.kernel.org/all/CAHk-=whWtUC-AjmGJveAETKOMeMFSTwKwu99v7+b6AyHMmaDFA@mail.gmail.com/ Link: https://lore.kernel.org/all/CAHk-=wjAmmHUg6vho1KjzQi2=psR30+CogFd4aXrThr2gsiS4g@mail.gmail.com/ [0] Link: KSPP#90 [1] Signed-off-by: Yafang Shao <laoar.shao@gmail.com> Suggested-by: Linus Torvalds <torvalds@linux-foundation.org> Cc: Alexander Viro <viro@zeniv.linux.org.uk> Cc: Christian Brauner <brauner@kernel.org> Cc: Jan Kara <jack@suse.cz> Cc: Eric Biederman <ebiederm@xmission.com> Cc: Kees Cook <keescook@chromium.org> Cc: Alexei Starovoitov <alexei.starovoitov@gmail.com> Cc: Matus Jokay <matus.jokay@stuba.sk> Cc: Alejandro Colomar <alx@kernel.org> Cc: "Serge E. Hallyn" <serge@hallyn.com> Cc: Catalin Marinas <catalin.marinas@arm.com> Cc: Justin Stitt <justinstitt@google.com> Cc: Steven Rostedt (Google) <rostedt@goodmis.org> Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> Cc: Andy Shevchenko <andy.shevchenko@gmail.com> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> Cc: David Airlie <airlied@gmail.com> Cc: Eric Paris <eparis@redhat.com> Cc: James Morris <jmorris@namei.org> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Cc: Matthew Wilcox <willy@infradead.org> Cc: Maxime Ripard <mripard@kernel.org> Cc: Ondrej Mosnacek <omosnace@redhat.com> Cc: Paul Moore <paul@paul-moore.com> Cc: Quentin Monnet <qmo@kernel.org> Cc: Simon Horman <horms@kernel.org> Cc: Stephen Smalley <stephen.smalley.work@gmail.com> Cc: Thomas Zimmermann <tzimmermann@suse.de> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
Patch series "Improve the copy of task comm", v8. Using {memcpy,strncpy,strcpy,kstrdup} to copy the task comm relies on the length of task comm. Changes in the task comm could result in a destination string that is overflow. Therefore, we should explicitly ensure the destination string is always NUL-terminated, regardless of the task comm. This approach will facilitate future extensions to the task comm. As suggested by Linus [0], we can identify all relevant code with the following git grep command: git grep 'memcpy.*->comm\>' git grep 'kstrdup.*->comm\>' git grep 'strncpy.*->comm\>' git grep 'strcpy.*->comm\>' PATCH #2~#4: memcpy PATCH #5~torvalds#6: kstrdup PATCH torvalds#7: strcpy Please note that strncpy() is not included in this series as it is being tracked by another effort. [1] This patch (of 7): We want to eliminate the use of __get_task_comm() for the following reasons: - The task_lock() is unnecessary Quoted from Linus [0]: : Since user space can randomly change their names anyway, using locking : was always wrong for readers (for writers it probably does make sense : to have some lock - although practically speaking nobody cares there : either, but at least for a writer some kind of race could have : long-term mixed results Link: https://lkml.kernel.org/r/20241007144911.27693-1-laoar.shao@gmail.com Link: https://lkml.kernel.org/r/20241007144911.27693-2-laoar.shao@gmail.com Link: https://lore.kernel.org/all/CAHk-=wivfrF0_zvf+oj6==Sh=-npJooP8chLPEfaFV0oNYTTBA@mail.gmail.com [0] Link: https://lore.kernel.org/all/CAHk-=whWtUC-AjmGJveAETKOMeMFSTwKwu99v7+b6AyHMmaDFA@mail.gmail.com/ Link: https://lore.kernel.org/all/CAHk-=wjAmmHUg6vho1KjzQi2=psR30+CogFd4aXrThr2gsiS4g@mail.gmail.com/ [0] Link: KSPP#90 [1] Signed-off-by: Yafang Shao <laoar.shao@gmail.com> Suggested-by: Linus Torvalds <torvalds@linux-foundation.org> Cc: Alexander Viro <viro@zeniv.linux.org.uk> Cc: Christian Brauner <brauner@kernel.org> Cc: Jan Kara <jack@suse.cz> Cc: Eric Biederman <ebiederm@xmission.com> Cc: Kees Cook <keescook@chromium.org> Cc: Alexei Starovoitov <alexei.starovoitov@gmail.com> Cc: Matus Jokay <matus.jokay@stuba.sk> Cc: Alejandro Colomar <alx@kernel.org> Cc: "Serge E. Hallyn" <serge@hallyn.com> Cc: Catalin Marinas <catalin.marinas@arm.com> Cc: Justin Stitt <justinstitt@google.com> Cc: Steven Rostedt (Google) <rostedt@goodmis.org> Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> Cc: Andy Shevchenko <andy.shevchenko@gmail.com> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> Cc: David Airlie <airlied@gmail.com> Cc: Eric Paris <eparis@redhat.com> Cc: James Morris <jmorris@namei.org> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Cc: Matthew Wilcox <willy@infradead.org> Cc: Maxime Ripard <mripard@kernel.org> Cc: Ondrej Mosnacek <omosnace@redhat.com> Cc: Paul Moore <paul@paul-moore.com> Cc: Quentin Monnet <qmo@kernel.org> Cc: Simon Horman <horms@kernel.org> Cc: Stephen Smalley <stephen.smalley.work@gmail.com> Cc: Thomas Zimmermann <tzimmermann@suse.de> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
The
strncpy()
function is actively dangerous to use since it may not NUL-terminate the destination string, resulting in potential memory content exposures, unbounded reads, or crashes. Replacing uses requires some careful attention, though, sincestrncpy
gets used also for two other cases:NLA_STRING
where the length stored separately, andNLA_NUL_STRING
which uses a "traditional" NUL-terminated string. For the cases wherestrncpy()
is used to copy non-NUL-terminated strings, the destination buffer needs to be marked with the__nonstring
attribute, so that compiler diagnostics will avoid warning about cases where the character array is considered NUL-terminated.strscpy_pad()
, ormemcpy_and_pad()
when the destination is length-bounded.https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings
The workflow to replace
strncpy()
, therefore, needs to be:kstrndup()
strscpy_pad()
strscpy()
__nonstring
attribute and:memcpy_and_pad(dst, sizeof(dst), src, min(sizeof(dst), strnlen(src, sizeof(dst)), pad_char)
(perhaps this needs a macro)memcpy(dst, src, min(sizeof(dst), strnlen(src, sizeof(dst))
(perhaps this needs a macro)One can find instances to replace using this search:
git grep '\bstrncpy\b' | grep -vE '^(Documentation|tools|samples|scripts|arch/.*/lib)/' | grep -vE 'fortify|include/linux/string.h|lib/string.c'
The text was updated successfully, but these errors were encountered: