* [PATCH] staging: rtl8723bs: Fix uninitialized variable @ 2021-06-06 7:00 Wenli Looi 2021-06-06 7:13 ` Greg Kroah-Hartman 2021-06-07 8:33 ` [PATCH] " Dan Carpenter 0 siblings, 2 replies; 17+ messages in thread From: Wenli Looi @ 2021-06-06 7:00 UTC (permalink / raw) To: Greg Kroah-Hartman, linux-staging, linux-kernel; +Cc: Wenli Looi 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 <wlooi@ucalgary.ca> --- 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 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH] staging: rtl8723bs: Fix uninitialized variable 2021-06-06 7:00 [PATCH] staging: rtl8723bs: Fix uninitialized variable Wenli Looi @ 2021-06-06 7:13 ` Greg Kroah-Hartman 2021-06-06 7:51 ` Wenli Looi 2021-06-07 8:33 ` [PATCH] " Dan Carpenter 1 sibling, 1 reply; 17+ messages in thread From: Greg Kroah-Hartman @ 2021-06-06 7:13 UTC (permalink / raw) To: Wenli Looi; +Cc: linux-staging, linux-kernel 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 <wlooi@ucalgary.ca> > --- > 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] staging: rtl8723bs: Fix uninitialized variable 2021-06-06 7:13 ` Greg Kroah-Hartman @ 2021-06-06 7:51 ` Wenli Looi 2021-06-06 8:00 ` Fabio M. De Francesco 0 siblings, 1 reply; 17+ messages in thread From: Wenli Looi @ 2021-06-06 7:51 UTC (permalink / raw) To: Greg Kroah-Hartman; +Cc: linux-staging, linux-kernel On Sun, Jun 6, 2021 at 12:13 AM Greg Kroah-Hartman <gregkh@linuxfoundation.org> 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 <wlooi@ucalgary.ca> > > --- > > 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. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] staging: rtl8723bs: Fix uninitialized variable 2021-06-06 7:51 ` Wenli Looi @ 2021-06-06 8:00 ` Fabio M. De Francesco 2021-06-06 8:09 ` Wenli Looi 0 siblings, 1 reply; 17+ messages in thread From: Fabio M. De Francesco @ 2021-06-06 8:00 UTC (permalink / raw) To: Greg Kroah-Hartman, Wenli Looi; +Cc: linux-staging, linux-kernel 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 > > <gregkh@linuxfoundation.org> 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 <wlooi@ucalgary.ca> > > > --- > > > > > > 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. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] staging: rtl8723bs: Fix uninitialized variable 2021-06-06 8:00 ` Fabio M. De Francesco @ 2021-06-06 8:09 ` Wenli Looi 0 siblings, 0 replies; 17+ messages in thread From: Wenli Looi @ 2021-06-06 8:09 UTC (permalink / raw) To: Fabio M. De Francesco; +Cc: Greg Kroah-Hartman, linux-staging, linux-kernel On Sun, Jun 6, 2021 at 1:00 AM Fabio M. De Francesco <fmdefrancesco@gmail.com> 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 > > > > <gregkh@linuxfoundation.org> 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 <wlooi@ucalgary.ca> > > > > --- > > > > > > > > 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. > > > > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] staging: rtl8723bs: Fix uninitialized variable @ 2021-06-06 8:09 ` Wenli Looi 0 siblings, 0 replies; 17+ messages in thread From: Wenli Looi @ 2021-06-06 8:09 UTC (permalink / raw) To: Fabio M. De Francesco; +Cc: Greg Kroah-Hartman, linux-staging, linux-kernel On Sun, Jun 6, 2021 at 1:00 AM Fabio M. De Francesco <fmdefrancesco@gmail.com> 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 > > > > <gregkh@linuxfoundation.org> 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 <wlooi@ucalgary.ca> > > > > --- > > > > > > > > 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. > > > > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] staging: rtl8723bs: Fix uninitialized variable 2021-06-06 8:09 ` Wenli Looi (?) @ 2021-06-06 8:45 ` Fabio M. De Francesco 2021-06-06 18:46 ` [PATCH v2] " Wenli Looi -1 siblings, 1 reply; 17+ messages in thread From: Fabio M. De Francesco @ 2021-06-06 8:45 UTC (permalink / raw) To: Wenli Looi; +Cc: Greg Kroah-Hartman, linux-staging, linux-kernel 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 > > <fmdefrancesco@gmail.com> 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 > > > > > > <gregkh@linuxfoundation.org> 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 <wlooi@ucalgary.ca> > > > > > --- > > > > > > > > > > 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. ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v2] staging: rtl8723bs: Fix uninitialized variable 2021-06-06 8:45 ` Fabio M. De Francesco @ 2021-06-06 18:46 ` Wenli Looi 2021-06-07 8:35 ` Dan Carpenter 0 siblings, 1 reply; 17+ messages in thread From: Wenli Looi @ 2021-06-06 18:46 UTC (permalink / raw) To: Greg Kroah-Hartman, linux-staging, linux-kernel, Fabio M. De Francesco Cc: Wenli Looi 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 <wlooi@ucalgary.ca> --- 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 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v2] staging: rtl8723bs: Fix uninitialized variable 2021-06-06 18:46 ` [PATCH v2] " Wenli Looi @ 2021-06-07 8:35 ` Dan Carpenter 2021-06-07 8:46 ` Dan Carpenter 0 siblings, 1 reply; 17+ messages in thread From: Dan Carpenter @ 2021-06-07 8:35 UTC (permalink / raw) To: Wenli Looi Cc: Greg Kroah-Hartman, linux-staging, linux-kernel, Fabio M. De Francesco 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 <wlooi@ucalgary.ca> > --- > > 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2] staging: rtl8723bs: Fix uninitialized variable 2021-06-07 8:35 ` Dan Carpenter @ 2021-06-07 8:46 ` Dan Carpenter 2021-06-08 6:35 ` Wenli Looi 0 siblings, 1 reply; 17+ messages in thread From: Dan Carpenter @ 2021-06-07 8:46 UTC (permalink / raw) To: Wenli Looi Cc: Greg Kroah-Hartman, linux-staging, linux-kernel, Fabio M. De Francesco 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 <wlooi@ucalgary.ca> > > --- > > > > 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2] staging: rtl8723bs: Fix uninitialized variable 2021-06-07 8:46 ` Dan Carpenter @ 2021-06-08 6:35 ` Wenli Looi 0 siblings, 0 replies; 17+ messages in thread From: Wenli Looi @ 2021-06-08 6:35 UTC (permalink / raw) To: Dan Carpenter Cc: Greg Kroah-Hartman, linux-staging, linux-kernel, Fabio M. De Francesco 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 <dan.carpenter@oracle.com> 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 <wlooi@ucalgary.ca> > > > --- > > > > > > 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 > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2] staging: rtl8723bs: Fix uninitialized variable @ 2021-06-08 6:35 ` Wenli Looi 0 siblings, 0 replies; 17+ messages in thread From: Wenli Looi @ 2021-06-08 6:35 UTC (permalink / raw) To: Dan Carpenter Cc: Greg Kroah-Hartman, linux-staging, linux-kernel, Fabio M. De Francesco 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 <dan.carpenter@oracle.com> 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 <wlooi@ucalgary.ca> > > > --- > > > > > > 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 > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] staging: rtl8723bs: Fix uninitialized variable 2021-06-06 7:00 [PATCH] staging: rtl8723bs: Fix uninitialized variable Wenli Looi 2021-06-06 7:13 ` Greg Kroah-Hartman @ 2021-06-07 8:33 ` Dan Carpenter 2021-06-07 9:23 ` Greg Kroah-Hartman 2021-06-08 6:46 ` [PATCH] staging: rtl8723bs: Fix uninitialized variables Wenli Looi 1 sibling, 2 replies; 17+ messages in thread From: Dan Carpenter @ 2021-06-07 8:33 UTC (permalink / raw) To: Wenli Looi; +Cc: Greg Kroah-Hartman, linux-staging, linux-kernel 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 <wlooi@ucalgary.ca> > --- 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] staging: rtl8723bs: Fix uninitialized variable 2021-06-07 8:33 ` [PATCH] " Dan Carpenter @ 2021-06-07 9:23 ` Greg Kroah-Hartman 2021-06-07 10:43 ` Dan Carpenter 2021-06-08 6:46 ` [PATCH] staging: rtl8723bs: Fix uninitialized variables Wenli Looi 1 sibling, 1 reply; 17+ messages in thread From: Greg Kroah-Hartman @ 2021-06-07 9:23 UTC (permalink / raw) To: Dan Carpenter; +Cc: Wenli Looi, linux-staging, linux-kernel 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] staging: rtl8723bs: Fix uninitialized variable 2021-06-07 9:23 ` Greg Kroah-Hartman @ 2021-06-07 10:43 ` Dan Carpenter 0 siblings, 0 replies; 17+ messages in thread From: Dan Carpenter @ 2021-06-07 10:43 UTC (permalink / raw) To: Greg Kroah-Hartman; +Cc: Wenli Looi, linux-staging, linux-kernel 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH] staging: rtl8723bs: Fix uninitialized variables 2021-06-07 8:33 ` [PATCH] " Dan Carpenter 2021-06-07 9:23 ` Greg Kroah-Hartman @ 2021-06-08 6:46 ` Wenli Looi 2021-06-08 7:20 ` Dan Carpenter 1 sibling, 1 reply; 17+ messages in thread From: Wenli Looi @ 2021-06-08 6:46 UTC (permalink / raw) To: Dan Carpenter, Greg Kroah-Hartman, linux-staging, linux-kernel; +Cc: Wenli Looi 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 <wlooi@ucalgary.ca> --- 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 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH] staging: rtl8723bs: Fix uninitialized variables 2021-06-08 6:46 ` [PATCH] staging: rtl8723bs: Fix uninitialized variables Wenli Looi @ 2021-06-08 7:20 ` Dan Carpenter 0 siblings, 0 replies; 17+ messages in thread From: Dan Carpenter @ 2021-06-08 7:20 UTC (permalink / raw) To: Wenli Looi Cc: Greg Kroah-Hartman, linux-staging, linux-kernel, Arend van Spriel, Johannes Berg 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 <dan.carpenter@oracle.com> 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 <wlooi@ucalgary.ca> > --- > 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 > ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2021-06-08 7:20 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-06-06 7:00 [PATCH] staging: rtl8723bs: Fix uninitialized variable Wenli Looi 2021-06-06 7:13 ` Greg Kroah-Hartman 2021-06-06 7:51 ` Wenli Looi 2021-06-06 8:00 ` Fabio M. De Francesco 2021-06-06 8:09 ` Wenli Looi 2021-06-06 8:09 ` Wenli Looi 2021-06-06 8:45 ` Fabio M. De Francesco 2021-06-06 18:46 ` [PATCH v2] " Wenli Looi 2021-06-07 8:35 ` Dan Carpenter 2021-06-07 8:46 ` Dan Carpenter 2021-06-08 6:35 ` Wenli Looi 2021-06-08 6:35 ` Wenli Looi 2021-06-07 8:33 ` [PATCH] " Dan Carpenter 2021-06-07 9:23 ` Greg Kroah-Hartman 2021-06-07 10:43 ` Dan Carpenter 2021-06-08 6:46 ` [PATCH] staging: rtl8723bs: Fix uninitialized variables Wenli Looi 2021-06-08 7:20 ` Dan Carpenter
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.