Re: [PATCH 0/4] brcm80211: Misc coverity fixes

[Florian Fainelli <f.fainelli@gmail.com>]

On 07/19/2016 12:36 PM, Arend Van Spriel wrote:
> On 19-7-2016 20:30, Florian Fainelli wrote:
>> On 07/19/2016 11:21 AM, Arend Van Spriel wrote:
>>> On 19-7-2016 18:41, Florian Fainelli wrote:
>>>> On 07/19/2016 02:20 AM, Arend Van Spriel wrote:
>>>>> + Bob
>>>>>
>>>>> On 19-7-2016 1:24, Florian Fainelli wrote:
>>>>>> Hi,
>>>>>>
>>>>>> This patch series addresses several coverity issues, they all seemed relevant
>>>>>> to me.
>>>>>
>>>>> Hi Florian,
>>>>>
>>>>> Been a while so nice to see coverity fixes popping up. Actually
>>>>> something that I have on my todo list to add our brcm80211 to coverity
>>>>> within Broadcom. So being curious as to whether this comes from a public
>>>>> coverity server like scan.coverity.com. Maybe bit redundant to setup
>>>>> internally if there is a good coverity analysis publicly available.
>>>>
>>>> This is coming from the public coverity instance, if you create an
>>>> account there I could transfer to you the other bugs that affect the
>>>> brcm80211 drivers (hint: there is a ton of of them because of
>>>> brcmf_fil_iovar_int_get and friends).
>>>
>>> I did create an account and noticed "Broadcom Linux Kernel Open Source
>>> Repository" project. Anyways, let me have it :-p
>>>
>>>>>
>>>>>> There is also a ton of warnings in Coverity caused by brcmf_fil_iovar_int_get()
>>>>>> and friends because of the initial access:
>>>>>>
>>>>>> __le32 data_le = cpu_to_le32(*data) which can utilize unitialized memory. I am
>>>>>> not sure if we actually care about any kind of initial, value, but if we don't,
>>>>>> then the fix should be fairly obvious.
>>>>>
>>>>> If we are talking only about "get" variant than we mostly don't care.
>>>>> Some getters support filter variables to be passed towards firmware. I
>>>>> have not looked at the analysis to give any judgement here.
>>>>
>>>> Alright, do you have a good way to test a patch that would just zero
>>>> initialize the data variable in brcmf_fil_iovar_int_get()? If so, I will
>>>> submit one with the appropriate CID references.
>>>
>>> But why would we care to zero the data when firmware simply overwrites
>>> it. These control messages are not high-prio so we can burn some cycles
>>> to initialize the variable, but to me this is a false positive.
>>
>> We have something like this all over the place:
>>
>> func() {
>> 	u32 val;
>>
>> 	brcmf_fil_iovar_int_get(..., &val, sizeof(val));
> 
> If I am not mistaken the iovar_int_get only passes &val.

We pass data and len:

        err = brcmf_fil_iovar_data_get(ifp, name, &data_le,
sizeof(data_le));
        if (err == 0)



> 
>> }
>>
>> brcmf_fil_iovar_int_get(.., const u32 *data, size_t len)
>> {
>> 	__le32 data_le = cpu_to_le32(*data);
>> 	...
>> }
>>
>> So here data_le also has an uninitialized value, and later on, this:
>>
>>
>>         buflen = brcmf_create_iovar(name, data, len, drvr->proto_buf,
>>                                     sizeof(drvr->proto_buf));
> 
> create_iovar is used for both get and set...
> 
>> }
>>
>> And this function does this:
>>
>> static u32
>> brcmf_create_iovar(char *name, const char *data, u32 datalen,
>>                    char *buf, u32 buflen)
>> {
>>         u32 len;
>>
>>         len = strlen(name) + 1;
>>
>>         if ((len + datalen) > buflen)
>>                 return 0;
>>
>>         memcpy(buf, name, len);
>>
>>         /* append data onto the end of the name string */
>>         if (data && datalen)
>>                 memcpy(&buf[len], data, datalen);
> 
> but it does not have that info so it does the memcpy. We could add an
> extra parameter to avoid the memcpy for get, but as said there are
> instances where get passes query data to firmware.

OK.

> 
>>         return len + datalen;
>> }
>>
>>
>> We are essentially copying into buf an uninitialized value coming from
>> the stack, which seems like a problem to me.
> 
> Functionally I still don't see the problem. For the instances where this
> is true, the buf will be overwritten with data from firmware. It may be
> a security issue as stack content is passed to an external device.

That's the part that I am mostly concerned with, and the fact that this
is a genuine uninitialized value, not under direct control, but still.
-- 
Florian