Hello everyone,
I recently found that a local variable in passed uninitialised to the
function at
drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c:2950
u32 var;
err = brcmf_fil_iovar_int_get(ifp, "dtim_assoc", &var);
if (err) {
brcmf_err("wl dtim_assoc failed (%d)\n", err);
goto update_bss_info_out;
}
dtim_period = (u8)var;
Now, the brcmf_fil_iovar_int_get() is defined as:
s32
brcmf_fil_iovar_int_get(struct brcmf_if *ifp, char *name, u32 *data)
{
__le32 data_le = cpu_to_le32(*data);
s32 err;
err = brcmf_fil_iovar_data_get(ifp, name, &data_le, sizeof(data_le));
if (err == 0)
*data = le32_to_cpu(data_le);
return err;
}
We can cleary see that 'var' in used uninitialised in the very first line
which is an undefined behavior.
So, what could be a possible fix for the above ?
I'm not sure initialising 'var' to 0 would be the correct solution.
--
Thanks
Himanshu Jha
On 3/28/2018 1:20 PM, Himanshu Jha wrote:
> Hello everyone,
You added everyone, but me :-(
Not really a problem, but it would help if the driver name was mentioned
in the subject.
> I recently found that a local variable in passed uninitialised to the
> function at
>
> drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c:2950
>
> u32 var;
> err = brcmf_fil_iovar_int_get(ifp, "dtim_assoc", &var);
> if (err) {
> brcmf_err("wl dtim_assoc failed (%d)\n", err);
> goto update_bss_info_out;
> }
> dtim_period = (u8)var;
>
>
> Now, the brcmf_fil_iovar_int_get() is defined as:
>
> s32
> brcmf_fil_iovar_int_get(struct brcmf_if *ifp, char *name, u32 *data)
> {
> __le32 data_le = cpu_to_le32(*data);
> s32 err;
>
> err = brcmf_fil_iovar_data_get(ifp, name, &data_le, sizeof(data_le));
> if (err == 0)
> *data = le32_to_cpu(data_le);
> return err;
> }
>
> We can cleary see that 'var' in used uninitialised in the very first line
> which is an undefined behavior.
Why undefined? We copy some stack data and we do transfer that to the
device. However in this case the device does nothing with it and it is
simply overwritten by the response.
> So, what could be a possible fix for the above ?
>
> I'm not sure initialising 'var' to 0 would be the correct solution.
Coverity flagged this and probably still does. For this particular
instance setting var to '0' is fine. However, there are quite a few
other places. For some instances the data contains a selector value for
obtaining info from the device, which is what we copy in
brcmf_fil_iovar_int_get(). So maybe it would be best to have a separate
function for those, eg. brcmf_fil_iovar_int_selget() or so.
Regards,
Arend
Arend van Spriel <[email protected]> writes:
> On 3/28/2018 1:20 PM, Himanshu Jha wrote:
>> I recently found that a local variable in passed uninitialised to the
>> function at
>>
>> drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c:2950
>>
>> u32 var;
>> err = brcmf_fil_iovar_int_get(ifp, "dtim_assoc", &var);
>>
>> s32
>> brcmf_fil_iovar_int_get(struct brcmf_if *ifp, char *name, u32 *data)
>> {
>> __le32 data_le = cpu_to_le32(*data);
>> }
>>
>> We can cleary see that 'var' in used uninitialised in the very first line
>> which is an undefined behavior.
>
> Why undefined? We copy some stack data and we do transfer that to the device. However in this case
> the device does nothing with it and it is simply overwritten by the response.
"Undefined behavior" is a technical term for when there are no
guarantees as to what the result of executing a given code will be. None
at all--it might for example abort, and that would be perfectly valid as
well. (To be clear, this is not about the device, but about the CPU that
this code runs on.)
Uninitialized reads are one example of a code construct that invokes
undefined behavior.
Thanks,
Petr