| Message ID | 20240702122450.2213833-2-suhui@nfschina.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | wifi: cfg80211: avoid some garbage values | expand |
On Tue, Jul 02, 2024 at 08:24:44PM +0800, Su Hui wrote: > brcmf_fil_cmd_int_get() reads the value of 'io_type' and passes it to > brcmf_fil_cmd_data_get(). Initialize 'io_type' to avoid garbage value. Since you're going to be resending anyway, please delete the space char from the start of the line. It's weird that brcmf_fil_cmd_data_get() uses the uninitialized data. It looks like it just goes to great lengths to preserve the original data in io_type... So it likely is harmless enough but still a strange and complicated way write a no-op. regards, dan carpenter
On July 2, 2024 3:57:27 PM Dan Carpenter <dan.carpenter@linaro.org> wrote: > On Tue, Jul 02, 2024 at 08:24:44PM +0800, Su Hui wrote: >> brcmf_fil_cmd_int_get() reads the value of 'io_type' and passes it to >> brcmf_fil_cmd_data_get(). Initialize 'io_type' to avoid garbage value. > > Since you're going to be resending anyway, please delete the space char > from the start of the line. > > It's weird that brcmf_fil_cmd_data_get() uses the uninitialized data. > It looks like it just goes to great lengths to preserve the original > data in io_type... So it likely is harmless enough but still a strange > and complicated way write a no-op. Not sure if it helps, but I tried to explain the reason in response to patch 0 (cover letter). Regards, Arend
Arend Van Spriel <arend.vanspriel@broadcom.com> writes: > On July 2, 2024 3:57:27 PM Dan Carpenter <dan.carpenter@linaro.org> wrote: > >> On Tue, Jul 02, 2024 at 08:24:44PM +0800, Su Hui wrote: >>> brcmf_fil_cmd_int_get() reads the value of 'io_type' and passes it to >>> brcmf_fil_cmd_data_get(). Initialize 'io_type' to avoid garbage value. >> >> Since you're going to be resending anyway, please delete the space char >> from the start of the line. >> >> It's weird that brcmf_fil_cmd_data_get() uses the uninitialized data. >> It looks like it just goes to great lengths to preserve the original >> data in io_type... So it likely is harmless enough but still a strange >> and complicated way write a no-op. > > Not sure if it helps, but I tried to explain the reason in response to > patch 0 (cover letter). Would it make more sense to have just one patch? It's the same issue anyway.
On Tue, Jul 02, 2024 at 06:29:20PM +0300, Kalle Valo wrote: > Arend Van Spriel <arend.vanspriel@broadcom.com> writes: > > > On July 2, 2024 3:57:27 PM Dan Carpenter <dan.carpenter@linaro.org> wrote: > > > >> On Tue, Jul 02, 2024 at 08:24:44PM +0800, Su Hui wrote: > >>> brcmf_fil_cmd_int_get() reads the value of 'io_type' and passes it to > >>> brcmf_fil_cmd_data_get(). Initialize 'io_type' to avoid garbage value. > >> > >> Since you're going to be resending anyway, please delete the space char > >> from the start of the line. > >> > >> It's weird that brcmf_fil_cmd_data_get() uses the uninitialized data. > >> It looks like it just goes to great lengths to preserve the original > >> data in io_type... So it likely is harmless enough but still a strange > >> and complicated way write a no-op. > > > > Not sure if it helps, but I tried to explain the reason in response to > > patch 0 (cover letter). > > Would it make more sense to have just one patch? It's the same issue > anyway. The Fixes tags are different though. I'd probably leave them as separate patches just because of that. regards, dan carpenter
On July 2, 2024 5:29:27 PM Kalle Valo <kvalo@kernel.org> wrote: > Arend Van Spriel <arend.vanspriel@broadcom.com> writes: > >> On July 2, 2024 3:57:27 PM Dan Carpenter <dan.carpenter@linaro.org> wrote: >> >>> On Tue, Jul 02, 2024 at 08:24:44PM +0800, Su Hui wrote: >>>> brcmf_fil_cmd_int_get() reads the value of 'io_type' and passes it to >>>> brcmf_fil_cmd_data_get(). Initialize 'io_type' to avoid garbage value. >>> >>> Since you're going to be resending anyway, please delete the space char >>> from the start of the line. >>> >>> It's weird that brcmf_fil_cmd_data_get() uses the uninitialized data. >>> It looks like it just goes to great lengths to preserve the original >>> data in io_type... So it likely is harmless enough but still a strange >>> and complicated way write a no-op. >> >> Not sure if it helps, but I tried to explain the reason in response to >> patch 0 (cover letter). > > Would it make more sense to have just one patch? It's the same issue > anyway. Yes, but I would solve it in brcmf_fil_* functions (fwil.[ch]). Regards, Arend > -- > https://patchwork.kernel.org/project/linux-wireless/list/ > > https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
On July 2, 2024 5:37:10 PM Dan Carpenter <dan.carpenter@linaro.org> wrote: > On Tue, Jul 02, 2024 at 06:29:20PM +0300, Kalle Valo wrote: >> Arend Van Spriel <arend.vanspriel@broadcom.com> writes: >> >>> On July 2, 2024 3:57:27 PM Dan Carpenter <dan.carpenter@linaro.org> wrote: >>> >>>> On Tue, Jul 02, 2024 at 08:24:44PM +0800, Su Hui wrote: >>>>> brcmf_fil_cmd_int_get() reads the value of 'io_type' and passes it to >>>>> brcmf_fil_cmd_data_get(). Initialize 'io_type' to avoid garbage value. >>>> >>>> Since you're going to be resending anyway, please delete the space char >>>> from the start of the line. >>>> >>>> It's weird that brcmf_fil_cmd_data_get() uses the uninitialized data. >>>> It looks like it just goes to great lengths to preserve the original >>>> data in io_type... So it likely is harmless enough but still a strange >>>> and complicated way write a no-op. >>> >>> Not sure if it helps, but I tried to explain the reason in response to >>> patch 0 (cover letter). >> >> Would it make more sense to have just one patch? It's the same issue >> anyway. > > The Fixes tags are different though. I'd probably leave them as > separate patches just because of that. Depending how you look at the problem those tags are wrong. Regards, Arend
On Tue, Jul 02, 2024 at 06:26:49PM +0200, Arend Van Spriel wrote: > On July 2, 2024 5:37:10 PM Dan Carpenter <dan.carpenter@linaro.org> wrote: > > > On Tue, Jul 02, 2024 at 06:29:20PM +0300, Kalle Valo wrote: > > > Arend Van Spriel <arend.vanspriel@broadcom.com> writes: > > > > > > > On July 2, 2024 3:57:27 PM Dan Carpenter <dan.carpenter@linaro.org> wrote: > > > > > > > > > On Tue, Jul 02, 2024 at 08:24:44PM +0800, Su Hui wrote: > > > > > > brcmf_fil_cmd_int_get() reads the value of 'io_type' and passes it to > > > > > > brcmf_fil_cmd_data_get(). Initialize 'io_type' to avoid garbage value. > > > > > > > > > > Since you're going to be resending anyway, please delete the space char > > > > > from the start of the line. > > > > > > > > > > It's weird that brcmf_fil_cmd_data_get() uses the uninitialized data. > > > > > It looks like it just goes to great lengths to preserve the original > > > > > data in io_type... So it likely is harmless enough but still a strange > > > > > and complicated way write a no-op. > > > > > > > > Not sure if it helps, but I tried to explain the reason in response to > > > > patch 0 (cover letter). > > > > > > Would it make more sense to have just one patch? It's the same issue > > > anyway. > > > > The Fixes tags are different though. I'd probably leave them as > > separate patches just because of that. > > Depending how you look at the problem those tags are wrong. Tags are often unfair in that way where you could blame different commits and you have to pick one. We end up picking the practical commit instead of the most guilty commit. Like if you do a partial or incorrect fix normally you'll be blamed instead of the original patch which has no fix at all. It works because if the backporter hasn't tried to backport the partial fix, they don't care about the complete fix either. regards, dan carpenter
On 2024/7/2 23:39, Arend Van Spriel wrote: > On July 2, 2024 5:29:27 PM Kalle Valo <kvalo@kernel.org> wrote: > >> Arend Van Spriel <arend.vanspriel@broadcom.com> writes: >> >>> On July 2, 2024 3:57:27 PM Dan Carpenter <dan.carpenter@linaro.org> >>> wrote: >>> >>>> On Tue, Jul 02, 2024 at 08:24:44PM +0800, Su Hui wrote: >>>>> brcmf_fil_cmd_int_get() reads the value of 'io_type' and passes it to >>>>> brcmf_fil_cmd_data_get(). Initialize 'io_type' to avoid garbage >>>>> value. >>>> >>>> Since you're going to be resending anyway, please delete the space >>>> char >>>> from the start of the line. >>>> >>>> It's weird that brcmf_fil_cmd_data_get() uses the uninitialized data. >>>> It looks like it just goes to great lengths to preserve the original >>>> data in io_type... So it likely is harmless enough but still a >>>> strange >>>> and complicated way write a no-op. >>> >>> Not sure if it helps, but I tried to explain the reason in response to >>> patch 0 (cover letter). >> >> Would it make more sense to have just one patch? It's the same issue >> anyway. > > Yes, but I would solve it in brcmf_fil_* functions (fwil.[ch]). It seems you will send a new patch to solve this issue. And I guess there is no need for me to resend a v2 patchset or just one patch.
On July 3, 2024 3:42:18 AM Su Hui <suhui@nfschina.com> wrote: > On 2024/7/2 23:39, Arend Van Spriel wrote: >> On July 2, 2024 5:29:27 PM Kalle Valo <kvalo@kernel.org> wrote: >> >>> Arend Van Spriel <arend.vanspriel@broadcom.com> writes: >>> >>>> On July 2, 2024 3:57:27 PM Dan Carpenter <dan.carpenter@linaro.org> >>>> wrote: >>>> >>>>> On Tue, Jul 02, 2024 at 08:24:44PM +0800, Su Hui wrote: >>>>>> brcmf_fil_cmd_int_get() reads the value of 'io_type' and passes it to >>>>>> brcmf_fil_cmd_data_get(). Initialize 'io_type' to avoid garbage >>>>>> value. >>>>> >>>>> Since you're going to be resending anyway, please delete the space >>>>> char >>>>> from the start of the line. >>>>> >>>>> It's weird that brcmf_fil_cmd_data_get() uses the uninitialized data. >>>>> It looks like it just goes to great lengths to preserve the original >>>>> data in io_type... So it likely is harmless enough but still a >>>>> strange >>>>> and complicated way write a no-op. >>>> >>>> Not sure if it helps, but I tried to explain the reason in response to >>>> patch 0 (cover letter). >>> >>> Would it make more sense to have just one patch? It's the same issue >>> anyway. >> >> Yes, but I would solve it in brcmf_fil_* functions (fwil.[ch]). > It seems you will send a new patch to solve this issue. > And I guess there is no need for me to resend a v2 patchset or just one > patch. I am not entirely sure. If both gcc and clang would warn about using uninitialized data I would be fine with these patches rolled into one. Regards, Arend
On 2024/7/3 12:42, Arend Van Spriel wrote: > On July 3, 2024 3:42:18 AM Su Hui <suhui@nfschina.com> wrote: > >> On 2024/7/2 23:39, Arend Van Spriel wrote: >>> On July 2, 2024 5:29:27 PM Kalle Valo <kvalo@kernel.org> wrote: >>> >>>> Arend Van Spriel <arend.vanspriel@broadcom.com> writes: >>>> >>>>> On July 2, 2024 3:57:27 PM Dan Carpenter <dan.carpenter@linaro.org> >>>>> wrote: >>>>> >>>>>> On Tue, Jul 02, 2024 at 08:24:44PM +0800, Su Hui wrote: >>>>>>> brcmf_fil_cmd_int_get() reads the value of 'io_type' and passes >>>>>>> it to >>>>>>> brcmf_fil_cmd_data_get(). Initialize 'io_type' to avoid garbage >>>>>>> value. >>>>>> >>>>>> Since you're going to be resending anyway, please delete the space >>>>>> char >>>>>> from the start of the line. >>>>>> >>>>>> It's weird that brcmf_fil_cmd_data_get() uses the uninitialized >>>>>> data. >>>>>> It looks like it just goes to great lengths to preserve the original >>>>>> data in io_type... So it likely is harmless enough but still a >>>>>> strange >>>>>> and complicated way write a no-op. >>>>> >>>>> Not sure if it helps, but I tried to explain the reason in >>>>> response to >>>>> patch 0 (cover letter). >>>> >>>> Would it make more sense to have just one patch? It's the same issue >>>> anyway. >>> >>> Yes, but I would solve it in brcmf_fil_* functions (fwil.[ch]). >> It seems you will send a new patch to solve this issue. >> And I guess there is no need for me to resend a v2 patchset or just one >> patch. > > I am not entirely sure. If both gcc and clang would warn about using > uninitialized data I would be fine with these patches rolled into one. It's sad that gcc wouldn't warn about this uninitialized data. And my gcc version is 10.2.1 20210110 (Debian 10.2.1-6) . By the way, I found a funny thing about this uninitialized warning. Just with the patch as follows , gcc will give a uninitialized warning. --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c @@ -130,6 +130,7 @@ static int brcmf_c_download_blob(struct brcmf_if *ifp, u32 status; s32 err; + err = brcmf_fil_iovar_int_get(ifp, statvar, &status); brcmf_dbg(TRACE, "Enter\n"); chunk_buf = kzalloc(struct_size(chunk_buf, data, MAX_CHUNK_LEN), It seems that gcc only issue this uninitialized warning in some sitution. I think it's worth a patch to fix this uninitialized problem. :) Regards, Su Hui
On Wed, Jul 03, 2024 at 06:42:32AM +0200, Arend Van Spriel wrote: > On July 3, 2024 3:42:18 AM Su Hui <suhui@nfschina.com> wrote: > > > On 2024/7/2 23:39, Arend Van Spriel wrote: > > > On July 2, 2024 5:29:27 PM Kalle Valo <kvalo@kernel.org> wrote: > > > > > > > Arend Van Spriel <arend.vanspriel@broadcom.com> writes: > > > > > > > > > On July 2, 2024 3:57:27 PM Dan Carpenter <dan.carpenter@linaro.org> > > > > > wrote: > > > > > > > > > > > On Tue, Jul 02, 2024 at 08:24:44PM +0800, Su Hui wrote: > > > > > > > brcmf_fil_cmd_int_get() reads the value of 'io_type' and passes it to > > > > > > > brcmf_fil_cmd_data_get(). Initialize 'io_type' to avoid garbage > > > > > > > value. > > > > > > > > > > > > Since you're going to be resending anyway, please delete the space > > > > > > char > > > > > > from the start of the line. > > > > > > > > > > > > It's weird that brcmf_fil_cmd_data_get() uses the uninitialized data. > > > > > > It looks like it just goes to great lengths to preserve the original > > > > > > data in io_type... So it likely is harmless enough but still a > > > > > > strange > > > > > > and complicated way write a no-op. > > > > > > > > > > Not sure if it helps, but I tried to explain the reason in response to > > > > > patch 0 (cover letter). > > > > > > > > Would it make more sense to have just one patch? It's the same issue > > > > anyway. > > > > > > Yes, but I would solve it in brcmf_fil_* functions (fwil.[ch]). > > It seems you will send a new patch to solve this issue. > > And I guess there is no need for me to resend a v2 patchset or just one > > patch. > > I am not entirely sure. If both gcc and clang would warn about using > uninitialized data I would be fine with these patches rolled into one. We should definitely fix this, it's just a matter of how. UBSan will also detect these at run time. And honestly, it's not clear to me where these eventually do get copied to? Is it to the firmware? In that case it might be that we'd treat these as a CVE. regards, dan carpenter
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c index 5fe0e671ecb3..6be7e7bd8ce7 100644 --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c @@ -8249,7 +8249,7 @@ struct brcmf_cfg80211_info *brcmf_cfg80211_attach(struct brcmf_pub *drvr, struct brcmf_cfg80211_vif *vif; struct brcmf_if *ifp; s32 err = 0; - s32 io_type; + s32 io_type = 0; u16 *cap = NULL; if (!ndev) {
brcmf_fil_cmd_int_get() reads the value of 'io_type' and passes it to brcmf_fil_cmd_data_get(). Initialize 'io_type' to avoid garbage value. Fixes: 83cf17aa8082 ("brcmfmac: adopt new d11 interface") Signed-off-by: Su Hui <suhui@nfschina.com> --- drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)