| Message ID | 20230911-strncpy-arch-um-os-linux-drivers-ethertap_user-c-v1-1-d9e53f52ab32@google.com (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | e0bbf92682ad1df36ef43104a036469ac0ab3a4a |
| Headers | show |
| Series | um,ethertap: refactor deprecated strncpy | expand |
Hi Justin, Thanks for your patch! On Mon, Sep 11, 2023 at 7:53 PM Justin Stitt <justinstitt@google.com> wrote: > `strncpy` is deprecated for use on NUL-terminated destination strings [1]. > > `gate_buf` should always be NUL-terminated and does not require > NUL-padding. It is used as a string arg inside an argv array given to Can you please explain why it does not require NUL-padding? It looks like this buffer is passed eventually to a user space application, thus possibly leaking uninitialized stack data. > `run_helper()`. Due to this, let's use `strscpy` as it guarantees > NUL-terminated on the destination buffer preventing potential buffer > overreads [2]. > > This exact invocation was changed from `strcpy` to `strncpy` in commit > 7879b1d94badb ("um,ethertap: use strncpy") back in 2015. Let's continue > hardening our `str*cpy` apis and use the newer and safer `strscpy`! > > 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 > Cc: Kees Cook <keescook@chromium.org> > Signed-off-by: Justin Stitt <justinstitt@google.com> > --- > arch/um/os-Linux/drivers/ethertap_user.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/um/os-Linux/drivers/ethertap_user.c b/arch/um/os-Linux/drivers/ethertap_user.c > index 9483021d86dd..3363851a4ae8 100644 > --- a/arch/um/os-Linux/drivers/ethertap_user.c > +++ b/arch/um/os-Linux/drivers/ethertap_user.c > @@ -105,7 +105,7 @@ static int etap_tramp(char *dev, char *gate, int control_me, > sprintf(data_fd_buf, "%d", data_remote); > sprintf(version_buf, "%d", UML_NET_VERSION); > if (gate != NULL) { > - strncpy(gate_buf, gate, 15); > + strscpy(gate_buf, gate, sizeof(gate_buf)); > args = setup_args; > } > else args = nosetup_args; > > --- > base-commit: 2dde18cd1d8fac735875f2e4987f11817cc0bc2c > change-id: 20230911-strncpy-arch-um-os-linux-drivers-ethertap_user-c-859160d13f59 Gr{oetje,eeting}s, Geert
On Tue, Sep 12, 2023 at 12:36 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > Hi Justin, > > Thanks for your patch! > > On Mon, Sep 11, 2023 at 7:53 PM Justin Stitt <justinstitt@google.com> wrote: > > `strncpy` is deprecated for use on NUL-terminated destination strings [1]. > > > > `gate_buf` should always be NUL-terminated and does not require > > NUL-padding. It is used as a string arg inside an argv array given to > > Can you please explain why it does not require NUL-padding? > It looks like this buffer is passed eventually to a user space > application, thus possibly leaking uninitialized stack data. It looks like it's being passed as a list of command-line arguments in `run_helper()`. Should this be NUL-padded due to its eventual use in user space? If we think yes I can send a v2. Thanks for pointing this out. > > > `run_helper()`. Due to this, let's use `strscpy` as it guarantees > > NUL-terminated on the destination buffer preventing potential buffer > > overreads [2]. > > > > This exact invocation was changed from `strcpy` to `strncpy` in commit > > 7879b1d94badb ("um,ethertap: use strncpy") back in 2015. Let's continue > > hardening our `str*cpy` apis and use the newer and safer `strscpy`! > > > > 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 > > Cc: Kees Cook <keescook@chromium.org> > > Signed-off-by: Justin Stitt <justinstitt@google.com> > > --- > > arch/um/os-Linux/drivers/ethertap_user.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/arch/um/os-Linux/drivers/ethertap_user.c b/arch/um/os-Linux/drivers/ethertap_user.c > > index 9483021d86dd..3363851a4ae8 100644 > > --- a/arch/um/os-Linux/drivers/ethertap_user.c > > +++ b/arch/um/os-Linux/drivers/ethertap_user.c > > @@ -105,7 +105,7 @@ static int etap_tramp(char *dev, char *gate, int control_me, > > sprintf(data_fd_buf, "%d", data_remote); > > sprintf(version_buf, "%d", UML_NET_VERSION); > > if (gate != NULL) { > > - strncpy(gate_buf, gate, 15); > > + strscpy(gate_buf, gate, sizeof(gate_buf)); > > args = setup_args; > > } > > else args = nosetup_args; > > > > --- > > base-commit: 2dde18cd1d8fac735875f2e4987f11817cc0bc2c > > change-id: 20230911-strncpy-arch-um-os-linux-drivers-ethertap_user-c-859160d13f59 > > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like that. > -- Linus Torvalds
On Tue, Sep 12, 2023 at 01:12:35PM -0700, Justin Stitt wrote: > On Tue, Sep 12, 2023 at 12:36 AM Geert Uytterhoeven > <geert@linux-m68k.org> wrote: > > > > Hi Justin, > > > > Thanks for your patch! > > > > On Mon, Sep 11, 2023 at 7:53 PM Justin Stitt <justinstitt@google.com> wrote: > > > `strncpy` is deprecated for use on NUL-terminated destination strings [1]. > > > > > > `gate_buf` should always be NUL-terminated and does not require > > > NUL-padding. It is used as a string arg inside an argv array given to > > > > Can you please explain why it does not require NUL-padding? > > It looks like this buffer is passed eventually to a user space > > application, thus possibly leaking uninitialized stack data. > > It looks like it's being passed as a list of command-line arguments in > `run_helper()`. Should this be NUL-padded due to its eventual use in > user space? If we think yes I can send a v2. Thanks for pointing this > out. No, it's passed as a pointer to a string, and the clone call will ultimately make a copy-until-%NUL when building the new process. This doesn't need padding. Reviewed-by: Kees Cook <keescook@chromium.org> -Kees
On Mon, 11 Sep 2023 17:52:44 +0000, Justin Stitt wrote: > `strncpy` is deprecated for use on NUL-terminated destination strings [1]. > > `gate_buf` should always be NUL-terminated and does not require > NUL-padding. It is used as a string arg inside an argv array given to > `run_helper()`. Due to this, let's use `strscpy` as it guarantees > NUL-terminated on the destination buffer preventing potential buffer > overreads [2]. > > [...] Applied to for-next/hardening, thanks! [1/1] um,ethertap: refactor deprecated strncpy https://git.kernel.org/kees/c/d4e178fe19c9 Take care,
diff --git a/arch/um/os-Linux/drivers/ethertap_user.c b/arch/um/os-Linux/drivers/ethertap_user.c index 9483021d86dd..3363851a4ae8 100644 --- a/arch/um/os-Linux/drivers/ethertap_user.c +++ b/arch/um/os-Linux/drivers/ethertap_user.c @@ -105,7 +105,7 @@ static int etap_tramp(char *dev, char *gate, int control_me, sprintf(data_fd_buf, "%d", data_remote); sprintf(version_buf, "%d", UML_NET_VERSION); if (gate != NULL) { - strncpy(gate_buf, gate, 15); + strscpy(gate_buf, gate, sizeof(gate_buf)); args = setup_args; } else args = nosetup_args;
`strncpy` is deprecated for use on NUL-terminated destination strings [1]. `gate_buf` should always be NUL-terminated and does not require NUL-padding. It is used as a string arg inside an argv array given to `run_helper()`. Due to this, let's use `strscpy` as it guarantees NUL-terminated on the destination buffer preventing potential buffer overreads [2]. This exact invocation was changed from `strcpy` to `strncpy` in commit 7879b1d94badb ("um,ethertap: use strncpy") back in 2015. Let's continue hardening our `str*cpy` apis and use the newer and safer `strscpy`! 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 Cc: Kees Cook <keescook@chromium.org> Signed-off-by: Justin Stitt <justinstitt@google.com> --- arch/um/os-Linux/drivers/ethertap_user.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) --- base-commit: 2dde18cd1d8fac735875f2e4987f11817cc0bc2c change-id: 20230911-strncpy-arch-um-os-linux-drivers-ethertap_user-c-859160d13f59 Best regards, -- Justin Stitt <justinstitt@google.com>