Hi Ingo, On 9/6/23 14:10, Ingo Molnar wrote: > > * Justin Stitt <justinstitt@xxxxxxxxxx> 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@xxxxxxxxxxxxxxx >> Signed-off-by: Justin Stitt <justinstitt@xxxxxxxxxx> > >> 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)) >
