Message ID | 20230905-strncpy-arch-x86-platform-uv-uv_nmi-v3-1-3efd6798b569@google.com (mailing list archive) |
---|---|
State | Superseded |
Headers | show |
Series | [v3] x86/platform/uv: refactor deprecated strcpy and strncpy | expand |
Hi, On 9/5/23 23:54, Justin Stitt wrote: > Both `strncpy` and `strcpy` are deprecated for use on NUL-terminated > destination strings [1]. > > We can see that `arg` and `uv_nmi_action` are expected to be > NUL-terminated strings due to their use within `strcmp()` and format > strings respectively. > > With this in mind, 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` or `strcpy`! > > In this case, we can drop both the forced NUL-termination and the `... -1` from: > | strncpy(arg, val, ACTION_LEN - 1); > as `strscpy` implicitly has this behavior. > > Link: 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 > Signed-off-by: Justin Stitt <justinstitt@google.com> > --- > Changes in v3: > - Use sizeof instead of strlen (thanks Andy and Dimitri) > - Drop unrelated changes regarding strnchrnul (thanks Hans) > - Link to v2: https://lore.kernel.org/r/20230824-strncpy-arch-x86-platform-uv-uv_nmi-v2-1-e16d9a3ec570@google.com Thanks, patch looks good to me: Reviewed-by: Hans de Goede <hdegoede@redhat.com> Regards, Hans > > Changes in v2: > - use `sizeof` on destination string instead of `strlen` (thanks Andy, Kees and Dimitri) > - refactor code to remove potential new-line chars (thanks Yang Yang and Andy) > - Link to v1: https://lore.kernel.org/r/20230822-strncpy-arch-x86-platform-uv-uv_nmi-v1-1-931f2943de0d@google.com > --- > Note: build-tested only > --- > arch/x86/platform/uv/uv_nmi.c | 7 +++---- > 1 file changed, 3 insertions(+), 4 deletions(-) > > diff --git a/arch/x86/platform/uv/uv_nmi.c b/arch/x86/platform/uv/uv_nmi.c > index a60af0230e27..dd30fb2baf6c 100644 > --- a/arch/x86/platform/uv/uv_nmi.c > +++ b/arch/x86/platform/uv/uv_nmi.c > @@ -205,8 +205,7 @@ static int param_set_action(const char *val, const struct kernel_param *kp) > char arg[ACTION_LEN], *p; > > /* (remove possible '\n') */ > - strncpy(arg, val, ACTION_LEN - 1); > - arg[ACTION_LEN - 1] = '\0'; > + strscpy(arg, val, sizeof(arg)); > p = strchr(arg, '\n'); > if (p) > *p = '\0'; > @@ -216,7 +215,7 @@ static int param_set_action(const char *val, const struct kernel_param *kp) > break; > > if (i < n) { > - strcpy(uv_nmi_action, arg); > + strscpy(uv_nmi_action, arg, sizeof(uv_nmi_action)); > pr_info("UV: New NMI action:%s\n", uv_nmi_action); > return 0; > } > @@ -959,7 +958,7 @@ static int uv_handle_nmi(unsigned int reason, struct pt_regs *regs) > > /* Unexpected return, revert action to "dump" */ > if (master) > - strncpy(uv_nmi_action, "dump", strlen(uv_nmi_action)); > + strscpy(uv_nmi_action, "dump", sizeof(uv_nmi_action)); > } > > /* Pause as all CPU's enter the NMI handler */ > > --- > base-commit: 2dde18cd1d8fac735875f2e4987f11817cc0bc2c > change-id: 20230822-strncpy-arch-x86-platform-uv-uv_nmi-474e5295c2c1 > > Best regards, > -- > Justin Stitt <justinstitt@google.com> >
* Justin Stitt <justinstitt@google.com> wrote: > Both `strncpy` and `strcpy` are deprecated for use on NUL-terminated > destination strings [1]. > > We can see that `arg` and `uv_nmi_action` are expected to be > NUL-terminated strings due to their use within `strcmp()` and format > strings respectively. > > With this in mind, 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` or `strcpy`! > > In this case, we can drop both the forced NUL-termination and the `... -1` from: > | strncpy(arg, val, ACTION_LEN - 1); > as `strscpy` implicitly has this behavior. > > Link: 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 > Signed-off-by: Justin Stitt <justinstitt@google.com> > arch/x86/platform/uv/uv_nmi.c | 7 +++---- > 1 file changed, 3 insertions(+), 4 deletions(-) Note that this commit is already upstream: 1e6f01f72855 ("x86/platform/uv: Refactor code using deprecated strcpy()/strncpy() interfaces to use strscpy()") Below is the delta your v3 patch has compared to what is upstream - is it really necessary to open code it, instead of using strnchrnul() as your original patch did? Am I missing anything here? Thanks, Ingo --- a/arch/x86/platform/uv/uv_nmi.c +++ b/arch/x86/platform/uv/uv_nmi.c @@ -202,10 +202,13 @@ static int param_set_action(const char *val, const struct kernel_param *kp) { int i; int n = ARRAY_SIZE(valid_acts); - char arg[ACTION_LEN]; + char arg[ACTION_LEN], *p; /* (remove possible '\n') */ - strscpy(arg, val, strnchrnul(val, sizeof(arg)-1, '\n') - val + 1); + strscpy(arg, val, sizeof(arg)); + p = strchr(arg, '\n'); + if (p) + *p = '\0'; for (i = 0; i < n; i++) if (!strcmp(arg, valid_acts[i].action))
Hi Ingo, On 9/6/23 14:10, Ingo Molnar wrote: > > * Justin Stitt <justinstitt@google.com> wrote: > >> Both `strncpy` and `strcpy` are deprecated for use on NUL-terminated >> destination strings [1]. >> >> We can see that `arg` and `uv_nmi_action` are expected to be >> NUL-terminated strings due to their use within `strcmp()` and format >> strings respectively. >> >> With this in mind, 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` or `strcpy`! >> >> In this case, we can drop both the forced NUL-termination and the `... -1` from: >> | strncpy(arg, val, ACTION_LEN - 1); >> as `strscpy` implicitly has this behavior. >> >> Link: 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 >> Signed-off-by: Justin Stitt <justinstitt@google.com> > >> arch/x86/platform/uv/uv_nmi.c | 7 +++---- >> 1 file changed, 3 insertions(+), 4 deletions(-) > > Note that this commit is already upstream: > > 1e6f01f72855 ("x86/platform/uv: Refactor code using deprecated strcpy()/strncpy() interfaces to use strscpy()") > > Below is the delta your v3 patch has compared to what is upstream - is it > really necessary to open code it, instead of using strnchrnul() as your > original patch did? Am I missing anything here? The new version is a result of a review from my because IMHO: strscpy(arg, val, strnchrnul(val, sizeof(arg)-1, '\n') - val + 1); Is really unreadable / really hard to reason about if this is actually correct and does not contain any of by 1 bugs. Note that the diff of v3 compared to the code before v2 landed is actually smaller now and actually matches the subject of: "refactor deprecated strcpy and strncpy" Where as v2 actually touches more code / refactor things which fall outside of a "one change per patch" approach. The: p = strchr(arg, '\n'); if (p) *p = '\0'; was already there before v2 landed. I also suggested to do a follow up patch to change things to: strscpy(arg, val, sizeof(arg)); p = strchrnul(arg, '\n'); *p = '\0'; Which IMHO is much more readable then what has landed now. But since v2 has already landed I guess the best thing is just to stick with what we have upstream now... Regards, Hans > > Thanks, > > Ingo > > --- a/arch/x86/platform/uv/uv_nmi.c > +++ b/arch/x86/platform/uv/uv_nmi.c > @@ -202,10 +202,13 @@ static int param_set_action(const char *val, const struct kernel_param *kp) > { > int i; > int n = ARRAY_SIZE(valid_acts); > - char arg[ACTION_LEN]; > + char arg[ACTION_LEN], *p; > > /* (remove possible '\n') */ > - strscpy(arg, val, strnchrnul(val, sizeof(arg)-1, '\n') - val + 1); > + strscpy(arg, val, sizeof(arg)); > + p = strchr(arg, '\n'); > + if (p) > + *p = '\0'; > > for (i = 0; i < n; i++) > if (!strcmp(arg, valid_acts[i].action)) >
On Wed, Sep 6, 2023 at 3:16 PM Hans de Goede <hdegoede@redhat.com> wrote: > On 9/6/23 14:10, Ingo Molnar wrote: > strscpy(arg, val, strnchrnul(val, sizeof(arg)-1, '\n') - val + 1); If you want to make it short and more readable, you can use strscpy(arg, val, sizeof(arg)); strreplace(arg, '\n', '\0');
* Hans de Goede <hdegoede@redhat.com> wrote: > Hi Ingo, > > On 9/6/23 14:10, Ingo Molnar wrote: > > > > * Justin Stitt <justinstitt@google.com> wrote: > > > >> Both `strncpy` and `strcpy` are deprecated for use on NUL-terminated > >> destination strings [1]. > >> > >> We can see that `arg` and `uv_nmi_action` are expected to be > >> NUL-terminated strings due to their use within `strcmp()` and format > >> strings respectively. > >> > >> With this in mind, 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` or `strcpy`! > >> > >> In this case, we can drop both the forced NUL-termination and the `... -1` from: > >> | strncpy(arg, val, ACTION_LEN - 1); > >> as `strscpy` implicitly has this behavior. > >> > >> Link: 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 > >> Signed-off-by: Justin Stitt <justinstitt@google.com> > > > >> arch/x86/platform/uv/uv_nmi.c | 7 +++---- > >> 1 file changed, 3 insertions(+), 4 deletions(-) > > > > Note that this commit is already upstream: > > > > 1e6f01f72855 ("x86/platform/uv: Refactor code using deprecated strcpy()/strncpy() interfaces to use strscpy()") > > > > Below is the delta your v3 patch has compared to what is upstream - is it > > really necessary to open code it, instead of using strnchrnul() as your > > original patch did? Am I missing anything here? > > The new version is a result of a review from my because IMHO: > > strscpy(arg, val, strnchrnul(val, sizeof(arg)-1, '\n') - val + 1); > > Is really unreadable / really hard to reason about if > this is actually correct and does not contain any > of by 1 bugs. > > Note that the diff of v3 compared to the code before v2 landed is > actually smaller now and actually matches the subject of: > "refactor deprecated strcpy and strncpy" > > Where as v2 actually touches more code / refactor things > which fall outside of a "one change per patch" approach. > The: > > p = strchr(arg, '\n'); > if (p) > *p = '\0'; > > was already there before v2 landed. > > I also suggested to do a follow up patch to change things to: > > strscpy(arg, val, sizeof(arg)); > p = strchrnul(arg, '\n'); > *p = '\0'; > > Which IMHO is much more readable then what has landed > now. But since v2 has already landed I guess the best > thing is just to stick with what we have upstream now... Well, how about we do a delta patch with all the changes you suggested? I'm all for readability. Thanks, Ingo
On Wed, Sep 06, 2023 at 04:09:01PM +0200, Ingo Molnar wrote: > > * Hans de Goede <hdegoede@redhat.com> wrote: > > > Hi Ingo, > > > > On 9/6/23 14:10, Ingo Molnar wrote: > > > > > > * Justin Stitt <justinstitt@google.com> wrote: > > > > > >> Both `strncpy` and `strcpy` are deprecated for use on NUL-terminated > > >> destination strings [1]. > > >> > > >> We can see that `arg` and `uv_nmi_action` are expected to be > > >> NUL-terminated strings due to their use within `strcmp()` and format > > >> strings respectively. > > >> > > >> With this in mind, 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` or `strcpy`! > > >> > > >> In this case, we can drop both the forced NUL-termination and the `... -1` from: > > >> | strncpy(arg, val, ACTION_LEN - 1); > > >> as `strscpy` implicitly has this behavior. > > >> > > >> Link: 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 > > >> Signed-off-by: Justin Stitt <justinstitt@google.com> > > > > > >> arch/x86/platform/uv/uv_nmi.c | 7 +++---- > > >> 1 file changed, 3 insertions(+), 4 deletions(-) > > > > > > Note that this commit is already upstream: > > > > > > 1e6f01f72855 ("x86/platform/uv: Refactor code using deprecated strcpy()/strncpy() interfaces to use strscpy()") > > > > > > Below is the delta your v3 patch has compared to what is upstream - is it > > > really necessary to open code it, instead of using strnchrnul() as your > > > original patch did? Am I missing anything here? > > > > The new version is a result of a review from my because IMHO: > > > > strscpy(arg, val, strnchrnul(val, sizeof(arg)-1, '\n') - val + 1); > > > > Is really unreadable / really hard to reason about if > > this is actually correct and does not contain any > > of by 1 bugs. > > > > Note that the diff of v3 compared to the code before v2 landed is > > actually smaller now and actually matches the subject of: > > "refactor deprecated strcpy and strncpy" > > > > Where as v2 actually touches more code / refactor things > > which fall outside of a "one change per patch" approach. > > The: > > > > p = strchr(arg, '\n'); > > if (p) > > *p = '\0'; > > > > was already there before v2 landed. > > > > I also suggested to do a follow up patch to change things to: > > > > strscpy(arg, val, sizeof(arg)); > > p = strchrnul(arg, '\n'); > > *p = '\0'; > > > > Which IMHO is much more readable then what has landed > > now. But since v2 has already landed I guess the best > > thing is just to stick with what we have upstream now... > > Well, how about we do a delta patch with all the changes > you suggested? I'm all for readability. For whatever it's worth, I vote in favor of adopting an increased readability version. I was on vacation when the patch came through, and by the time I reviewed it it was already accepted. I still puzzled through the -1/+1 stuff to be sure it functioned correctly; since it worked and was already accepted, I let it go. When Hans' comments on readability later came through, I was thinking "Yes, he's exactly right! Why, when I worked so hard on verifying that the code worked properly, did it not occur to me to suggest re-writing this in a simpler fashion to make the intent clear?" Thanks, --> Steve Wahl
Hi, On 9/6/23 16:09, Ingo Molnar wrote: > > * Hans de Goede <hdegoede@redhat.com> wrote: > >> Hi Ingo, >> >> On 9/6/23 14:10, Ingo Molnar wrote: >>> >>> * Justin Stitt <justinstitt@google.com> wrote: >>> >>>> Both `strncpy` and `strcpy` are deprecated for use on NUL-terminated >>>> destination strings [1]. >>>> >>>> We can see that `arg` and `uv_nmi_action` are expected to be >>>> NUL-terminated strings due to their use within `strcmp()` and format >>>> strings respectively. >>>> >>>> With this in mind, 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` or `strcpy`! >>>> >>>> In this case, we can drop both the forced NUL-termination and the `... -1` from: >>>> | strncpy(arg, val, ACTION_LEN - 1); >>>> as `strscpy` implicitly has this behavior. >>>> >>>> Link: 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 >>>> Signed-off-by: Justin Stitt <justinstitt@google.com> >>> >>>> arch/x86/platform/uv/uv_nmi.c | 7 +++---- >>>> 1 file changed, 3 insertions(+), 4 deletions(-) >>> >>> Note that this commit is already upstream: >>> >>> 1e6f01f72855 ("x86/platform/uv: Refactor code using deprecated strcpy()/strncpy() interfaces to use strscpy()") >>> >>> Below is the delta your v3 patch has compared to what is upstream - is it >>> really necessary to open code it, instead of using strnchrnul() as your >>> original patch did? Am I missing anything here? >> >> The new version is a result of a review from my because IMHO: >> >> strscpy(arg, val, strnchrnul(val, sizeof(arg)-1, '\n') - val + 1); >> >> Is really unreadable / really hard to reason about if >> this is actually correct and does not contain any >> of by 1 bugs. >> >> Note that the diff of v3 compared to the code before v2 landed is >> actually smaller now and actually matches the subject of: >> "refactor deprecated strcpy and strncpy" >> >> Where as v2 actually touches more code / refactor things >> which fall outside of a "one change per patch" approach. >> The: >> >> p = strchr(arg, '\n'); >> if (p) >> *p = '\0'; >> >> was already there before v2 landed. >> >> I also suggested to do a follow up patch to change things to: >> >> strscpy(arg, val, sizeof(arg)); >> p = strchrnul(arg, '\n'); >> *p = '\0'; >> >> Which IMHO is much more readable then what has landed >> now. But since v2 has already landed I guess the best >> thing is just to stick with what we have upstream now... > > Well, how about we do a delta patch with all the changes > you suggested? I'm all for readability. So I started doing this and notices that all the string manipulation + parsing done here is really just a DYI implementation of sysfs_match_string(). So I have prepared a patch to switch to sysfs_match_string(), which completely removes the need to make a copy of the val string. I'll submit the patch right after this email. Regards, Hans
* Hans de Goede <hdegoede@redhat.com> wrote: > >> Which IMHO is much more readable then what has landed now. But since > >> v2 has already landed I guess the best thing is just to stick with > >> what we have upstream now... > > > > Well, how about we do a delta patch with all the changes you suggested? > > I'm all for readability. > > So I started doing this and notices that all the string manipulation + > parsing done here is really just a DYI implementation of > sysfs_match_string(). > > So I have prepared a patch to switch to sysfs_match_string(), which > completely removes the need to make a copy of the val string. > > I'll submit the patch right after this email. Thank you - that looks a far more thorough cleanup indeed. Ingo
diff --git a/arch/x86/platform/uv/uv_nmi.c b/arch/x86/platform/uv/uv_nmi.c index a60af0230e27..dd30fb2baf6c 100644 --- a/arch/x86/platform/uv/uv_nmi.c +++ b/arch/x86/platform/uv/uv_nmi.c @@ -205,8 +205,7 @@ static int param_set_action(const char *val, const struct kernel_param *kp) char arg[ACTION_LEN], *p; /* (remove possible '\n') */ - strncpy(arg, val, ACTION_LEN - 1); - arg[ACTION_LEN - 1] = '\0'; + strscpy(arg, val, sizeof(arg)); p = strchr(arg, '\n'); if (p) *p = '\0'; @@ -216,7 +215,7 @@ static int param_set_action(const char *val, const struct kernel_param *kp) break; if (i < n) { - strcpy(uv_nmi_action, arg); + strscpy(uv_nmi_action, arg, sizeof(uv_nmi_action)); pr_info("UV: New NMI action:%s\n", uv_nmi_action); return 0; } @@ -959,7 +958,7 @@ static int uv_handle_nmi(unsigned int reason, struct pt_regs *regs) /* Unexpected return, revert action to "dump" */ if (master) - strncpy(uv_nmi_action, "dump", strlen(uv_nmi_action)); + strscpy(uv_nmi_action, "dump", sizeof(uv_nmi_action)); } /* Pause as all CPU's enter the NMI handler */
Both `strncpy` and `strcpy` are deprecated for use on NUL-terminated destination strings [1]. We can see that `arg` and `uv_nmi_action` are expected to be NUL-terminated strings due to their use within `strcmp()` and format strings respectively. With this in mind, 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` or `strcpy`! In this case, we can drop both the forced NUL-termination and the `... -1` from: | strncpy(arg, val, ACTION_LEN - 1); as `strscpy` implicitly has this behavior. Link: 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 Signed-off-by: Justin Stitt <justinstitt@google.com> --- Changes in v3: - Use sizeof instead of strlen (thanks Andy and Dimitri) - Drop unrelated changes regarding strnchrnul (thanks Hans) - Link to v2: https://lore.kernel.org/r/20230824-strncpy-arch-x86-platform-uv-uv_nmi-v2-1-e16d9a3ec570@google.com Changes in v2: - use `sizeof` on destination string instead of `strlen` (thanks Andy, Kees and Dimitri) - refactor code to remove potential new-line chars (thanks Yang Yang and Andy) - Link to v1: https://lore.kernel.org/r/20230822-strncpy-arch-x86-platform-uv-uv_nmi-v1-1-931f2943de0d@google.com --- Note: build-tested only --- arch/x86/platform/uv/uv_nmi.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) --- base-commit: 2dde18cd1d8fac735875f2e4987f11817cc0bc2c change-id: 20230822-strncpy-arch-x86-platform-uv-uv_nmi-474e5295c2c1 Best regards, -- Justin Stitt <justinstitt@google.com>