From mboxrd@z Thu Jan 1 00:00:00 1970 From: Arend van Spriel Subject: Re: Passing uninitialised local variable Date: Thu, 29 Mar 2018 23:14:29 +0200 Message-ID: <5ABD5735.1050608@broadcom.com> References: <20180328112014.GA11484@himanshu-Vostro-3559> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Cc: kvalo@codeaurora.org, johannes.berg@intel.com, linux-wireless@vger.kernel.org, brcm80211-dev-list.pdl@broadcom.com, brcm80211-dev-list@cypress.com, netdev@vger.kernel.org To: Himanshu Jha , franky.lin@broadcom.com, hante.meuleman@broadcom.com, chi-hsien.lin@cypress.com, wright.feng@cypress.com Return-path: Received: from mail-qk0-f181.google.com ([209.85.220.181]:33429 "EHLO mail-qk0-f181.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752116AbeC2VOe (ORCPT ); Thu, 29 Mar 2018 17:14:34 -0400 Received: by mail-qk0-f181.google.com with SMTP id d206so7453629qkb.0 for ; Thu, 29 Mar 2018 14:14:33 -0700 (PDT) In-Reply-To: <20180328112014.GA11484@himanshu-Vostro-3559> Sender: netdev-owner@vger.kernel.org List-ID: 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