linux-kernel - Re: [PATCH] staging: rtl8723bs: Variable rf_type in function rtw_cfg80211_init

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Tue, 1 Oct 2019 14:30:43 +0300
From:   Dan Carpenter <dan.carpenter@...cle.com>
To:     Yizhuo <yzhai003@....edu>
Cc:     csong@...ucr.edu, devel@...verdev.osuosl.org,
        Hariprasad Kelam <hariprasad.kelam@...il.com>,
        Jeeeun Evans <jeeeunevans@...il.com>,
        Puranjay Mohan <puranjay12@...il.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        zhiyunq@...ucr.edu, linux-kernel@...r.kernel.org,
        Larry Finger <Larry.Finger@...inger.net>
Subject: Re: [PATCH] staging: rtl8723bs: Variable rf_type in function
 rtw_cfg80211_init_wiphy() could be uninitialized

On Fri, Sep 27, 2019 at 05:06:51PM -0700, Yizhuo wrote:
> In function rtw_cfg80211_init_wiphy(), local variable "rf_type" could
> be uninitialized if function rtw_hal_get_hwreg() fails to initialize
> it. However, this value is used in function rtw_cfg80211_init_ht_capab()
> and used to decide the value writing to ht_cap, which is potentially
> unsafe.

I feel like this is from a Smatch warning.  Sure, it looks from reading
the code that rtw_hal_get_hwreg() can fail, but actually it cannot.

The longer explanation is that in these rtl drivers if you see a
function with "_hal_" in it that stands for "Hardware Abstraction Layer".
The HAL layer is nonsense.

drivers/staging/rtl8723bs/hal/hal_intf.c
   139  void rtw_hal_get_hwreg(struct adapter *padapter, u8 variable, u8 *val)
   140  {
   141          if (padapter->HalFunc.GetHwRegHandler)
   142                  padapter->HalFunc.GetHwRegHandler(padapter, variable, val);
   143  }

It looks as if reading the hardware register is an optional feature but
obviously that's not possibly true.

We can use Smatch to find out which functions implement the function
pointer:
~/smatch/smatch_data/db/smdb.py functions GetHwRegHandler

drivers/staging/rtl8723bs/hal/sdio_halinit.c | (struct hal_ops)->GetHwRegHandler | GetHwReg8723BS  | 1

So in this driver the ->GetHwRegHandler pointer always points to the
GetHwReg8723BS() function.  Then we can check what the return states
for that function are:

~/smatch/smatch_data/db/smdb.py return_states GetHwReg8723BS

It prints a lot of information but the relevant line is:

drivers/staging/rtl8723bs/hal/sdio_halinit.c | GetHwReg8723BS | 84 |               |     PARAM_SET |   2 |                   *$ |             0-u16max |

Which means that the *val is always set and never uninitialized.  This
is after I have rebuilt my Smatch DB several times.  I rebuild it every
day and it has been a long time since I started from scratch.

So removing the HAL layer would make this code parsable by Smatch and it
would make it more readable for human beings as well.  Another option
would be to just delete the "if (padapter->HalFunc.GetHwRegHandler)"
check which would also silence the false positive.  A third option would
be to add "rtw_hal_get_hwreg 2" to the
~/smatch/smatch_data/kernel.ignore_uninitialized_param file.

regards,
dan carpenter