On Sun, Jun 6, 2021 at 1:00 AM Fabio M. De Francesco
[off-list ref] wrote:

On Sunday, June 6, 2021 9:51:35 AM CEST Wenli Looi wrote:

quoted

On Sun, Jun 6, 2021 at 12:13 AM Greg Kroah-Hartman

[off-list ref] wrote:

quoted

On Sun, Jun 06, 2021 at 12:00:21AM -0700, Wenli Looi wrote:

quoted

Uninitialized struct with invalid pointer causes BUG and prevents access
point from working. Access point works once I apply this patch.

https://forum.armbian.com/topic/14727-wifi-ap-kernel-bug-in-kernel-5444/
has more details.

Signed-off-by: Wenli Looi <redacted>
---

 drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c

b/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c index 2fb80b6eb..

7308e1185

quoted

quoted

quoted

100644

--- a/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c
+++ b/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c

@@ -2384,7 +2384,7 @@ void rtw_cfg80211_indicate_sta_assoc(struct

adapter *padapter,

quoted

quoted

quoted

u8 *pmgmt_frame,> >
      DBG_871X(FUNC_ADPT_FMT"\n", FUNC_ADPT_ARG(padapter));

      {

-             struct station_info sinfo;
+             struct station_info sinfo = {};

What caused this bug to show up?  Did it happen from some other commit?

Are you sure that all of the fields are being cleared properly here,
what about any "holes" in the structure?

thanks,
greg k-h

I believe this bug has been present since the driver was added to
staging:
https://github.com/torvalds/linux/commit/

554c0a3abf216c991c5ebddcdb2c08689ecd290b

quoted

It's probably not necessary to zero the entire struct, only
sinfo->pertid, which causes problems with the code here:
https://github.com/torvalds/linux/blob/

f5b6eb1e018203913dfefcf6fa988649ad11ad6e/net/wire

quoted

less/nl80211.c#L5919

You can see the following proposed OpenWrt patch
(700-wifi-8723bs-ap-bugfix.patch in
https://github.com/openwrt/openwrt/pull/4053/files) which sets
sinfo.pertid = 0; instead of zeroing the struct.

Looking at similar code in a non-staging driver, we can see that the
code there zeros the struct using kzalloc():
https://github.com/torvalds/linux/blob/

f5b6eb1e018203913dfefcf6fa988649ad11ad6e/drivers/

quoted

net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c#L6064

Do you think kzalloc() would be preferable?

You cannot use kzalloc there: 'sinfo' is instantiated automatically on the
stack. The example you took had a pointer to the struct.

The stack variable could be replaced with code like:

struct station_info *sinfo;if (!sinfo)
sinfo = kzalloc(sizeof(*sinfo), GFP_KERNEL);
...
cfg80211_new_sta(..., sinfo, ...);
kfree(sinfo);

which is what the linked code basically does. I'm not sure if this is preferred?

Fabio

quoted

Sorry, I'm not familiar with "holes" in the struct.