[PATCH v3] nl80211: take RCU read lock when calling ieee80211_bss_get_ie()

[Dominik Brodowski]

As ieee80211_bss_get_ie() derefences an RCU to return ssid_ie, both
the call to this function and any operation on this variable need
protection by the RCU read lock.

Fixes: 44905265bc15 ("nl80211: don't expose wdev->ssid for most interfaces")
Signed-off-by: Dominik Brodowski 
---

> but after, perhaps it's easier to just do
> 
>   if (ssid_ie &&
>   nla_put(...)
>   goto nla_put_failure_rcu_locked;
> 
> and avoid the extra label (but yeah, it's getting late)

OK, done that (and updated the commit message), and testet it.

Thanks,
Dominik

diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 2b3dbcd40e46..ed87a97fcb0b 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -2618,12 +2618,13 @@ static int nl80211_send_iface(struct sk_buff *msg, u32 
portid, u32 seq, int flag
const u8 *ssid_ie;
if (!wdev->current_bss)
break;
+   rcu_read_lock();
ssid_ie = ieee80211_bss_get_ie(>current_bss->pub,
   WLAN_EID_SSID);
-   if (!ssid_ie)
-   break;
-   if (nla_put(msg, NL80211_ATTR_SSID, ssid_ie[1], ssid_ie + 2))
-   goto nla_put_failure_locked;
+   if (ssid_ie &&
+   nla_put(msg, NL80211_ATTR_SSID, ssid_ie[1], ssid_ie + 2))
+   goto nla_put_failure_rcu_locked;
+   rcu_read_unlock();
break;
}
default:
@@ -2635,6 +2636,8 @@ static int nl80211_send_iface(struct sk_buff *msg, u32 
portid, u32 seq, int flag
genlmsg_end(msg, hdr);
return 0;
 
+ nla_put_failure_rcu_locked:
+   rcu_read_unlock();
  nla_put_failure_locked:
wdev_unlock(wdev);
  nla_put_failure:

Re: [PATCH v2] nl80211: take RCU read lock when calling ieee80211_bss_get_ie()

[Johannes Berg]

On Sun, 2018-01-14 at 23:22 +0100, Dominik Brodowski wrote:
> 
> + rcu_read_lock();
>   ssid_ie = ieee80211_bss_get_ie(>current_bss->pub,
>  WLAN_EID_SSID);
>   if (!ssid_ie)
> - break;

nit-picking now: that "break" here may have been easier before these
changes

> + goto nla_rcu_unlock;
>   if (nla_put(msg, NL80211_ATTR_SSID, ssid_ie[1], ssid_ie + 2))
> - goto nla_put_failure_locked;
> + goto nla_put_failure_rcu_locked;
> + nla_rcu_unlock:
> + rcu_read_unlock();
>   break;

but after, perhaps it's easier to just do

if (ssid_ie &&
nla_put(...)
goto nla_put_failure_rcu_locked;

and avoid the extra label (but yeah, it's getting late)

johannes

[PATCH v2] nl80211: take RCU read lock when calling ieee80211_bss_get_ie()

[Dominik Brodowski]

As ieee80211_bss_get_ie() derefences an RCU, it needs to be called with
rcu_read_lock held.

Fixes: 44905265bc15 ("nl80211: don't expose wdev->ssid for most interfaces")
Signed-off-by: Dominik Brodowski 
---

> This uses the ssid_ie, so that doesn't really seem right? The
> protection should extend beyond the usage.

Indeed -- I had misread the code and hadn't thought of ssid_ie also needing
the protection during its lifetime. So here's a new version 2 -- which I
will only be able to test tomorrow, though...

Thanks,
Dominik

diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 2b3dbcd40e46..b53bd8db7974 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -2618,12 +2618,15 @@ static int nl80211_send_iface(struct sk_buff *msg, u32 
portid, u32 seq, int flag
const u8 *ssid_ie;
if (!wdev->current_bss)
break;
+   rcu_read_lock();
ssid_ie = ieee80211_bss_get_ie(>current_bss->pub,
   WLAN_EID_SSID);
if (!ssid_ie)
-   break;
+   goto nla_rcu_unlock;
if (nla_put(msg, NL80211_ATTR_SSID, ssid_ie[1], ssid_ie + 2))
-   goto nla_put_failure_locked;
+   goto nla_put_failure_rcu_locked;
+ nla_rcu_unlock:
+   rcu_read_unlock();
break;
}
default:
@@ -2635,6 +2638,8 @@ static int nl80211_send_iface(struct sk_buff *msg, u32 
portid, u32 seq, int flag
genlmsg_end(msg, hdr);
return 0;
 
+ nla_put_failure_rcu_locked:
+   rcu_read_unlock();
  nla_put_failure_locked:
wdev_unlock(wdev);
  nla_put_failure:

Re: [PATCH] nl80211: take RCU read lock when calling ieee80211_bss_get_ie()

[Johannes Berg]

Hi,

> Fixes: 44905265bc15 ("nl80211: don't expose wdev->ssid for most interfaces")
> Signed-off-by: Dominik Brodowski 
> ---
> 
> This patch fixes the regression I reported in the last couple of weeks for
> various v4.15-rcX revisions to netdev, where a "suspicious RCU usage"
> showed up in net/wireless/util.c:778.

Huh. You should added linux-wireless to those reports, I simply didn't
see them!

> diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
> index 2b3dbcd40e46..1eecc249fb5e 100644
> --- a/net/wireless/nl80211.c
> +++ b/net/wireless/nl80211.c
> @@ -2618,8 +2618,10 @@ static int nl80211_send_iface(struct sk_buff *msg, u32 
> portid, u32 seq, int flag
>   const u8 *ssid_ie;
>   if (!wdev->current_bss)
>   break;
> + rcu_read_lock();
>   ssid_ie = ieee80211_bss_get_ie(>current_bss->pub,
>  WLAN_EID_SSID);
> + rcu_read_unlock();
>   if (!ssid_ie)
>   break;
>   if (nla_put(msg, NL80211_ATTR_SSID, ssid_ie[1], ssid_ie + 2))

This uses the ssid_ie, so that doesn't really seem right? The
protection should extend beyond the usage.

johannes

Re: [PATCH] bcma: Fix 'allmodconfig' and BCMA builds on MIPS targets

[Guenter Roeck]

[ copying linux-mips ]

On 01/14/2018 01:34 PM, Guenter Roeck wrote:

Mips builds with BCMA host mode enabled fail in mainline and -next
with:

In file included from include/linux/bcma/bcma.h:10:0,
  from drivers/bcma/bcma_private.h:9,
 from drivers/bcma/main.c:8:
include/linux/bcma/bcma_driver_pci.h:218:24: error:
field 'pci_controller' has incomplete type

Bisect points to commit d41e6858ba58c ("MIPS: Kconfig: Set default MIPS
system type as generic") as the culprit. Analysis shows that the commmit
changes PCI configuration and enables PCI_DRIVERS_GENERIC. This in turn
disables PCI_DRIVERS_LEGACY. 'struct pci_controller' is, however, only
defined if PCI_DRIVERS_LEGACY is enabled.

Ultimately that means that BCMA_DRIVER_PCI_HOSTMODE depends on
PCI_DRIVERS_LEGACY. Add the missing dependency.

Fixes: d41e6858ba58c ("MIPS: Kconfig: Set default MIPS system type as ...")
Cc: Matt Redfearn 
Cc: James Hogan 
Signed-off-by: Guenter Roeck 
---
I am aware that this problem has been reported several times. I have
not been able to find a fix, but I may have missed it. If so, my
apologies for the noise.


I should have said "I have not been able to find a patch fixing it".


Also note that this is not the only fix required; commit d41e6858ba58c,
as simple as it looks like, does a pretty good job messing up
"mips:allmodconfig" builds.


... nor did I find patch(es) fixing the other build problem(s) introduced
by d41e6858ba58c.

Guenter


  drivers/bcma/Kconfig | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/bcma/Kconfig b/drivers/bcma/Kconfig
index 02d78f6cecbb..ba8acca036df 100644
--- a/drivers/bcma/Kconfig
+++ b/drivers/bcma/Kconfig
@@ -55,7 +55,7 @@ config BCMA_DRIVER_PCI
  
  config BCMA_DRIVER_PCI_HOSTMODE

bool "Driver for PCI core working in hostmode"
-   depends on MIPS && BCMA_DRIVER_PCI
+   depends on MIPS && BCMA_DRIVER_PCI && PCI_DRIVERS_LEGACY
help
  PCI core hostmode operation (external PCI bus).
  

[PATCH] bcma: Fix 'allmodconfig' and BCMA builds on MIPS targets

[Guenter Roeck]

Mips builds with BCMA host mode enabled fail in mainline and -next
with:

In file included from include/linux/bcma/bcma.h:10:0,
 from drivers/bcma/bcma_private.h:9,
 from drivers/bcma/main.c:8:
include/linux/bcma/bcma_driver_pci.h:218:24: error:
field 'pci_controller' has incomplete type

Bisect points to commit d41e6858ba58c ("MIPS: Kconfig: Set default MIPS
system type as generic") as the culprit. Analysis shows that the commmit
changes PCI configuration and enables PCI_DRIVERS_GENERIC. This in turn
disables PCI_DRIVERS_LEGACY. 'struct pci_controller' is, however, only
defined if PCI_DRIVERS_LEGACY is enabled.

Ultimately that means that BCMA_DRIVER_PCI_HOSTMODE depends on
PCI_DRIVERS_LEGACY. Add the missing dependency.

Fixes: d41e6858ba58c ("MIPS: Kconfig: Set default MIPS system type as ...")
Cc: Matt Redfearn 
Cc: James Hogan 
Signed-off-by: Guenter Roeck 
---
I am aware that this problem has been reported several times. I have
not been able to find a fix, but I may have missed it. If so, my
apologies for the noise.

Also note that this is not the only fix required; commit d41e6858ba58c,
as simple as it looks like, does a pretty good job messing up
"mips:allmodconfig" builds.

 drivers/bcma/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/bcma/Kconfig b/drivers/bcma/Kconfig
index 02d78f6cecbb..ba8acca036df 100644
--- a/drivers/bcma/Kconfig
+++ b/drivers/bcma/Kconfig
@@ -55,7 +55,7 @@ config BCMA_DRIVER_PCI
 
 config BCMA_DRIVER_PCI_HOSTMODE
bool "Driver for PCI core working in hostmode"
-   depends on MIPS && BCMA_DRIVER_PCI
+   depends on MIPS && BCMA_DRIVER_PCI && PCI_DRIVERS_LEGACY
help
  PCI core hostmode operation (external PCI bus).
 
-- 
2.7.4

[PATCH] nl80211: take RCU read lock when calling ieee80211_bss_get_ie()

[Dominik Brodowski]

As ieee80211_bss_get_ie() derefences an RCU, it needs to be called with
rcu_read_lock held.

Fixes: 44905265bc15 ("nl80211: don't expose wdev->ssid for most interfaces")
Signed-off-by: Dominik Brodowski 
---

This patch fixes the regression I reported in the last couple of weeks for
various v4.15-rcX revisions to netdev, where a "suspicious RCU usage"
showed up in net/wireless/util.c:778.

diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 2b3dbcd40e46..1eecc249fb5e 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -2618,8 +2618,10 @@ static int nl80211_send_iface(struct sk_buff *msg, u32 
portid, u32 seq, int flag
const u8 *ssid_ie;
if (!wdev->current_bss)
break;
+   rcu_read_lock();
ssid_ie = ieee80211_bss_get_ie(>current_bss->pub,
   WLAN_EID_SSID);
+   rcu_read_unlock();
if (!ssid_ie)
break;
if (nla_put(msg, NL80211_ATTR_SSID, ssid_ie[1], ssid_ie + 2))

Re: [PATCH 0/5] iwlwifi: updates intended for v4.16 2017-12-23

[Kalle Valo]

"Grumbach, Emmanuel"  writes:

> On Fri, 2018-01-12 at 13:31 +0200, Kalle Valo wrote:
>> From: Luca Coelho 
>>
>> > Luca Coelho (1):
>> >   iwlwifi: mvm: check if mac80211_queue is valid in
>> > iwl_mvm_disable_txq
>> > 
>> > Mordechay Goodstein (1):
>> >   iwlwifi: set default timstamp marker cmd
>> > 
>> > Sara Sharon (3):
>> >   iwlwifi: mvm: flip AMSDU addresses only for 9000 family
>> >   iwlwifi: mvm: take RCU lock before dereferencing
>> >   iwlwifi: mvm: move TSO segment to a separate function
>> 
>> Most likely the merge window starts in 9 days so I need to start
>> getting patches ready. As Luca is away should I apply these patches
>> directly to wireless-drivers-next?
>> 
>
> First let me thank you for tracking this. I don't see anything that we
> really need in 4.16 here. Besides Luca's UBSAN warning, all the rest
> is for a new device family which won't be supported in 4.16 anyway.
>
> So I opt to wait for Luca here.

Ok, I'll wait for him then.

-- 
Kalle Valo