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>
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 can see that client->name should be NUL-terminated based on its usage with a %s C-string format specifier. | client->thread = kthread_run(ioreq_task, client, "VM%u-%s", | client->vm->vmid, client->name); NUL-padding is not required as client is already zero-allocated: | client = kzalloc(sizeof(*client), 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. Note that this patch relies on the _new_ 2-argument version of strscpy() introduced in Commit e6584c3 ("string: Allow 2-argument strscpy()"). 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#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/20240320-strncpy-drivers-virt-acrn-ioreq-c-v1-1-db6996770341@google.com Signed-off-by: Kees Cook <keescook@chromium.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. Our goal here is to get @namebuf populated with @name's contents but surrounded with quotes. There is some careful handling done to ensure we properly truncate @name so that we have room for a literal quote as well as a NUL-term. All this careful handling can be done with scnprintf using the dynamic string width specifier %.*s which allows us to pass in the max size for a source string. Doing this, we can put literal quotes in our format specifier and ensure @name is truncated to fit inbetween these quotes (-3 is from 2 quotes + 1 NUL-byte). All in all, we get to remove a deprecated use of strncpy and clean up this code nicely! 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#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/20240328-strncpy-fs-reiserfs-item_ops-c-v1-1-2dab6d22a996@google.com Signed-off-by: Kees Cook <keescook@chromium.org>
strncpy() is deprecated with NUL-terminated destination strings [1]. The copy_name() method does a lot of manual buffer manipulation to eventually arrive with its desired string. If we don't know the namespace this attr has or belongs to we want to prepend "osx." to our final string. Following this, we're copying xattr_name and doing a bizarre manual NUL-byte assignment with a memset where n=1. Really, we can use some more obvious string APIs to acomplish this, improving readability and security. Following the same control flow as before: if we don't know the namespace let's use scnprintf() to form our prefix + xattr_name pairing (while NUL-terminating too!). Otherwise, use strscpy() to return the number of bytes copied into our buffer. Additionally, for non-empty strings, include the NUL-byte in the length -- matching the behavior of the previous implementation. Note that strscpy() _can_ return -E2BIG but this is already handled by all callsites: In both hfsplus_listxattr_finder_info() and hfsplus_listxattr(), ret is already type ssize_t so we can change the return type of copy_name() to match (understanding that scnprintf()'s return type is different yet fully representable by ssize_t). Furthermore, listxattr() in fs/xattr.c is well-equipped to handle a potential -E2BIG return result from vfs_listxattr(): | ssize_t error; ... | error = vfs_listxattr(d, klist, size); | if (error > 0) { | if (size && copy_to_user(list, klist, error)) | error = -EFAULT; | } else if (error == -ERANGE && size >= XATTR_LIST_MAX) { | /* The file system tried to returned a list bigger | than XATTR_LIST_MAX bytes. Not possible. */ | error = -E2BIG; | } ... the error can potentially already be -E2BIG, skipping this else-if and ending up at the same state as other errors. 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#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/20240401-strncpy-fs-hfsplus-xattr-c-v2-1-6e089999355e@google.com Signed-off-by: Kees Cook <keescook@chromium.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. A good alternative is strscpy() as it guarantees NUL-termination on the destination buffer. In crypto.c: We expect cipher_name to be NUL-terminated based on its use with the C-string format specifier %s and with other string apis like strlen(): | printk(KERN_ERR "Error attempting to initialize key TFM " | "cipher with name = [%s]; rc = [%d]\n", | tmp_tfm->cipher_name, rc); and | int cipher_name_len = strlen(cipher_name); In main.c: We can remove the manual NUL-byte assignments as well as the pointers to destinations (which I assume only existed to trim down on line length?) in favor of directly using the destination buffer which allows the compiler to get size information -- enabling the usage of the new 2-argument strscpy(). Note that this patch relies on the _new_ 2-argument versions of strscpy() and strscpy_pad() introduced in Commit e6584c3 ("string: Allow 2-argument strscpy()"). 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#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/20240321-strncpy-fs-ecryptfs-crypto-c-v1-1-d78b74c214ac@google.com Signed-off-by: Kees Cook <keescook@chromium.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. A better alternative is strscpy() as it guarantees NUL-termination on the destination buffer. Since we are eventually copying over to userspace, let's ensure we NUL-pad the destination buffer by using the pad variant of strscpy. - core/fb_chrdev.c: 234 | err = copy_to_user(&fix32->id, &fix->id, sizeof(fix32->id)); Furthermore, we can use the new 2-argument variants of strscpy() and strscpy_pad() introduced by Commit e6584c3 ("string: Allow 2-argument strscpy()") to simplify the syntax even more. 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#90 Cc: linux-hardening@vger.kernel.org Signed-off-by: Justin Stitt <justinstitt@google.com> Reviewed-by: Kees Cook <keescook@chromium.org> Signed-off-by: Helge Deller <deller@gmx.de>
strncpy() is deprecated for use on NUL-terminated destination strings [1] and as such we should prefer more robust and less ambiguous string interfaces. Let's use the new 2-argument strscpy() which guarantees NUL-termination on the destination buffer while also simplifying the syntax. Note that strscpy() will not NUL-pad the destination buffer like strncpy() does. However, the NUL-padding behavior of strncpy() is not required since fbdev is already NUL-allocated from au1200fb_drv_probe() -> frameuffer_alloc(), rendering any additional NUL-padding redundant. | p = kzalloc(fb_info_size + size, GFP_KERNEL); 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#90 Cc: linux-hardening@vger.kernel.org Signed-off-by: Justin Stitt <justinstitt@google.com> Reviewed-by: Kees Cook <keescook@chromium.org> Signed-off-by: Helge Deller <deller@gmx.de>
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 v86d_path to be NUL-terminated based on its use with the C-string format specifier in printf-likes: | pr_err("failed to execute %s\n", v86d_path); and | return snprintf(buf, PAGE_SIZE, "%s\n", v86d_path); Let's also opt to pad v86d_path since it may get used in and around userspace: | return call_usermodehelper(v86d_path, argv, envp, UMH_WAIT_PROC); Considering the above, strscpy_pad() is the best replacement as it guarantees both NUL-termination and NUL-padding on the destination buffer. Note that this patch relies on the _new_ 2-argument versions of strscpy() and strscpy_pad() introduced in Commit e6584c3 ("string: Allow 2-argument strscpy()"). 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#90 Cc: linux-hardening@vger.kernel.org Signed-off-by: Justin Stitt <justinstitt@google.com> Reviewed-by: Kees Cook <keescook@chromium.org> Signed-off-by: Helge Deller <deller@gmx.de>
strncpy() is deprecated for use on NUL-terminated destination strings [1] and as such we should prefer more robust and less ambiguous string interfaces. It looks like the main use of strncpy() here is to limit the amount of bytes printed from hdmi_log() by using a tmp buffer and limiting the number of bytes copied. Really, we should use the %.<len>s format qualifier to achieve this. 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#90 Cc: linux-hardening@vger.kernel.org Signed-off-by: Justin Stitt <justinstitt@google.com> Reviewed-by: Kees Cook <keescook@chromium.org> Signed-off-by: Helge Deller <deller@gmx.de>
strncpy() is in the process of being replaced as it is deprecated [1]. We should move towards safer and less ambiguous string interfaces. Looking at vmcoredd_header's definition: | struct vmcoredd_header { | __u32 n_namesz; /* Name size */ | __u32 n_descsz; /* Content size */ | __u32 n_type; /* NT_VMCOREDD */ | __u8 name[8]; /* LINUX\0\0\0 */ | __u8 dump_name[VMCOREDD_MAX_NAME_BYTES]; /* Device dump's name */ | }; .. we see that @name wants to be NUL-padded. We're copying data->dump_name which is defined as: | char dump_name[VMCOREDD_MAX_NAME_BYTES]; /* Unique name of the dump */ .. which shares the same size as vdd_hdr->dump_name. Let's make sure we NUL-pad this as well. Use strscpy_pad() which NUL-terminates and NUL-pads its destination buffers. Specifically, use the new 2-argument version of strscpy_pad introduced in Commit e6584c3 ("string: Allow 2-argument strscpy()"). Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1] Link: KSPP#90 Link: https://lkml.kernel.org/r/20240401-strncpy-fs-proc-vmcore-c-v2-1-dd0a73f42635@google.com Signed-off-by: Justin Stitt <justinstitt@google.com> Acked-by: Baoquan He <bhe@redhat.com> Cc: Dave Young <dyoung@redhat.com> Cc: Vivek Goyal <vgoyal@redhat.com> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
This pattern of strncpy with some pointer arithmetic setting fixed-sized intervals with string literal data is a bit weird so let's use ethtool_puts() as this has more obvious behavior and is less-error prone. Nicely, we also get to drop a usage of the now deprecated strncpy() [1]. Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1] Link: KSPP#90 Cc: linux-hardening@vger.kernel.org Cc: Kees Cook <keescook@chromium.org> Suggested-by: Alexander Lobakin <aleksander.lobakin@intel.com> Reviewed-by: Kees Cook <keescook@chromium.org> Signed-off-by: Justin Stitt <justinstitt@google.com> Signed-off-by: NipaLocal <nipa@local>
This pattern of strncpy with some pointer arithmetic setting fixed-sized intervals with string literal data is a bit weird so let's use ethtool_puts() as this has more obvious behavior and is less-error prone. Nicely, we also get to drop a usage of the now deprecated strncpy() [1]. Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1] Link: KSPP#90 Cc: linux-hardening@vger.kernel.org Cc: Kees Cook <keescook@chromium.org> Suggested-by: Alexander Lobakin <aleksander.lobakin@intel.com> Reviewed-by: Kees Cook <keescook@chromium.org> Signed-off-by: Justin Stitt <justinstitt@google.com> Signed-off-by: NipaLocal <nipa@local>
This pattern of strncpy with some pointer arithmetic setting fixed-sized intervals with string literal data is a bit weird so let's use ethtool_puts() as this has more obvious behavior and is less-error prone. Nicely, we also get to drop a usage of the now deprecated strncpy() [1]. Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1] Link: KSPP#90 Suggested-by: Alexander Lobakin <aleksander.lobakin@intel.com> Reviewed-by: Kees Cook <keescook@chromium.org> Signed-off-by: Justin Stitt <justinstitt@google.com> Link: https://lore.kernel.org/r/20240425-strncpy-drivers-net-dsa-lan9303-core-c-v4-1-9fafd419d7bb@google.com Signed-off-by: Jakub Kicinski <kuba@kernel.org>
strncpy() is in the process of being replaced as it is deprecated [1]. We should move towards safer and less ambiguous string interfaces. Looking at vmcoredd_header's definition: | struct vmcoredd_header { | __u32 n_namesz; /* Name size */ | __u32 n_descsz; /* Content size */ | __u32 n_type; /* NT_VMCOREDD */ | __u8 name[8]; /* LINUX\0\0\0 */ | __u8 dump_name[VMCOREDD_MAX_NAME_BYTES]; /* Device dump's name */ | }; .. we see that @name wants to be NUL-padded. We're copying data->dump_name which is defined as: | char dump_name[VMCOREDD_MAX_NAME_BYTES]; /* Unique name of the dump */ .. which shares the same size as vdd_hdr->dump_name. Let's make sure we NUL-pad this as well. Use strscpy_pad() which NUL-terminates and NUL-pads its destination buffers. Specifically, use the new 2-argument version of strscpy_pad introduced in Commit e6584c3 ("string: Allow 2-argument strscpy()"). Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1] Link: KSPP#90 Link: https://lkml.kernel.org/r/20240401-strncpy-fs-proc-vmcore-c-v2-1-dd0a73f42635@google.com Signed-off-by: Justin Stitt <justinstitt@google.com> Acked-by: Baoquan He <bhe@redhat.com> Cc: Dave Young <dyoung@redhat.com> Cc: Vivek Goyal <vgoyal@redhat.com> 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. We can see that client->name should be NUL-terminated based on its usage with a %s C-string format specifier. | client->thread = kthread_run(ioreq_task, client, "VM%u-%s", | client->vm->vmid, client->name); NUL-padding is not required as client is already zero-allocated: | client = kzalloc(sizeof(*client), 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. Note that this patch relies on the _new_ 2-argument version of strscpy() introduced in Commit e6584c3 ("string: Allow 2-argument strscpy()"). 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#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/20240320-strncpy-drivers-virt-acrn-ioreq-c-v1-1-db6996770341@google.com Signed-off-by: Kees Cook <keescook@chromium.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. Our goal here is to get @namebuf populated with @name's contents but surrounded with quotes. There is some careful handling done to ensure we properly truncate @name so that we have room for a literal quote as well as a NUL-term. All this careful handling can be done with scnprintf using the dynamic string width specifier %.*s which allows us to pass in the max size for a source string. Doing this, we can put literal quotes in our format specifier and ensure @name is truncated to fit inbetween these quotes (-3 is from 2 quotes + 1 NUL-byte). All in all, we get to remove a deprecated use of strncpy and clean up this code nicely! 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#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/20240328-strncpy-fs-reiserfs-item_ops-c-v1-1-2dab6d22a996@google.com Signed-off-by: Kees Cook <keescook@chromium.org>
strncpy() is deprecated with NUL-terminated destination strings [1]. The copy_name() method does a lot of manual buffer manipulation to eventually arrive with its desired string. If we don't know the namespace this attr has or belongs to we want to prepend "osx." to our final string. Following this, we're copying xattr_name and doing a bizarre manual NUL-byte assignment with a memset where n=1. Really, we can use some more obvious string APIs to acomplish this, improving readability and security. Following the same control flow as before: if we don't know the namespace let's use scnprintf() to form our prefix + xattr_name pairing (while NUL-terminating too!). Otherwise, use strscpy() to return the number of bytes copied into our buffer. Additionally, for non-empty strings, include the NUL-byte in the length -- matching the behavior of the previous implementation. Note that strscpy() _can_ return -E2BIG but this is already handled by all callsites: In both hfsplus_listxattr_finder_info() and hfsplus_listxattr(), ret is already type ssize_t so we can change the return type of copy_name() to match (understanding that scnprintf()'s return type is different yet fully representable by ssize_t). Furthermore, listxattr() in fs/xattr.c is well-equipped to handle a potential -E2BIG return result from vfs_listxattr(): | ssize_t error; ... | error = vfs_listxattr(d, klist, size); | if (error > 0) { | if (size && copy_to_user(list, klist, error)) | error = -EFAULT; | } else if (error == -ERANGE && size >= XATTR_LIST_MAX) { | /* The file system tried to returned a list bigger | than XATTR_LIST_MAX bytes. Not possible. */ | error = -E2BIG; | } ... the error can potentially already be -E2BIG, skipping this else-if and ending up at the same state as other errors. 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#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/20240401-strncpy-fs-hfsplus-xattr-c-v2-1-6e089999355e@google.com Signed-off-by: Kees Cook <keescook@chromium.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. A good alternative is strscpy() as it guarantees NUL-termination on the destination buffer. In crypto.c: We expect cipher_name to be NUL-terminated based on its use with the C-string format specifier %s and with other string apis like strlen(): | printk(KERN_ERR "Error attempting to initialize key TFM " | "cipher with name = [%s]; rc = [%d]\n", | tmp_tfm->cipher_name, rc); and | int cipher_name_len = strlen(cipher_name); In main.c: We can remove the manual NUL-byte assignments as well as the pointers to destinations (which I assume only existed to trim down on line length?) in favor of directly using the destination buffer which allows the compiler to get size information -- enabling the usage of the new 2-argument strscpy(). Note that this patch relies on the _new_ 2-argument versions of strscpy() and strscpy_pad() introduced in Commit e6584c3 ("string: Allow 2-argument strscpy()"). 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#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/20240321-strncpy-fs-ecryptfs-crypto-c-v1-1-d78b74c214ac@google.com Signed-off-by: Kees Cook <keescook@chromium.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. This kernel config option is simply assigned with the resume_file buffer. It should be NUL-terminated but not necessarily NUL-padded as per its further usage with other string apis: | static int __init find_resume_device(void) | { | if (!strlen(resume_file)) | return -ENOENT; | | pm_pr_dbg("Checking hibernation image partition %s\n", resume_file); Use strscpy [2] as it guarantees NUL-termination on the destination buffer. Specifically, use the new 2-argument version of strscpy() introduced in Commit e6584c3 ("string: Allow 2-argument strscpy()"). 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#90 Cc: linux-hardening@vger.kernel.org Signed-off-by: Justin Stitt <justinstitt@google.com>
Cleanup some deprecated uses of strncpy() and strcpy() [1]. There doesn't seem to be any bugs with the current code but the readability of this code could benefit from a quick makeover while removing some deprecated stuff as a benefit. The most interesting replacement made in this patch involves concatenating "ttyS" with a digit-led user-supplied string. Instead of doing two distinct string copies with carefully managed offsets and lengths, let's use the more robust and self-explanatory scnprintf(). scnprintf will 1) respect the bounds of @buf, 2) null-terminate @buf, 3) do the concatenation. This allows us to drop the manual NUL-byte assignment. Also, since isdigit() is used about a dozen lines after the open-coded version we'll replace it for uniformity's sake. All the strcpy() --> strscpy() replacements are trivial as the source strings are literals and much smaller than the destination size. No behavioral change here. Use the new 2-argument version of strscpy() introduced in Commit e6584c3 ("string: Allow 2-argument strscpy()"). However, to make this work fully (since the size must be known at compile time), also update the extern-qualified declaration to have the proper size information. 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 [3] Cc: linux-hardening@vger.kernel.org Signed-off-by: Justin Stitt <justinstitt@google.com>
strncpy() is deprecated for use on NUL-terminated destination strings [1] and as such we should prefer more robust and less ambiguous string interfaces. data_page wants to be NUL-terminated and NUL-padded, use strscpy_pad to provide both of these. data_page no longer awkwardly relies on init_mount to perform its NUL-termination, although that sanity check is left unchanged. 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/20240402-strncpy-init-do_mounts-c-v1-1-e16d7bc20974@google.com Signed-off-by: Kees Cook <keescook@chromium.org>
JIRA: https://issues.redhat.com/browse/RHEL-24508 commit e100c01efa85c8a0ee7527bf28ef7ea7c3ca57e1 Author: Justin Stitt <justinstitt@google.com> Date: Mon Feb 26 23:53:44 2024 +0000 scsi: lpfc: Replace deprecated strncpy() with strscpy() 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 ae->value_string to be NUL-terminated because there's a comment that says as much; these attr strings are also used with other string APIs, further cementing the fact. Now, the question of whether or not to NUL-pad the destination buffer: lpfc_fdmi_rprt_defer() initializes vports (all zero-initialized), then we call lpfc_fdmi_cmd() with each vport and a mask. Then, inside of lpfc_fdmi_cmd() we check each bit in the mask to invoke the proper callback. Importantly, the zero-initialized vport is passed in as the "attr" parameter. Seeing this: | struct lpfc_fdmi_attr_string *ae = attr; ... we can tell that ae->value_string is entirely zero-initialized. Due to this, NUL-padding is _not_ required as it would be redundant. Considering the above, a suitable replacement is strscpy() [2]. 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> Link: https://lore.kernel.org/r/20240226-strncpy-drivers-scsi-lpfc-lpfc_ct-c-v2-1-2df2e46569b9@google.com Reviewed-by: Kees Cook <keescook@chromium.org> Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com> Signed-off-by: Dick Kennedy <dkennedy@redhat.com>
JIRA: https://issues.redhat.com/browse/RHEL-24466 Upstream-status: v6.8 commit 10b49d0e76513a6f6890f321a10a2df0b779c761 Author: Justin Stitt <justinstitt@google.com> Date: Wed Oct 11 21:29:57 2023 +0000 net/mlx5: simplify mlx5_set_driver_version string assignments In total, just assigning this version string takes: (1) strncpy()'s (5) strlen()'s (3) strncat()'s (1) snprintf()'s (4) max_t()'s Moreover, `strncpy` is deprecated [1] and `strncat` really shouldn't be used either [2]. With this in mind, let's simply use a single `snprintf`. Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1] Link: https://elixir.bootlin.com/linux/v6.6-rc5/source/include/linux/fortify-string.h#L448 [2] Link: KSPP/linux#90 Cc: linux-hardening@vger.kernel.org Cc: Kees Cook <keescook@chromium.org> Signed-off-by: Justin Stitt <justinstitt@google.com> Reviewed-by: Kees Cook <keescook@chromium.org> Signed-off-by: Saeed Mahameed <saeedm@nvidia.com> Signed-off-by: Benjamin Poirier <bpoirier@nvidia.com>
JIRA: https://issues.redhat.com/browse/RHEL-25547 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 both data->subsysnqn and data->hostnqn to be NUL-terminated based on their usage with format specifier ("%s"): fabrics.c: 322: dev_err(ctrl->device, 323: "%s, subsysnqn \"%s\"\n", 324: inv_data, data->subsysnqn); ... 349: dev_err(ctrl->device, 350: "Connect for subsystem %s is not allowed, hostnqn: %s\n", 351: data->subsysnqn, data->hostnqn); Moreover, there's no need to NUL-pad since `data` is zero-allocated already in fabrics.c: 383: data = kzalloc(sizeof(*data), GFP_KERNEL); ... therefore any further NUL-padding is rendered useless. 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. I opted not to switch NVMF_NQN_SIZE to sizeof(data->xyz) because the size is defined as: | /* NQN names in commands fields specified one size */ | #define NVMF_NQN_FIELD_LEN 256 ... while NVMF_NQN_SIZE is defined as: | /* However the max length of a qualified name is another size */ | #define NVMF_NQN_SIZE 223 Since 223 seems pretty magic, I'm not going to touch it. 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/20231018-strncpy-drivers-nvme-host-fabrics-c-v1-1-b6677df40a35@google.com Signed-off-by: Kees Cook <keescook@chromium.org> (cherry picked from commit 576b75f93b3d3c408235808f689453f1ed891486) Signed-off-by: Maurizio Lombardi <mlombard@redhat.com>
JIRA: https://issues.redhat.com/browse/RHEL-25547 strncpy() is deprecated for use on NUL-terminated destination strings [1] and as such we should prefer more robust and less ambiguous string interfaces. Let's instead use strscpy() [2] as it guarantees NUL-termination on the destination buffer. Moreover, there is no need to use: | min(FCNVME_ASSOC_HOSTNQN_LEN, NVMF_NQN_SIZE)); I imagine this was originally done to make sure the destination buffer is NUL-terminated by ensuring we copy a number of bytes less than the size of our destination, thus leaving some NUL-bytes at the end. However, with strscpy(), we no longer need to do this and we can instead opt for the more idiomatic strscpy() usage of: | strscpy(dest, src, sizeof(dest)) Also, no NUL-padding is required as lsop is zero-allocated: | lsop = kzalloc((sizeof(*lsop) + | sizeof(*assoc_rqst) + sizeof(*assoc_acc) + | ctrl->lport->ops->lsrqst_priv_sz), GFP_KERNEL); ... and assoc_rqst points to a field in lsop: | assoc_rqst = (struct fcnvme_ls_cr_assoc_rqst *)&lsop[1]; Therefore, any additional NUL-byte assignments (like the ones that strncpy() makes) are redundant. 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> Similar-to: https://lore.kernel.org/all/20231018-strncpy-drivers-nvme-host-fabrics-c-v1-1-b6677df40a35@google.com/ Reviewed-by: Kees Cook <keescook@chromium.org> Link: https://lore.kernel.org/r/20231019-strncpy-drivers-nvme-host-fc-c-v1-1-5805c15e4b49@google.com Signed-off-by: Kees Cook <keescook@chromium.org> (cherry picked from commit e5a4975ca463e91c2009f5950e0156f0b857eb10) Signed-off-by: Maurizio Lombardi <mlombard@redhat.com>
JIRA: https://issues.redhat.com/browse/RHEL-24474 Upstream-status: v6.7-rc1 commit 88fca39b660b97651fbf5ba18074b7999fab98a7 Author: Justin Stitt <justinstitt@google.com> Date: Wed Oct 11 21:04:37 2023 +0000 net/mlx4_core: replace deprecated strncpy with strscpy `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 `dst` to be NUL-terminated based on its use with format strings: | mlx4_dbg(dev, "Reporting Driver Version to FW: %s\n", dst); Moreover, NUL-padding is not required. 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 Signed-off-by: Justin Stitt <justinstitt@google.com> Reviewed-by: Kees Cook <keescook@chromium.org> Reviewed-by: Saeed Mahameed <saeedm@nvidia.com> Link: https://lore.kernel.org/r/20231011-strncpy-drivers-net-ethernet-mellanox-mlx4-fw-c-v1-1-4d7b5d34c933@google.com Signed-off-by: Jakub Kicinski <kuba@kernel.org> Signed-off-by: Benjamin Poirier <bpoirier@nvidia.com>
JIRA: https://issues.redhat.com/browse/RHEL-32446 commit 0faf84caee63a5f331bda130265fdceb7d4101b5 Author: Justin Stitt <justinstitt@google.com> Date: Fri, 29 Sep 2023 14:48:31 +0000 `strncpy` is deprecated for use on NUL-terminated destination strings [1]. We should prefer more robust and less ambiguous string interfaces. Both `policy->last_governor` and `default_governor` are expected to be NUL-terminated which is shown by their heavy usage with other string apis like `strcmp`. A suitable replacement is `strscpy` [2] due to the fact that it guarantees NUL-termination on the destination buffer. 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> Acked-by: Viresh Kumar <viresh.kumar@linaro.org> Link: https://lore.kernel.org/r/20230913-strncpy-drivers-cpufreq-cpufreq-c-v1-1-f1608bfeff63@google.com Signed-off-by: Kees Cook <keescook@chromium.org> Signed-off-by: Mark Langsdorf <mlangsdo@redhat.com>
JIRA: https://issues.redhat.com/browse/RHEL-32447 commit b545465e22f5fec2862132c01a5d2abd3c4c4d50 Author: Justin Stitt <justinstitt@google.com> Date: Fri, 29 Sep 2023 14:48:31 +0000 `strncpy` is deprecated for use on NUL-terminated destination strings [1]. We should prefer more robust and less ambiguous string interfaces. A suitable replacement is `strscpy` [2] due to the fact that it guarantees NUL-termination on the destination buffer. With this, we can also drop the now unnecessary `CPUIDLE_(NAME|DESC)_LEN - 1` pieces. 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/20230913-strncpy-drivers-cpuidle-dt_idle_states-c-v1-1-d16a0dbe5658@google.com Signed-off-by: Kees Cook <keescook@chromium.org> Signed-off-by: Mark Langsdorf <mlangsdo@redhat.com>
JIRA: https://issues.redhat.com/browse/RHEL-33543 Upstream Status: From upstream linux mainline strncpy() is deprecated for use on NUL-terminated destination strings [1] and as such we should prefer more robust and less ambiguous string interfaces. These labels get copied out to the user so lets make sure they are NUL-terminated and NUL-padded. vparams is already memset to 0 so we don't need to do any NUL-padding (like what strncpy() is doing). 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. Let's also opt to use the more idiomatic strscpy() usage of: (dest, src, sizeof(dest)) as this more closely ties the destination buffer to the length. 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> Link: https://lore.kernel.org/r/20231023-strncpy-drivers-scsi-ch-c-v1-1-dc67ba8075a3@google.com Reviewed-by: Kees Cook <keescook@chromium.org> Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com> (cherry picked from commit dc7a7f10e673499856dd275691768df6d07a2815) Signed-off-by: Ewan D. Milne <emilne@redhat.com>
JIRA: https://issues.redhat.com/browse/RHEL-24508 commit e100c01efa85c8a0ee7527bf28ef7ea7c3ca57e1 Author: Justin Stitt <justinstitt@google.com> Date: Mon Feb 26 23:53:44 2024 +0000 scsi: lpfc: Replace deprecated strncpy() with strscpy() 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 ae->value_string to be NUL-terminated because there's a comment that says as much; these attr strings are also used with other string APIs, further cementing the fact. Now, the question of whether or not to NUL-pad the destination buffer: lpfc_fdmi_rprt_defer() initializes vports (all zero-initialized), then we call lpfc_fdmi_cmd() with each vport and a mask. Then, inside of lpfc_fdmi_cmd() we check each bit in the mask to invoke the proper callback. Importantly, the zero-initialized vport is passed in as the "attr" parameter. Seeing this: | struct lpfc_fdmi_attr_string *ae = attr; ... we can tell that ae->value_string is entirely zero-initialized. Due to this, NUL-padding is _not_ required as it would be redundant. Considering the above, a suitable replacement is strscpy() [2]. 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> Link: https://lore.kernel.org/r/20240226-strncpy-drivers-scsi-lpfc-lpfc_ct-c-v2-1-2df2e46569b9@google.com Reviewed-by: Kees Cook <keescook@chromium.org> Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com> Signed-off-by: Dick Kennedy <dkennedy@redhat.com>
strncpy() is deprecated for use on NUL-terminated destination strings [1] and as such we should prefer more robust and less ambiguous string interfaces. data_page wants to be NUL-terminated and NUL-padded, use strscpy_pad to provide both of these. data_page no longer awkwardly relies on init_mount to perform its NUL-termination, although that sanity check is left unchanged. 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#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/20240402-strncpy-init-do_mounts-c-v1-1-e16d7bc20974@google.com Signed-off-by: Kees Cook <keescook@chromium.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)The text was updated successfully, but these errors were encountered: