On Tue, Oct 17, 2023 at 08:11:29PM +0000, Justin Stitt wrote:
> Let's move away from using strncpy and instead use the more obvious
> interface for this context.
>
> For wlc->pub->srom_ccode, we're just copying two bytes from ccode into
> wlc->pub->srom_ccode with no expectation that srom_ccode be
> NUL-terminated:
> wlc->pub->srom_ccode is only used in regulatory_hint():
> 1193 | if (wl->pub->srom_ccode[0] &&
> 1194 | regulatory_hint(wl->wiphy, wl->pub->srom_ccode))
> 1195 | wiphy_err(wl->wiphy, "%s: regulatory hint failed\n",
> __func__);
>
> We can see that only index 0 and index 1 are accessed.
> 3307 | int regulatory_hint(struct wiphy *wiphy, const char *alpha2)
> 3308 | {
> ... | ...
> 3322 | request->alpha2[0] = alpha2[0];
> 3323 | request->alpha2[1] = alpha2[1];
> ... | ...
> 3332 | }
>
> Since this is just a simple byte copy with correct lengths, let's use
> memcpy(). There should be no functional change.
>
> In a similar boat, both wlc->country_default and
> wlc->autocountry_default are just simple byte copies so let's use
> memcpy. However, FWICT they aren't used anywhere. (they should be
> used or removed -- not in scope of my patch, though).
>
> Signed-off-by: Justin Stitt <[email protected]>
> ---
> drivers/net/wireless/broadcom/brcm80211/brcmsmac/channel.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmsmac/channel.c
> b/drivers/net/wireless/broadcom/brcm80211/brcmsmac/channel.c
> index 5a6d9c86552a..f6962e558d7c 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmsmac/channel.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmsmac/channel.c
> @@ -341,7 +341,7 @@ struct brcms_cm_info *brcms_c_channel_mgr_attach(struct
> brcms_c_info *wlc)
> /* store the country code for passing up as a regulatory hint */
> wlc_cm->world_regd = brcms_world_regd(ccode, ccode_len);
> if (brcms_c_country_valid(ccode))
> - strncpy(wlc->pub->srom_ccode, ccode, ccode_len);
> + memcpy(wlc->pub->srom_ccode, ccode, ccode_len);
const char *ccode = sprom->alpha2;
int ccode_len = sizeof(sprom->alpha2);
struct ssb_sprom {
...
char alpha2[2]; /* Country Code as two chars like EU or US */
This should be marked __nonstring, IMO.
struct brcms_pub {
...
char srom_ccode[BRCM_CNTRY_BUF_SZ]; /* Country Code in SROM */
#define BRCM_CNTRY_BUF_SZ 4 /* Country string is 3 bytes + NUL */
This, however, is shown as explicitly %NUL terminated.
The old strncpy wasn't %NUL terminating wlc->pub->srom_ccode, though, so
the memcpy is the same result, but is that actually _correct_ here?
>
> /*
> * If no custom world domain is found in the SROM, use the
> @@ -354,10 +354,10 @@ struct brcms_cm_info *brcms_c_channel_mgr_attach(struct
> brcms_c_info *wlc)
> }
>
> /* save default country for exiting 11d regulatory mode */
> - strncpy(wlc->country_default, ccode, ccode_len);
> + memcpy(wlc->country_default, ccode, ccode_len);
>
> /* initialize autocountry_default to driver default */
> - strncpy(wlc->autocountry_default, ccode, ccode_len);
> + memcpy(wlc->autocountry_default, ccode, ccode_len);
struct brcms_c_info {
...
char country_default[BRCM_CNTRY_BUF_SZ];
char autocountry_default[BRCM_CNTRY_BUF_SZ];
These are similar...
So, this change results in the same behavior, but is it right?
-Kees
--
Kees Cook
