| Message ID | 20230911-strncpy-drivers-acpi-osi-c-v1-1-ca2ec0667b18@google.com (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | f1fce1cf4509aa0676b363c203b9ab190c1fd8e8 |
| Headers | show |
| Series | ACPI: OSI: refactor deprecated strncpy | expand |
On Mon, Sep 11, 2023 at 08:36:44PM +0000, Justin Stitt wrote: > `strncpy` is deprecated for use on NUL-terminated destination strings [1]. > > We know `osi->string` is a NUL-terminated string due to its eventual use > in `acpi_install_interface()` and `acpi_remove_interface()` which expect > a `acpi_string` which has been specifically typedef'd as: > | typedef char *acpi_string; /* Null terminated ASCII string */ > > ... and which also has other string functions used on it like `strlen`. > Furthermore, padding is not needed in this instance either. Following the callers, I agree, this doesn't need %NUL padding -- it's always processed as a regular C string. Reviewed-by: Kees Cook <keescook@chromium.org> -Kees > > Due to the reasoning above a suitable replacement is `strscpy` [2] since > it guarantees NUL-termination on the destination buffer and doesn't > unnecessarily NUL-pad. > > While there is unlikely to be a buffer overread (or other related bug) > in this case, we should still favor a more robust and less ambiguous > interface. > > 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: https://github.com/KSPP/linux/issues/90 > Cc: linux-hardening@vger.kernel.org > Cc: Kees Cook <keescook@chromium.org> > Signed-off-by: Justin Stitt <justinstitt@google.com> > --- > Note: build-tested > --- > drivers/acpi/osi.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/acpi/osi.c b/drivers/acpi/osi.c > index d4405e1ca9b9..df9328c850bd 100644 > --- a/drivers/acpi/osi.c > +++ b/drivers/acpi/osi.c > @@ -110,7 +110,7 @@ void __init acpi_osi_setup(char *str) > break; > } else if (osi->string[0] == '\0') { > osi->enable = enable; > - strncpy(osi->string, str, OSI_STRING_LENGTH_MAX); > + strscpy(osi->string, str, OSI_STRING_LENGTH_MAX); > break; > } > } > > --- > base-commit: 2dde18cd1d8fac735875f2e4987f11817cc0bc2c > change-id: 20230911-strncpy-drivers-acpi-osi-c-c801b7427987 > > Best regards, > -- > Justin Stitt <justinstitt@google.com> >
On Fri, Sep 15, 2023 at 5:16 AM Kees Cook <keescook@chromium.org> wrote: > > On Mon, Sep 11, 2023 at 08:36:44PM +0000, Justin Stitt wrote: > > `strncpy` is deprecated for use on NUL-terminated destination strings [1]. > > > > We know `osi->string` is a NUL-terminated string due to its eventual use > > in `acpi_install_interface()` and `acpi_remove_interface()` which expect > > a `acpi_string` which has been specifically typedef'd as: > > | typedef char *acpi_string; /* Null terminated ASCII string */ > > > > ... and which also has other string functions used on it like `strlen`. > > Furthermore, padding is not needed in this instance either. > > Following the callers, I agree, this doesn't need %NUL padding -- it's > always processed as a regular C string. > > Reviewed-by: Kees Cook <keescook@chromium.org> Applied as 6.7 material, thanks!
diff --git a/drivers/acpi/osi.c b/drivers/acpi/osi.c index d4405e1ca9b9..df9328c850bd 100644 --- a/drivers/acpi/osi.c +++ b/drivers/acpi/osi.c @@ -110,7 +110,7 @@ void __init acpi_osi_setup(char *str) break; } else if (osi->string[0] == '\0') { osi->enable = enable; - strncpy(osi->string, str, OSI_STRING_LENGTH_MAX); + strscpy(osi->string, str, OSI_STRING_LENGTH_MAX); break; } }
`strncpy` is deprecated for use on NUL-terminated destination strings [1]. We know `osi->string` is a NUL-terminated string due to its eventual use in `acpi_install_interface()` and `acpi_remove_interface()` which expect a `acpi_string` which has been specifically typedef'd as: | typedef char *acpi_string; /* Null terminated ASCII string */ ... and which also has other string functions used on it like `strlen`. Furthermore, padding is not needed in this instance either. Due to the reasoning above a suitable replacement is `strscpy` [2] since it guarantees NUL-termination on the destination buffer and doesn't unnecessarily NUL-pad. While there is unlikely to be a buffer overread (or other related bug) in this case, we should still favor a more robust and less ambiguous interface. 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: https://github.com/KSPP/linux/issues/90 Cc: linux-hardening@vger.kernel.org Cc: Kees Cook <keescook@chromium.org> Signed-off-by: Justin Stitt <justinstitt@google.com> --- Note: build-tested --- drivers/acpi/osi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) --- base-commit: 2dde18cd1d8fac735875f2e4987f11817cc0bc2c change-id: 20230911-strncpy-drivers-acpi-osi-c-c801b7427987 Best regards, -- Justin Stitt <justinstitt@google.com>