Search Linux Wireless

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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 2022/01/06 23:20, Andy Shevchenko wrote:
> 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:

The point is that BRCMF_FW_MACADDR_LEN and BRCMF_FW_MACADDR_FMT need to
be in sync, and BRCMF_FW_MACADDR_LEN is used in two different places. If
I inline the format string into the code, someone could change it
without changing the length, or changing the length inline only next to
the format string. Then we overflow the NVRAM buffer because the
allocation is not sized properly.

By having them as defines, it is obvious that they go together, and if
one changes the other one has to change too, and the nvram allocation
can't end up improperly sized as long as they are kept in sync.

> 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?
> 

That snprintf can never fail as long as the format string/length are in
sync. I'll make it BUG_ON(... != size), so it complains loudly if
someone screws up the format string in the future.

-- 
Hector Martin (marcan@xxxxxxxxx)
Public Key: https://mrcn.st/pub



[Index of Archives]     [Linux Host AP]     [ATH6KL]     [Linux Wireless Personal Area Network]     [Linux Bluetooth]     [Wireless Regulations]     [Linux Netdev]     [Kernel Newbies]     [Linux Kernel]     [IDE]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite Hiking]     [MIPS Linux]     [ARM Linux]     [Linux RAID]

  Powered by Linux