Messages in this thread Patch in this message | | | Date | Wed, 6 Sep 2023 14:16:30 +0200 | Subject | Re: [PATCH v3] x86/platform/uv: refactor deprecated strcpy and strncpy | From | Hans de Goede <> |
| |
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)) >
| |