Re: [PATCH v2 10/35] brcmfmac: firmware: Allow platform to override macaddr

[Andy Shevchenko]

On Wed, Jan 5, 2022 at 3:26 PM Hector Martin <marcan@xxxxxxxxx> wrote:
> On 04/01/2022 23.23, Andy Shevchenko wrote:
> > On Tue, Jan 4, 2022 at 9:29 AM Hector Martin <marcan@xxxxxxxxx> wrote:

...

> >> +#define BRCMF_FW_MACADDR_FMT                   "macaddr=%pM"

> >> +       snprintf(&nvp->nvram[nvp->nvram_len], BRCMF_FW_MACADDR_LEN + 1,
> >> +                BRCMF_FW_MACADDR_FMT, mac);
> >
> > Please, avoid using implict format string, it's dangerous from security p.o.v.
>
> What do you mean by implicit format string?

When I read the above code I feel uncomfortable because no-one can see
(without additional action and more reading and checking) if it's
correct or not. This is potential to be error prone.

> The format string is at the
> top of the file and its length is right next to it, which makes it
> harder for them to accidentally fall out of sync.

It is not an argument. Just you may do the same in the code directly
and more explicitly:

Also you don't check the return code of snprintf which means that you
don't care about the result, which seems to me wrong approach. If you
don't care about the result, so it means it's not very important,
right?

> +#define BRCMF_FW_MACADDR_FMT                   "macaddr=%pM"
> +#define BRCMF_FW_MACADDR_LEN                   (7 + ETH_ALEN * 3)



-- 
With Best Regards,
Andy Shevchenko