| Message ID | 20230703181256.3712079-1-azeemshaikh38@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | wifi: mt76: Replace strlcpy with strscpy | expand |
On Mon, Jul 03, 2023 at 06:12:56PM +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 DEV_ASSIGN is only used by > TRACE macros and the return values are ignored. > > [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> Looks good -- thing is using return values from the macros. Reviewed-by: Kees Cook <keescook@chromium.org>
On Wed, Jul 12, 2023 at 7:54 PM Kees Cook <keescook@chromium.org> wrote: > > On Mon, Jul 03, 2023 at 06:12:56PM +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 DEV_ASSIGN is only used by > > TRACE macros and the return values are ignored. > > > > [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> > > Looks good -- thing is using return values from the macros. Just to confirm, you mean *not* using return values from the macros?
On Mon, 03 Jul 2023 18:12:56 +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: mt76: Replace strlcpy with strscpy https://git.kernel.org/kees/c/535c78cbc0c4 Best regards,
Kees Cook <keescook@chromium.org> writes: > On Mon, 03 Jul 2023 18:12:56 +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: mt76: Replace strlcpy with strscpy > https://git.kernel.org/kees/c/535c78cbc0c4 Why did you take this? mt76 is in active development so risk of conflicts is high.
On Thu, Jul 27, 2023 at 07:00:21PM +0300, Kalle Valo wrote: > Kees Cook <keescook@chromium.org> writes: > > > On Mon, 03 Jul 2023 18:12:56 +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: mt76: Replace strlcpy with strscpy > > https://git.kernel.org/kees/c/535c78cbc0c4 > > Why did you take this? mt76 is in active development so risk of > conflicts is high. There didn't seem to be any further activity for 3 weeks, and it was a relatively mechanical change. I can drop it from my tree. What's needed for it to be picked up through wireless? -Kees
On Tue, Jul 18, 2023 at 12:38:37AM -0400, Azeem Shaikh wrote: > On Wed, Jul 12, 2023 at 7:54 PM Kees Cook <keescook@chromium.org> wrote: > > > > On Mon, Jul 03, 2023 at 06:12:56PM +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 DEV_ASSIGN is only used by > > > TRACE macros and the return values are ignored. > > > > > > [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> > > > > Looks good -- thing is using return values from the macros. > > Just to confirm, you mean *not* using return values from the macros? I thought I'd replied to this, but I see it didn't happen: yes, I meant "not using return values". Sorry for the confusion!
Kees Cook <keescook@chromium.org> writes: > On Thu, Jul 27, 2023 at 07:00:21PM +0300, Kalle Valo wrote: >> Kees Cook <keescook@chromium.org> writes: >> >> > On Mon, 03 Jul 2023 18:12:56 +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: mt76: Replace strlcpy with strscpy >> > https://git.kernel.org/kees/c/535c78cbc0c4 >> >> Why did you take this? mt76 is in active development so risk of >> conflicts is high. > > There didn't seem to be any further activity for 3 weeks, and it was a > relatively mechanical change. That's because the wireless trees were on a summer break: https://lore.kernel.org/all/87y1kncuh4.fsf@kernel.org/ > I can drop it from my tree. Yes, please drop this. And in the future don't take any wireless patches unless acked by Johannes or me, I want to minimize the risk of conflicts between the trees. If a patch is missed for whatever reason please let me know, do not take it to your tree. > What's needed for it to be picked up through wireless? I don't know why Felix didn't take this patch but now I assigned it to me on patchwork: https://patchwork.kernel.org/project/linux-wireless/patch/20230703181256.3712079-1-azeemshaikh38@gmail.com/ It should be in wireless-next this week.
Azeem Shaikh <azeemshaikh38@gmail.com> 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 DEV_ASSIGN is only used by > TRACE macros and the return values are ignored. > > [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> Patch applied to wireless-next.git, thanks. d6b484b5cb2a wifi: mt76: Replace strlcpy() with strscpy()
diff --git a/drivers/net/wireless/mediatek/mt76/mt7615/mt7615_trace.h b/drivers/net/wireless/mediatek/mt76/mt7615/mt7615_trace.h index d3eb49d83b98..9be5a58a4e6d 100644 --- a/drivers/net/wireless/mediatek/mt76/mt7615/mt7615_trace.h +++ b/drivers/net/wireless/mediatek/mt76/mt7615/mt7615_trace.h @@ -14,7 +14,7 @@ #define MAXNAME 32 #define DEV_ENTRY __array(char, wiphy_name, 32) -#define DEV_ASSIGN strlcpy(__entry->wiphy_name, \ +#define DEV_ASSIGN strscpy(__entry->wiphy_name, \ wiphy_name(mt76_hw(dev)->wiphy), MAXNAME) #define DEV_PR_FMT "%s" #define DEV_PR_ARG __entry->wiphy_name diff --git a/drivers/net/wireless/mediatek/mt76/mt76x02_trace.h b/drivers/net/wireless/mediatek/mt76/mt76x02_trace.h index 6a98092e996b..11d119cd0f6f 100644 --- a/drivers/net/wireless/mediatek/mt76/mt76x02_trace.h +++ b/drivers/net/wireless/mediatek/mt76/mt76x02_trace.h @@ -14,7 +14,7 @@ #define MAXNAME 32 #define DEV_ENTRY __array(char, wiphy_name, 32) -#define DEV_ASSIGN strlcpy(__entry->wiphy_name, \ +#define DEV_ASSIGN strscpy(__entry->wiphy_name, \ wiphy_name(mt76_hw(dev)->wiphy), MAXNAME) #define DEV_PR_FMT "%s" #define DEV_PR_ARG __entry->wiphy_name diff --git a/drivers/net/wireless/mediatek/mt76/mt7921/mt7921_trace.h b/drivers/net/wireless/mediatek/mt76/mt7921/mt7921_trace.h index 9bc4db67f352..887d5cfddd01 100644 --- a/drivers/net/wireless/mediatek/mt76/mt7921/mt7921_trace.h +++ b/drivers/net/wireless/mediatek/mt76/mt7921/mt7921_trace.h @@ -14,7 +14,7 @@ #define MAXNAME 32 #define DEV_ENTRY __array(char, wiphy_name, 32) -#define DEV_ASSIGN strlcpy(__entry->wiphy_name, \ +#define DEV_ASSIGN strscpy(__entry->wiphy_name, \ wiphy_name(mt76_hw(dev)->wiphy), MAXNAME) #define DEV_PR_FMT "%s" #define DEV_PR_ARG __entry->wiphy_name diff --git a/drivers/net/wireless/mediatek/mt76/trace.h b/drivers/net/wireless/mediatek/mt76/trace.h index c3d0ef8e2890..109a07f9733a 100644 --- a/drivers/net/wireless/mediatek/mt76/trace.h +++ b/drivers/net/wireless/mediatek/mt76/trace.h @@ -14,7 +14,7 @@ #define MAXNAME 32 #define DEV_ENTRY __array(char, wiphy_name, 32) -#define DEVICE_ASSIGN strlcpy(__entry->wiphy_name, \ +#define DEVICE_ASSIGN strscpy(__entry->wiphy_name, \ wiphy_name(dev->hw->wiphy), MAXNAME) #define DEV_PR_FMT "%s" #define DEV_PR_ARG __entry->wiphy_name diff --git a/drivers/net/wireless/mediatek/mt76/usb_trace.h b/drivers/net/wireless/mediatek/mt76/usb_trace.h index f5ab3215af80..7b261ddb2ac6 100644 --- a/drivers/net/wireless/mediatek/mt76/usb_trace.h +++ b/drivers/net/wireless/mediatek/mt76/usb_trace.h @@ -14,7 +14,7 @@ #define MAXNAME 32 #define DEV_ENTRY __array(char, wiphy_name, 32) -#define DEV_ASSIGN strlcpy(__entry->wiphy_name, \ +#define DEV_ASSIGN strscpy(__entry->wiphy_name, \ wiphy_name(dev->hw->wiphy), MAXNAME) #define DEV_PR_FMT "%s " #define DEV_PR_ARG __entry->wiphy_name
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 DEV_ASSIGN is only used by TRACE macros and the return values are ignored. [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/mediatek/mt76/mt7615/mt7615_trace.h | 2 +- drivers/net/wireless/mediatek/mt76/mt76x02_trace.h | 2 +- drivers/net/wireless/mediatek/mt76/mt7921/mt7921_trace.h | 2 +- drivers/net/wireless/mediatek/mt76/trace.h | 2 +- drivers/net/wireless/mediatek/mt76/usb_trace.h | 2 +- 5 files changed, 5 insertions(+), 5 deletions(-)