| Message ID | 20230710030625.812707-1-azeemshaikh38@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | wifi: mwifiex: Replace strlcpy with strscpy | expand |
On Mon, Jul 10, 2023 at 03:06:25AM +0000, Azeem Shaikh wrote: > strlcpy() reads the entire source buffer first. > This read may exceed the destination size limit. > This is both inefficient and can lead to linear read > overflows if a source string is not NUL-terminated [1]. > In an effort to remove strlcpy() completely [2], replace > strlcpy() here with strscpy(). > > Direct replacement is safe here since return value of -errno > is used to check for truncation instead of sizeof(dest). > > [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy > [2] https://github.com/KSPP/linux/issues/89 > > Signed-off-by: Azeem Shaikh <azeemshaikh38@gmail.com> Reviewed-by: Kees Cook <keescook@chromium.org>
On Mon, 10 Jul 2023 03:06:25 +0000, Azeem Shaikh wrote: > strlcpy() reads the entire source buffer first. > This read may exceed the destination size limit. > This is both inefficient and can lead to linear read > overflows if a source string is not NUL-terminated [1]. > In an effort to remove strlcpy() completely [2], replace > strlcpy() here with strscpy(). > > [...] Applied, thanks! [1/1] wifi: mwifiex: Replace strlcpy with strscpy https://git.kernel.org/kees/c/5469fb73e96d Best regards,
Kees Cook <keescook@chromium.org> writes: > On Mon, 10 Jul 2023 03:06:25 +0000, Azeem Shaikh wrote: >> strlcpy() reads the entire source buffer first. >> This read may exceed the destination size limit. >> This is both inefficient and can lead to linear read >> overflows if a source string is not NUL-terminated [1]. >> In an effort to remove strlcpy() completely [2], replace >> strlcpy() here with strscpy(). >> >> [...] > > Applied, thanks! > > [1/1] wifi: mwifiex: Replace strlcpy with strscpy > https://git.kernel.org/kees/c/5469fb73e96d And the same question here, why are you taking wifi patches without acks? And this already fixed differently in wireless-next so our trees conflict now: https://git.kernel.org/pub/scm/linux/kernel/git/wireless/wireless-next.git/commit/?id=caf9ead2c7d06fd7aa4cb48bd569ad61db9a0b4a
On Thu, Jul 27, 2023 at 07:02:31PM +0300, Kalle Valo wrote: > Kees Cook <keescook@chromium.org> writes: > > > On Mon, 10 Jul 2023 03:06:25 +0000, Azeem Shaikh wrote: > >> strlcpy() reads the entire source buffer first. > >> This read may exceed the destination size limit. > >> This is both inefficient and can lead to linear read > >> overflows if a source string is not NUL-terminated [1]. > >> In an effort to remove strlcpy() completely [2], replace > >> strlcpy() here with strscpy(). > >> > >> [...] > > > > Applied, thanks! > > > > [1/1] wifi: mwifiex: Replace strlcpy with strscpy > > https://git.kernel.org/kees/c/5469fb73e96d > > And the same question here, why are you taking wifi patches without > acks? And this already fixed differently in wireless-next so our trees > conflict now: > > https://git.kernel.org/pub/scm/linux/kernel/git/wireless/wireless-next.git/commit/?id=caf9ead2c7d06fd7aa4cb48bd569ad61db9a0b4a Thanks for pointing that out! I saw no feedback on Azeem's patch, so it looked like it was being ignored. For the patch you linked to -- it's okay to have lost the overflow detection and warning? Regardless, I will drop this from my tree. -Kees
On Thu, Jul 27, 2023 at 10:04 AM Kees Cook <keescook@chromium.org> wrote: > > On Thu, Jul 27, 2023 at 07:02:31PM +0300, Kalle Valo wrote: > > https://git.kernel.org/pub/scm/linux/kernel/git/wireless/wireless-next.git/commit/?id=caf9ead2c7d06fd7aa4cb48bd569ad61db9a0b4a > > For the patch you linked to -- it's okay to have lost the overflow > detection and warning? It's a fixed constant string being copied into a larger fixed array. It really doesn't matter functionality-wise. At best, the error check would be useful for human readers or automated tools. So I didn't bother complaining one way or another, as it took enough tries for the submitter to get things right in the first place. Brian
On Thu, Jul 27, 2023 at 10:30:29AM -0700, Brian Norris wrote: > On Thu, Jul 27, 2023 at 10:04 AM Kees Cook <keescook@chromium.org> wrote: > > > > On Thu, Jul 27, 2023 at 07:02:31PM +0300, Kalle Valo wrote: > > > https://git.kernel.org/pub/scm/linux/kernel/git/wireless/wireless-next.git/commit/?id=caf9ead2c7d06fd7aa4cb48bd569ad61db9a0b4a > > > > For the patch you linked to -- it's okay to have lost the overflow > > detection and warning? > > It's a fixed constant string being copied into a larger fixed array. > It really doesn't matter functionality-wise. At best, the error check > would be useful for human readers or automated tools. So I didn't > bother complaining one way or another, as it took enough tries for the > submitter to get things right in the first place. Gotcha. Thanks!
diff --git a/drivers/net/wireless/marvell/mwifiex/main.c b/drivers/net/wireless/marvell/mwifiex/main.c index 1cd9d20cca16..8d3c4bcf9c89 100644 --- a/drivers/net/wireless/marvell/mwifiex/main.c +++ b/drivers/net/wireless/marvell/mwifiex/main.c @@ -725,9 +725,8 @@ static int mwifiex_init_hw_fw(struct mwifiex_adapter *adapter, * manufacturing mode is enabled */ if (mfg_mode) { - if (strlcpy(adapter->fw_name, MFG_FIRMWARE, - sizeof(adapter->fw_name)) >= - sizeof(adapter->fw_name)) { + if (strscpy(adapter->fw_name, MFG_FIRMWARE, + sizeof(adapter->fw_name)) < 0) { pr_err("%s: fw_name too long!\n", __func__); return -1; }
strlcpy() reads the entire source buffer first. This read may exceed the destination size limit. This is both inefficient and can lead to linear read overflows if a source string is not NUL-terminated [1]. In an effort to remove strlcpy() completely [2], replace strlcpy() here with strscpy(). Direct replacement is safe here since return value of -errno is used to check for truncation instead of sizeof(dest). [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy [2] https://github.com/KSPP/linux/issues/89 Signed-off-by: Azeem Shaikh <azeemshaikh38@gmail.com> --- drivers/net/wireless/marvell/mwifiex/main.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)