Subject: Re: [PATCH 0/4] brcm80211: Misc coverity fixes
To: Florian Fainelli <f.fainelli@gmail.com>,
	brcm80211-dev-list.pdl@broadcom.com
References: <1468884277-18606-1-git-send-email-f.fainelli@gmail.com>
 <bdbaa850-4ec4-9faf-dbae-e2a6dcc54e7c@broadcom.com>
 <578E5845.6030100@gmail.com>
 <53a36e85-37ad-10a5-81fc-40ce4efa6a65@broadcom.com>
 <578E71C8.1050003@gmail.com>
Cc: linux-wireless@vger.kernel.org, pieterpg@broadcom.com,
	kvalo@codeaurora.org, hante.meuleman@broadcom.com
From: Arend Van Spriel <arend.vanspriel@broadcom.com>
Message-ID: <9774b991-ef09-66c9-c997-b2d043db5f0f@broadcom.com> (sfid-20160719_213621_677701_CE549141)
Date: Tue, 19 Jul 2016 21:36:09 +0200
MIME-Version: 1.0
In-Reply-To: <578E71C8.1050003@gmail.com>
Content-Type: text/plain; charset=utf-8
Sender: linux-wireless-owner@vger.kernel.org

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.

> }
> 
> 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.

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

Regards,
Arend