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 <[email protected]>
---
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 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, u8 *pmgmt_frame,
DBG_871X(FUNC_ADPT_FMT"\n", FUNC_ADPT_ARG(padapter));
{
- struct station_info sinfo;
+ struct station_info sinfo = {};
u8 ie_offset;
if (GetFrameSubType(pmgmt_frame) == WIFI_ASSOCREQ)
ie_offset = _ASOCREQ_IE_OFFSET_;
--
2.25.1
On Sun, Jun 06, 2021 at 12:00:21AM -0700, Wenli Looi wrote:
> 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 <[email protected]>
> ---
> 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 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, 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
On Sun, Jun 6, 2021 at 12:13 AM Greg Kroah-Hartman
<[email protected]> wrote:
> On Sun, Jun 06, 2021 at 12:00:21AM -0700, Wenli Looi wrote:
> > 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 <[email protected]>
> > ---
> > 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 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, 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
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/wireless/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/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c#L6064
Do you think kzalloc() would be preferable?
Sorry, I'm not familiar with "holes" in the struct.
On Sunday, June 6, 2021 9:51:35 AM CEST Wenli Looi wrote:
> On Sun, Jun 6, 2021 at 12:13 AM Greg Kroah-Hartman
>
> <[email protected]> wrote:
> > On Sun, Jun 06, 2021 at 12:00:21AM -0700, Wenli Looi wrote:
> > > 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 <[email protected]>
> > > ---
> > >
> > > 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
> > > 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,
> > > 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
>
> 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
> 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/
> 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.
Fabio
>
> Sorry, I'm not familiar with "holes" in the struct.
On Sun, Jun 6, 2021 at 1:00 AM Fabio M. De Francesco
<[email protected]> wrote:
> On Sunday, June 6, 2021 9:51:35 AM CEST Wenli Looi wrote:
> > On Sun, Jun 6, 2021 at 12:13 AM Greg Kroah-Hartman
> >
> > <[email protected]> wrote:
> > > On Sun, Jun 06, 2021 at 12:00:21AM -0700, Wenli Looi wrote:
> > > > 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 <[email protected]>
> > > > ---
> > > >
> > > > 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
> > > > 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,
> > > > 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
> >
> > 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
> > 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/
> > 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
> >
> > Sorry, I'm not familiar with "holes" in the struct.
>
>
>
>
On Sunday, June 6, 2021 10:09:39 AM CEST Wenli Looi wrote:
> On Sun, Jun 6, 2021 at 1:00 AM Fabio M. De Francesco
>
> <[email protected]> wrote:
> > On Sunday, June 6, 2021 9:51:35 AM CEST Wenli Looi wrote:
> > > On Sun, Jun 6, 2021 at 12:13 AM Greg Kroah-Hartman
> > >
> > > <[email protected]> wrote:
> > > > On Sun, Jun 06, 2021 at 12:00:21AM -0700, Wenli Looi wrote:
> > > > > 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 <[email protected]>
> > > > > ---
> > > > >
> > > > > 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
> >
> > > > > 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,
> >
> > > > > 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
[CUT]
> > >
> > > 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)
>
Why that "if (!sinfo" before kzalloc?
>
> 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?
>
I don't know that code, but I think that:
(1) It is generally preferred that big structures should be allocated
dinamically: kernel stack is a scarce resource.
(2) Passing address of variables from the stack to other functions is
generally a good source of troubles.
I think you are closer to the proper solution.
Fabio
>
> >
> > > Sorry, I'm not familiar with "holes" in the struct.
Uninitialized struct with invalid pointer causes BUG and prevents access
point from working. Access point works once I apply this patch.
This problem seems to have been present from the time the driver was
added to staging. Most users probably do not use access point so they
will not encounter this bug.
https://forum.armbian.com/topic/14727-wifi-ap-kernel-bug-in-kernel-5444/
has more details.
kzalloc() seems to be what other drivers are doing in the same situation
of creating struct station_info and calling cfg80211_new_sta. In
particular, other drivers like ath6kl and mwifiex will silently return
when kzalloc fails, so this seems like the right behavior. (mwifiex
returns -ENOMEM from the place kzalloc is called, but if you follow the
chain of calls, the return value is ultimately ignored)
Links to same situation in other drivers:
https://github.com/torvalds/linux/blob/f5b6eb1e018203913dfefcf6fa988649ad11ad6e/drivers/net/wireless/ath/ath6kl/main.c#L488
https://github.com/torvalds/linux/blob/f5b6eb1e018203913dfefcf6fa988649ad11ad6e/drivers/net/wireless/marvell/mwifiex/uap_event.c#L120
Signed-off-by: Wenli Looi <[email protected]>
---
v1 -> v2: Switched from large stack variable to kzalloc
---
.../staging/rtl8723bs/os_dep/ioctl_cfg80211.c | 28 ++++++++++---------
1 file changed, 15 insertions(+), 13 deletions(-)
diff --git a/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c b/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c
index 9a6e47877..3bc498558 100644
--- a/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c
+++ b/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c
@@ -2082,20 +2082,22 @@ static int cfg80211_rtw_flush_pmksa(struct wiphy *wiphy,
void rtw_cfg80211_indicate_sta_assoc(struct adapter *padapter, u8 *pmgmt_frame, uint frame_len)
{
struct net_device *ndev = padapter->pnetdev;
+ struct station_info *sinfo;
+ u8 ie_offset;
- {
- struct station_info sinfo;
- u8 ie_offset;
- if (GetFrameSubType(pmgmt_frame) == WIFI_ASSOCREQ)
- ie_offset = _ASOCREQ_IE_OFFSET_;
- else /* WIFI_REASSOCREQ */
- ie_offset = _REASOCREQ_IE_OFFSET_;
-
- sinfo.filled = 0;
- sinfo.assoc_req_ies = pmgmt_frame + WLAN_HDR_A3_LEN + ie_offset;
- sinfo.assoc_req_ies_len = frame_len - WLAN_HDR_A3_LEN - ie_offset;
- cfg80211_new_sta(ndev, GetAddr2Ptr(pmgmt_frame), &sinfo, GFP_ATOMIC);
- }
+ sinfo = kzalloc(sizeof(*sinfo), GFP_KERNEL);
+ if (!sinfo)
+ return;
+
+ if (GetFrameSubType(pmgmt_frame) == WIFI_ASSOCREQ)
+ ie_offset = _ASOCREQ_IE_OFFSET_;
+ else /* WIFI_REASSOCREQ */
+ ie_offset = _REASOCREQ_IE_OFFSET_;
+
+ sinfo->assoc_req_ies = pmgmt_frame + WLAN_HDR_A3_LEN + ie_offset;
+ sinfo->assoc_req_ies_len = frame_len - WLAN_HDR_A3_LEN - ie_offset;
+ cfg80211_new_sta(ndev, GetAddr2Ptr(pmgmt_frame), sinfo, GFP_ATOMIC);
+ kfree(sinfo);
}
void rtw_cfg80211_indicate_sta_disassoc(struct adapter *padapter, unsigned char *da, unsigned short reason)
--
2.25.1
On Sun, Jun 06, 2021 at 12:00:21AM -0700, Wenli Looi wrote:
> 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 <[email protected]>
> ---
This patch is correct but the commit message needs to be updated. Your
version 2 patch is not correct.
We don't like "follow this link for all the information" type commit
messages. Clicking on a link is annoying and links die after five
years. The link can be there but the main information needs
to be in the commit message. Generally it's good to put the stack trace
in the commit so that people can search for it.
As Greg pointed out, you need to add a Fixes tag. So far as I can see
it's ->pertid and ->generation which are not initialized and the bugs
were introduced in two different commits so you need two Fixes tags.
Fixes: 8689c051a201 ("cfg80211: dynamically allocate per-tid stats for station info")
Fixes: f5ea9120be2e ("nl80211: add generation number to all dumps")
Adding a Fixes tag will mean the correct people are CC'd in the patch
and can review the fix.
Greg asked about struct holes and the answer is "= {}" will zero out
struct holes but it's not important in this case. The "= {}" is a GCC
extension for zeroing structs and it's not part of the C standard.
The struct has a kernel pointer in it so we had better not be shairing
it to user space.
Here is a better commit message. Please resend the commit with
something like the following.
staging: rtl8723bs: Fix uninitialized variables
The sinfo.pertid and sinfo.generation variables are not initialized
and it causes a crash when we use this as a wireless access point.
[ 456.873025] ------------[ cut here ]------------
[ 456.878198] kernel BUG at mm/slub.c:3968!
[ 456.882680] Internal error: Oops - BUG: 0 [#1] PREEMPT SMP ARM
[ snip ]
[ 457.271004] Backtrace:
[ 457.273733] [<c02b7ee4>] (kfree) from [<c0e2a470>] (nl80211_send_station+0x954/0xfc4)
[ 457.282481] r9:eccca0c0 r8:e8edfec0 r7:00000000 r6:00000011 r5:e80a9480 r4:e8edfe00
[ 457.291132] [<c0e29b1c>] (nl80211_send_station) from [<c0e2b18c>] (cfg80211_new_sta+0x90/0x1cc)
[ 457.300850] r10:e80a9480 r9:e8edfe00 r8:ea678cca r7:00000a20 r6:00000000 r5:ec46d000
[ 457.309586] r4:ec46d9e0
[ 457.312433] [<c0e2b0fc>] (cfg80211_new_sta) from [<bf086684>] (rtw_cfg80211_indicate_sta_assoc+0x80/0x9c [r8723bs])
[ 457.324095] r10:00009930 r9:e85b9d80 r8:bf091050 r7:00000000 r6:00000000 r5:0000001c
[ 457.332831] r4:c1606788
[ 457.335692] [<bf086604>] (rtw_cfg80211_indicate_sta_assoc [r8723bs]) from [<bf03df38>] (rtw_stassoc_event_callback+0x1c8/0x1d4 [r8723bs])
[ 457.349489] r7:ea678cc0 r6:000000a1 r5:f1225f84 r4:f086b000
[ 457.355845] [<bf03dd70>] (rtw_stassoc_event_callback [r8723bs]) from [<bf048e4c>] (mlme_evt_hdl+0x8c/0xb4 [r8723bs])
[ 457.367601] r7:c1604900 r6:f086c4b8 r5:00000000 r4:f086c000
[ 457.373959] [<bf048dc0>] (mlme_evt_hdl [r8723bs]) from [<bf03693c>] (rtw_cmd_thread+0x198/0x3d8 [r8723bs])
[ 457.384744] r5:f086e000 r4:f086c000
[ 457.388754] [<bf0367a4>] (rtw_cmd_thread [r8723bs]) from [<c014a214>] (kthread+0x170/0x174)
[ 457.398083] r10:ed7a57e8 r9:bf0367a4 r8:f086b000 r7:e8ede000 r6:00000000 r5:e9975200
[ 457.406828] r4:e8369900
[ 457.409653] [<c014a0a4>] (kthread) from [<c01010e8>] (ret_from_fork+0x14/0x2c)
[ 457.417718] Exception stack(0xe8edffb0 to 0xe8edfff8)
[ 457.423356] ffa0: 00000000 00000000 00000000 00000000
[ 457.432492] ffc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[ 457.441618] ffe0: 00000000 00000000 00000000 00000000 00000013 00000000
[ 457.449006] r10:00000000 r9:00000000 r8:00000000 r7:00000000 r6:00000000 r5:c014a0a4
[ 457.457750] r4:e9975200
[ 457.460574] Code: 1a000003 e5953004 e3130001 1a000000 (e7f001f2)
[ 457.467381] ---[ end trace 4acbc8c15e9e6aa7 ]---
Link: https://forum.armbian.com/topic/14727-wifi-ap-kernel-bug-in-kernel-5444/
Fixes: 8689c051a201 ("cfg80211: dynamically allocate per-tid stats for station info")
Fixes: f5ea9120be2e ("nl80211: add generation number to all dumps")
Signed-off-by:
regards,
dan carpenter
On Sun, Jun 06, 2021 at 11:46:38AM -0700, Wenli Looi wrote:
> Uninitialized struct with invalid pointer causes BUG and prevents access
> point from working. Access point works once I apply this patch.
>
> This problem seems to have been present from the time the driver was
> added to staging. Most users probably do not use access point so they
> will not encounter this bug.
>
> https://forum.armbian.com/topic/14727-wifi-ap-kernel-bug-in-kernel-5444/
> has more details.
>
> kzalloc() seems to be what other drivers are doing in the same situation
> of creating struct station_info and calling cfg80211_new_sta. In
> particular, other drivers like ath6kl and mwifiex will silently return
> when kzalloc fails, so this seems like the right behavior. (mwifiex
> returns -ENOMEM from the place kzalloc is called, but if you follow the
> chain of calls, the return value is ultimately ignored)
>
> Links to same situation in other drivers:
> https://github.com/torvalds/linux/blob/f5b6eb1e018203913dfefcf6fa988649ad11ad6e/drivers/net/wireless/ath/ath6kl/main.c#L488
> https://github.com/torvalds/linux/blob/f5b6eb1e018203913dfefcf6fa988649ad11ad6e/drivers/net/wireless/marvell/mwifiex/uap_event.c#L120
>
> Signed-off-by: Wenli Looi <[email protected]>
> ---
>
> v1 -> v2: Switched from large stack variable to kzalloc
Nah, v1 was better, it just needs an updated commit message. See my
other email for more details.
regards,
dan carpenter
On Mon, Jun 07, 2021 at 11:35:42AM +0300, Dan Carpenter wrote:
> On Sun, Jun 06, 2021 at 11:46:38AM -0700, Wenli Looi wrote:
> > Uninitialized struct with invalid pointer causes BUG and prevents access
> > point from working. Access point works once I apply this patch.
> >
> > This problem seems to have been present from the time the driver was
> > added to staging. Most users probably do not use access point so they
> > will not encounter this bug.
> >
> > https://forum.armbian.com/topic/14727-wifi-ap-kernel-bug-in-kernel-5444/
> > has more details.
> >
> > kzalloc() seems to be what other drivers are doing in the same situation
> > of creating struct station_info and calling cfg80211_new_sta. In
> > particular, other drivers like ath6kl and mwifiex will silently return
> > when kzalloc fails, so this seems like the right behavior. (mwifiex
> > returns -ENOMEM from the place kzalloc is called, but if you follow the
> > chain of calls, the return value is ultimately ignored)
> >
> > Links to same situation in other drivers:
> > https://github.com/torvalds/linux/blob/f5b6eb1e018203913dfefcf6fa988649ad11ad6e/drivers/net/wireless/ath/ath6kl/main.c#L488
> > https://github.com/torvalds/linux/blob/f5b6eb1e018203913dfefcf6fa988649ad11ad6e/drivers/net/wireless/marvell/mwifiex/uap_event.c#L120
> >
> > Signed-off-by: Wenli Looi <[email protected]>
> > ---
> >
> > v1 -> v2: Switched from large stack variable to kzalloc
>
>
> Nah, v1 was better, it just needs an updated commit message. See my
> other email for more details.
I read this again and I thought I should provide some more information.
This sinfo struct used to be huge and that's why people used to allocate
it if kzalloc() but now it's only 224 bytes so it's okay to put it on
the stack.
And the problem was never whether the variable was on the stack vs on
the heap so changing that wasn't part of the "a patch should do one
thing." If you want to change it to kzalloc you have to do that in a
separate patch (don't do that).
And you were reading Greg's questions as saying the patch was wrong, but
actually they were just questions.
regards,
dan carpenter
On Mon, Jun 07, 2021 at 11:33:17AM +0300, Dan Carpenter wrote:
> Greg asked about struct holes and the answer is "= {}" will zero out
> struct holes but it's not important in this case. The "= {}" is a GCC
> extension for zeroing structs and it's not part of the C standard.
> The struct has a kernel pointer in it so we had better not be shairing
> it to user space.
I thought we proved that "= {}" will _NOT_ zero out holes in structures.
Or did we really prove that? I can't remember now, do you?
thanks,
greg k-h
On Mon, Jun 07, 2021 at 11:23:22AM +0200, Greg Kroah-Hartman wrote:
> On Mon, Jun 07, 2021 at 11:33:17AM +0300, Dan Carpenter wrote:
> > Greg asked about struct holes and the answer is "= {}" will zero out
> > struct holes but it's not important in this case. The "= {}" is a GCC
> > extension for zeroing structs and it's not part of the C standard.
> > The struct has a kernel pointer in it so we had better not be shairing
> > it to user space.
>
> I thought we proved that "= {}" will _NOT_ zero out holes in structures.
> Or did we really prove that? I can't remember now, do you?
>
Assigning a struct to a struct will not initialize the struct holes.
struct foo foo = *p;
We worried about = {} and people looked at the C standard. The
standard is not clear. But then people said that = {} is a GCC
extension and will initialize the struct holes.
The other thing is that in GCC they had intended "= {0};" to work
exactly the same as "= {};" and initialize the holes but there was a
version which had a bug and didn't. :P
regards,
dan carpenter
I will resend the first patch. Thanks for your insightful comments. I
was wondering why every other driver seemed to be allocating "struct
station_info" using kzalloc()
On Mon, Jun 7, 2021 at 1:46 AM Dan Carpenter <[email protected]> wrote:
>
> [â–³EXTERNAL]
>
>
>
> On Mon, Jun 07, 2021 at 11:35:42AM +0300, Dan Carpenter wrote:
> > On Sun, Jun 06, 2021 at 11:46:38AM -0700, Wenli Looi wrote:
> > > Uninitialized struct with invalid pointer causes BUG and prevents access
> > > point from working. Access point works once I apply this patch.
> > >
> > > This problem seems to have been present from the time the driver was
> > > added to staging. Most users probably do not use access point so they
> > > will not encounter this bug.
> > >
> > > https://forum.armbian.com/topic/14727-wifi-ap-kernel-bug-in-kernel-5444/
> > > has more details.
> > >
> > > kzalloc() seems to be what other drivers are doing in the same situation
> > > of creating struct station_info and calling cfg80211_new_sta. In
> > > particular, other drivers like ath6kl and mwifiex will silently return
> > > when kzalloc fails, so this seems like the right behavior. (mwifiex
> > > returns -ENOMEM from the place kzalloc is called, but if you follow the
> > > chain of calls, the return value is ultimately ignored)
> > >
> > > Links to same situation in other drivers:
> > > https://github.com/torvalds/linux/blob/f5b6eb1e018203913dfefcf6fa988649ad11ad6e/drivers/net/wireless/ath/ath6kl/main.c#L488
> > > https://github.com/torvalds/linux/blob/f5b6eb1e018203913dfefcf6fa988649ad11ad6e/drivers/net/wireless/marvell/mwifiex/uap_event.c#L120
> > >
> > > Signed-off-by: Wenli Looi <[email protected]>
> > > ---
> > >
> > > v1 -> v2: Switched from large stack variable to kzalloc
> >
> >
> > Nah, v1 was better, it just needs an updated commit message. See my
> > other email for more details.
>
> I read this again and I thought I should provide some more information.
>
> This sinfo struct used to be huge and that's why people used to allocate
> it if kzalloc() but now it's only 224 bytes so it's okay to put it on
> the stack.
>
> And the problem was never whether the variable was on the stack vs on
> the heap so changing that wasn't part of the "a patch should do one
> thing." If you want to change it to kzalloc you have to do that in a
> separate patch (don't do that).
>
> And you were reading Greg's questions as saying the patch was wrong, but
> actually they were just questions.
>
> regards,
> dan carpenter
>
The sinfo.pertid and sinfo.generation variables are not initialized and
it causes a crash when we use this as a wireless access point.
[ 456.873025] ------------[ cut here ]------------
[ 456.878198] kernel BUG at mm/slub.c:3968!
[ 456.882680] Internal error: Oops - BUG: 0 [#1] PREEMPT SMP ARM
[ snip ]
[ 457.271004] Backtrace:
[ 457.273733] [<c02b7ee4>] (kfree) from [<c0e2a470>] (nl80211_send_station+0x954/0xfc4)
[ 457.282481] r9:eccca0c0 r8:e8edfec0 r7:00000000 r6:00000011 r5:e80a9480 r4:e8edfe00
[ 457.291132] [<c0e29b1c>] (nl80211_send_station) from [<c0e2b18c>] (cfg80211_new_sta+0x90/0x1cc)
[ 457.300850] r10:e80a9480 r9:e8edfe00 r8:ea678cca r7:00000a20 r6:00000000 r5:ec46d000
[ 457.309586] r4:ec46d9e0
[ 457.312433] [<c0e2b0fc>] (cfg80211_new_sta) from [<bf086684>] (rtw_cfg80211_indicate_sta_assoc+0x80/0x9c [r8723bs])
[ 457.324095] r10:00009930 r9:e85b9d80 r8:bf091050 r7:00000000 r6:00000000 r5:0000001c
[ 457.332831] r4:c1606788
[ 457.335692] [<bf086604>] (rtw_cfg80211_indicate_sta_assoc [r8723bs]) from [<bf03df38>] (rtw_stassoc_event_callback+0x1c8/0x1d4 [r8723bs])
[ 457.349489] r7:ea678cc0 r6:000000a1 r5:f1225f84 r4:f086b000
[ 457.355845] [<bf03dd70>] (rtw_stassoc_event_callback [r8723bs]) from [<bf048e4c>] (mlme_evt_hdl+0x8c/0xb4 [r8723bs])
[ 457.367601] r7:c1604900 r6:f086c4b8 r5:00000000 r4:f086c000
[ 457.373959] [<bf048dc0>] (mlme_evt_hdl [r8723bs]) from [<bf03693c>] (rtw_cmd_thread+0x198/0x3d8 [r8723bs])
[ 457.384744] r5:f086e000 r4:f086c000
[ 457.388754] [<bf0367a4>] (rtw_cmd_thread [r8723bs]) from [<c014a214>] (kthread+0x170/0x174)
[ 457.398083] r10:ed7a57e8 r9:bf0367a4 r8:f086b000 r7:e8ede000 r6:00000000 r5:e9975200
[ 457.406828] r4:e8369900
[ 457.409653] [<c014a0a4>] (kthread) from [<c01010e8>] (ret_from_fork+0x14/0x2c)
[ 457.417718] Exception stack(0xe8edffb0 to 0xe8edfff8)
[ 457.423356] ffa0: 00000000 00000000 00000000 00000000
[ 457.432492] ffc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[ 457.441618] ffe0: 00000000 00000000 00000000 00000000 00000013 00000000
[ 457.449006] r10:00000000 r9:00000000 r8:00000000 r7:00000000 r6:00000000 r5:c014a0a4
[ 457.457750] r4:e9975200
[ 457.460574] Code: 1a000003 e5953004 e3130001 1a000000 (e7f001f2)
[ 457.467381] ---[ end trace 4acbc8c15e9e6aa7 ]---
Link: https://forum.armbian.com/topic/14727-wifi-ap-kernel-bug-in-kernel-5444/
Fixes: 8689c051a201 ("cfg80211: dynamically allocate per-tid stats for station info")
Fixes: f5ea9120be2e ("nl80211: add generation number to all dumps")
Signed-off-by: Wenli Looi <[email protected]>
---
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 9a6e47877..2b45df79c 100644
--- a/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c
+++ b/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c
@@ -2084,7 +2084,7 @@ void rtw_cfg80211_indicate_sta_assoc(struct adapter *padapter, u8 *pmgmt_frame,
struct net_device *ndev = padapter->pnetdev;
{
- struct station_info sinfo;
+ struct station_info sinfo = {};
u8 ie_offset;
if (GetFrameSubType(pmgmt_frame) == WIFI_ASSOCREQ)
ie_offset = _ASOCREQ_IE_OFFSET_;
--
2.25.1
If you run get_maintainer.pl on the patch then adds the developers from
the Fixes tags so they can review the patch. I've added them but it's
harder for them to review when they can't apply the patch...
Anyway, it looks good. Thanks!
Reviewed-by: Dan Carpenter <[email protected]>
regards,
dan carpenter
On Mon, Jun 07, 2021 at 11:46:20PM -0700, Wenli Looi wrote:
> The sinfo.pertid and sinfo.generation variables are not initialized and
> it causes a crash when we use this as a wireless access point.
>
> [ 456.873025] ------------[ cut here ]------------
> [ 456.878198] kernel BUG at mm/slub.c:3968!
> [ 456.882680] Internal error: Oops - BUG: 0 [#1] PREEMPT SMP ARM
>
> [ snip ]
>
> [ 457.271004] Backtrace:
> [ 457.273733] [<c02b7ee4>] (kfree) from [<c0e2a470>] (nl80211_send_station+0x954/0xfc4)
> [ 457.282481] r9:eccca0c0 r8:e8edfec0 r7:00000000 r6:00000011 r5:e80a9480 r4:e8edfe00
> [ 457.291132] [<c0e29b1c>] (nl80211_send_station) from [<c0e2b18c>] (cfg80211_new_sta+0x90/0x1cc)
> [ 457.300850] r10:e80a9480 r9:e8edfe00 r8:ea678cca r7:00000a20 r6:00000000 r5:ec46d000
> [ 457.309586] r4:ec46d9e0
> [ 457.312433] [<c0e2b0fc>] (cfg80211_new_sta) from [<bf086684>] (rtw_cfg80211_indicate_sta_assoc+0x80/0x9c [r8723bs])
> [ 457.324095] r10:00009930 r9:e85b9d80 r8:bf091050 r7:00000000 r6:00000000 r5:0000001c
> [ 457.332831] r4:c1606788
> [ 457.335692] [<bf086604>] (rtw_cfg80211_indicate_sta_assoc [r8723bs]) from [<bf03df38>] (rtw_stassoc_event_callback+0x1c8/0x1d4 [r8723bs])
> [ 457.349489] r7:ea678cc0 r6:000000a1 r5:f1225f84 r4:f086b000
> [ 457.355845] [<bf03dd70>] (rtw_stassoc_event_callback [r8723bs]) from [<bf048e4c>] (mlme_evt_hdl+0x8c/0xb4 [r8723bs])
> [ 457.367601] r7:c1604900 r6:f086c4b8 r5:00000000 r4:f086c000
> [ 457.373959] [<bf048dc0>] (mlme_evt_hdl [r8723bs]) from [<bf03693c>] (rtw_cmd_thread+0x198/0x3d8 [r8723bs])
> [ 457.384744] r5:f086e000 r4:f086c000
> [ 457.388754] [<bf0367a4>] (rtw_cmd_thread [r8723bs]) from [<c014a214>] (kthread+0x170/0x174)
> [ 457.398083] r10:ed7a57e8 r9:bf0367a4 r8:f086b000 r7:e8ede000 r6:00000000 r5:e9975200
> [ 457.406828] r4:e8369900
> [ 457.409653] [<c014a0a4>] (kthread) from [<c01010e8>] (ret_from_fork+0x14/0x2c)
> [ 457.417718] Exception stack(0xe8edffb0 to 0xe8edfff8)
> [ 457.423356] ffa0: 00000000 00000000 00000000 00000000
> [ 457.432492] ffc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
> [ 457.441618] ffe0: 00000000 00000000 00000000 00000000 00000013 00000000
> [ 457.449006] r10:00000000 r9:00000000 r8:00000000 r7:00000000 r6:00000000 r5:c014a0a4
> [ 457.457750] r4:e9975200
> [ 457.460574] Code: 1a000003 e5953004 e3130001 1a000000 (e7f001f2)
> [ 457.467381] ---[ end trace 4acbc8c15e9e6aa7 ]---
>
> Link: https://forum.armbian.com/topic/14727-wifi-ap-kernel-bug-in-kernel-5444/
> Fixes: 8689c051a201 ("cfg80211: dynamically allocate per-tid stats for station info")
> Fixes: f5ea9120be2e ("nl80211: add generation number to all dumps")
> Signed-off-by: Wenli Looi <[email protected]>
> ---
> 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 9a6e47877..2b45df79c 100644
> --- a/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c
> +++ b/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c
> @@ -2084,7 +2084,7 @@ void rtw_cfg80211_indicate_sta_assoc(struct adapter *padapter, u8 *pmgmt_frame,
> struct net_device *ndev = padapter->pnetdev;
>
> {
> - struct station_info sinfo;
> + struct station_info sinfo = {};
> u8 ie_offset;
> if (GetFrameSubType(pmgmt_frame) == WIFI_ASSOCREQ)
> ie_offset = _ASOCREQ_IE_OFFSET_;
> --
> 2.25.1
>