| Message ID | 20220117142919.207370-4-marcan@marcan.st (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | misc brcmfmac fixes (M1/T2 series spin-off) | expand |
On 1/17/2022 3:29 PM, Hector Martin wrote: > This unbreaks support for USB devices, which do not have a board_type > to create an alt_path out of and thus were running into a NULL > dereference. > > Fixes: 5ff013914c62 ("brcmfmac: firmware: Allow per-board firmware binaries") Reviewed-by: Arend van Spriel <arend.vanspriel@broadcom.com> > Signed-off-by: Hector Martin <marcan@marcan.st> > --- > drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c | 3 +++ > 1 file changed, 3 insertions(+)
On Mon, Jan 17, 2022 at 4:30 PM Hector Martin <marcan@marcan.st> wrote: > > This unbreaks support for USB devices, which do not have a board_type > to create an alt_path out of and thus were running into a NULL > dereference. In v5.16 we have two call sites: 1. if (cur->type == BRCMF_FW_TYPE_NVRAM && fwctx->req->board_type) { ... alt_path = brcm_alt_fw_path(cur->path, fwctx->req->board_type); 2. alt_path = brcm_alt_fw_path(first->path, fwctx->req->board_type); if (alt_path) { ... Looking at them I would rather expect to see (as a quick fix, the better solution is to unify those call sites by splitting out a common helper): if (fwctx->req->board_type) { alt_path = brcm_alt_fw_path(first->path, fwctx->req->board_type); else alt_path = NULL; ... > @@ -599,6 +599,9 @@ static char *brcm_alt_fw_path(const char *path, const char *board_type) > char alt_path[BRCMF_FW_NAME_LEN]; > char suffix[5]; > > + if (!board_type) > + return NULL;
17.01.2022 17:29, Hector Martin пишет: > This unbreaks support for USB devices, which do not have a board_type > to create an alt_path out of and thus were running into a NULL > dereference. > > Fixes: 5ff013914c62 ("brcmfmac: firmware: Allow per-board firmware binaries") > Signed-off-by: Hector Martin <marcan@marcan.st> Technically, all patches that are intended to be included into next stable kernel update require the "Cc: stable@vger.kernel.org" tag. In practice such patches usually auto-picked by the patch bot, so no need to resend. > --- > drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c > index 1001c8888bfe..63821856bbe1 100644 > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c > @@ -599,6 +599,9 @@ static char *brcm_alt_fw_path(const char *path, const char *board_type) > char alt_path[BRCMF_FW_NAME_LEN]; > char suffix[5]; > > + if (!board_type) > + return NULL; > + > strscpy(alt_path, path, BRCMF_FW_NAME_LEN); > /* At least one character + suffix */ > if (strlen(alt_path) < 5) Good catch! Reviewed-by: Dmitry Osipenko <digetx@gmail.com>
On 20/01/2022 06.45, Andy Shevchenko wrote: > On Mon, Jan 17, 2022 at 4:30 PM Hector Martin <marcan@marcan.st> wrote: >> >> This unbreaks support for USB devices, which do not have a board_type >> to create an alt_path out of and thus were running into a NULL >> dereference. > > In v5.16 we have two call sites: > > 1. > if (cur->type == BRCMF_FW_TYPE_NVRAM && fwctx->req->board_type) { > ... > alt_path = brcm_alt_fw_path(cur->path, fwctx->req->board_type); > > 2. > alt_path = brcm_alt_fw_path(first->path, fwctx->req->board_type); > if (alt_path) { > ... > > Looking at them I would rather expect to see (as a quick fix, the > better solution is to unify those call sites by splitting out a common > helper): > > if (fwctx->req->board_type) { > alt_path = brcm_alt_fw_path(first->path, fwctx->req->board_type); > else > alt_path = NULL; > ... > Since brcm_alt_fw_path can fail anyway, and its return value is already NULL-checked, it makes sense to propagate the NULL board_path there rather than doing it at all the callsites. That's a common pattern, e.g. the entire DT API is designed to accept NULL nodes. That does mean that the first callsite has a redundant NULL check, yes, but that doesn't hurt. This is all going to change with subsequent patches anyway; the point of this patch is just to fix the regression.
On 1/19/2022 11:02 PM, Dmitry Osipenko wrote: > 17.01.2022 17:29, Hector Martin пишет: >> This unbreaks support for USB devices, which do not have a board_type >> to create an alt_path out of and thus were running into a NULL >> dereference. >> >> Fixes: 5ff013914c62 ("brcmfmac: firmware: Allow per-board firmware binaries") >> Signed-off-by: Hector Martin <marcan@marcan.st> > > Technically, all patches that are intended to be included into next > stable kernel update require the "Cc: stable@vger.kernel.org" tag. Being the nit picker that I am I would say it is recommended to safe yourself extra work, not required, for the reason you give below. > In practice such patches usually auto-picked by the patch bot, so no > need to resend. Regards, Arend
20.01.2022 11:29, Arend van Spriel пишет: > On 1/19/2022 11:02 PM, Dmitry Osipenko wrote: >> 17.01.2022 17:29, Hector Martin пишет: >>> This unbreaks support for USB devices, which do not have a board_type >>> to create an alt_path out of and thus were running into a NULL >>> dereference. >>> >>> Fixes: 5ff013914c62 ("brcmfmac: firmware: Allow per-board firmware >>> binaries") >>> Signed-off-by: Hector Martin <marcan@marcan.st> >> >> Technically, all patches that are intended to be included into next >> stable kernel update require the "Cc: stable@vger.kernel.org" tag. > > Being the nit picker that I am I would say it is recommended to safe > yourself extra work, not required, for the reason you give below. Will be nice if stable tag could officially become a recommendation, implying the stable tag. It's a requirement today, at least Greg KH always demands to add it :)
20.01.2022 16:23, Dmitry Osipenko пишет: > 20.01.2022 11:29, Arend van Spriel пишет: >> On 1/19/2022 11:02 PM, Dmitry Osipenko wrote: >>> 17.01.2022 17:29, Hector Martin пишет: >>>> This unbreaks support for USB devices, which do not have a board_type >>>> to create an alt_path out of and thus were running into a NULL >>>> dereference. >>>> >>>> Fixes: 5ff013914c62 ("brcmfmac: firmware: Allow per-board firmware >>>> binaries") >>>> Signed-off-by: Hector Martin <marcan@marcan.st> >>> >>> Technically, all patches that are intended to be included into next >>> stable kernel update require the "Cc: stable@vger.kernel.org" tag. >> >> Being the nit picker that I am I would say it is recommended to safe >> yourself extra work, not required, for the reason you give below. > > Will be nice if stable tag could officially become a recommendation, > implying the stable tag. It's a requirement today, at least Greg KH > always demands to add it :) *implying the stable tag if "fixes" tag presents.
On 1/20/2022 2:24 PM, Dmitry Osipenko wrote: > 20.01.2022 16:23, Dmitry Osipenko пишет: >> 20.01.2022 11:29, Arend van Spriel пишет: >>> On 1/19/2022 11:02 PM, Dmitry Osipenko wrote: >>>> 17.01.2022 17:29, Hector Martin пишет: >>>>> This unbreaks support for USB devices, which do not have a board_type >>>>> to create an alt_path out of and thus were running into a NULL >>>>> dereference. >>>>> >>>>> Fixes: 5ff013914c62 ("brcmfmac: firmware: Allow per-board firmware >>>>> binaries") >>>>> Signed-off-by: Hector Martin <marcan@marcan.st> >>>> >>>> Technically, all patches that are intended to be included into next >>>> stable kernel update require the "Cc: stable@vger.kernel.org" tag. >>> >>> Being the nit picker that I am I would say it is recommended to safe >>> yourself extra work, not required, for the reason you give below. >> >> Will be nice if stable tag could officially become a recommendation, >> implying the stable tag. It's a requirement today, at least Greg KH >> always demands to add it :) > > *implying the stable tag if "fixes" tag presents. I was a little confused reading your previous email in this thread. This makes a lot more sense :-p
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c index 1001c8888bfe..63821856bbe1 100644 --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c @@ -599,6 +599,9 @@ static char *brcm_alt_fw_path(const char *path, const char *board_type) char alt_path[BRCMF_FW_NAME_LEN]; char suffix[5]; + if (!board_type) + return NULL; + strscpy(alt_path, path, BRCMF_FW_NAME_LEN); /* At least one character + suffix */ if (strlen(alt_path) < 5)
This unbreaks support for USB devices, which do not have a board_type to create an alt_path out of and thus were running into a NULL dereference. Fixes: 5ff013914c62 ("brcmfmac: firmware: Allow per-board firmware binaries") Signed-off-by: Hector Martin <marcan@marcan.st> --- drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c | 3 +++ 1 file changed, 3 insertions(+)