[ldv-project] [net] libertas: potential race condition

Hi!

There is a potential race condition in 
drivers/net/wireless/libertas/libertas.ko.
In the function lbs_hard_start_xmit(..), line 159, a socket buffer is 
written to priv->current_skb with a spin_lock protection.
In the function lbs_mac_event_disconnected(..), lines 50-51, the field 
current_skb is cleaned. There is no protection used. The corresponding 
handlers are activated at the same time in lbs_start_card(..) and then 
may be executed simultaneously. Note, there are two structures 
lbs_netdev_ops and mesh_netdev_ops, which have the target handler 
lbs_hard_start_xmit.
Is it a real race or I have missed something?

-- 
Pavel Andrianov
Linux Verification Center, ISPRAS
web: http://linuxtesting.org
e-mail: andrianov@ispras.ru

On Tue, 2016-06-07 at 13:30 +0400, Pavel Andrianov wrote:

Hi!

There is a potential race condition in 
drivers/net/wireless/libertas/libertas.ko.
In the function lbs_hard_start_xmit(..), line 159, a socket buffer
is 
written to priv->current_skb with a spin_lock protection.
In the function lbs_mac_event_disconnected(..), lines 50-51, the
field 
current_skb is cleaned. There is no protection used. The
corresponding 
handlers are activated at the same time in lbs_start_card(..) and
then 
may be executed simultaneously. Note, there are two structures 
lbs_netdev_ops and mesh_netdev_ops, which have the target handler 
lbs_hard_start_xmit.
Is it a real race or I have missed something?

Yeah, it looks like it should be grabbing priv->driver_lock before
clearing priv->currenttxskb in lbs_mac_event_disconnected().  Care to
submit a patch after testing?  Do you have any of that hardware?

Dan

07.06.2016 18:39, Dan Williams пишет:

On Tue, 2016-06-07 at 13:30 +0400, Pavel Andrianov wrote:

quoted

Hi!

There is a potential race condition in
drivers/net/wireless/libertas/libertas.ko.
In the function lbs_hard_start_xmit(..), line 159, a socket buffer
is
written to priv->current_skb with a spin_lock protection.
In the function lbs_mac_event_disconnected(..), lines 50-51, the
field
current_skb is cleaned. There is no protection used. The
corresponding
handlers are activated at the same time in lbs_start_card(..) and
then
may be executed simultaneously. Note, there are two structures
lbs_netdev_ops and mesh_netdev_ops, which have the target handler
lbs_hard_start_xmit.
Is it a real race or I have missed something?

Yeah, it looks like it should be grabbing priv->driver_lock before
clearing priv->currenttxskb in lbs_mac_event_disconnected().  Care to
submit a patch after testing?  Do you have any of that hardware?

Dan

I have no that hardware and I have some doubts about the simple fix, 
you've suggested. For instance, in lbs_hard_start_xmit the lock is 
acquired twice and the priv->tx_pending_len can be modified also by 
lbs_mac_event_disconnected (even if spin_lock will be added to 
lbs_mac_event_disconnected). Moreover, the function lbs_send_tx_feedback 
also cleaned priv->currenttxskb, but it happens also without any 
protection. Thus, the fix has to be more complicated, and I have no 
ideas about it.

-- 
Pavel Andrianov
Linux Verification Center, ISPRAS
web: http://linuxtesting.org
e-mail: andrianov-ufN2psIa012HXe+LvDLADg@public.gmane.org

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Tue, Jun 07, 2016 at 09:39:55AM -0500, Dan Williams wrote:

On Tue, 2016-06-07 at 13:30 +0400, Pavel Andrianov wrote:

quoted

Hi!

There is a potential race condition in 
drivers/net/wireless/libertas/libertas.ko.
In the function lbs_hard_start_xmit(..), line 159, a socket buffer
is 
written to priv->current_skb with a spin_lock protection.
In the function lbs_mac_event_disconnected(..), lines 50-51, the
field 
current_skb is cleaned. There is no protection used. The
corresponding 
handlers are activated at the same time in lbs_start_card(..) and
then 
may be executed simultaneously. Note, there are two structures 
lbs_netdev_ops and mesh_netdev_ops, which have the target handler 
lbs_hard_start_xmit.
Is it a real race or I have missed something?

Yeah, it looks like it should be grabbing priv->driver_lock before
clearing priv->currenttxskb in lbs_mac_event_disconnected().  Care to
submit a patch after testing?  Do you have any of that hardware?

I've hardware, with serial console.

Can test any patch, on USB (8388) or SDIO (8686).

-- 
James Cameron
http://quozl.netrek.org/

08.06.2016 02:51, James Cameron пишет:

On Tue, Jun 07, 2016 at 09:39:55AM -0500, Dan Williams wrote:

quoted

On Tue, 2016-06-07 at 13:30 +0400, Pavel Andrianov wrote:

quoted

Hi!

There is a potential race condition in
drivers/net/wireless/libertas/libertas.ko.
In the function lbs_hard_start_xmit(..), line 159, a socket buffer
is
written to priv->current_skb with a spin_lock protection.
In the function lbs_mac_event_disconnected(..), lines 50-51, the
field
current_skb is cleaned. There is no protection used. The
corresponding
handlers are activated at the same time in lbs_start_card(..) and
then
may be executed simultaneously. Note, there are two structures
lbs_netdev_ops and mesh_netdev_ops, which have the target handler
lbs_hard_start_xmit.
Is it a real race or I have missed something?

Yeah, it looks like it should be grabbing priv->driver_lock before
clearing priv->currenttxskb in lbs_mac_event_disconnected().  Care to
submit a patch after testing?  Do you have any of that hardware?

I've hardware, with serial console.

Can test any patch, on USB (8388) or SDIO (8686).

Hi!

I've prepare the patch for this issue. Could you test it?

Thank you.

-- 
Pavel Andrianov
Linux Verification Center, ISPRAS
web: http://linuxtesting.org
e-mail: andrianov@ispras.ru

On Tue, Jun 14, 2016 at 05:16:11PM +0400, Pavel Andrianov wrote:

08.06.2016 02:51, James Cameron пишет:

quoted

On Tue, Jun 07, 2016 at 09:39:55AM -0500, Dan Williams wrote:

quoted

On Tue, 2016-06-07 at 13:30 +0400, Pavel Andrianov wrote:

quoted

Hi!

There is a potential race condition in
drivers/net/wireless/libertas/libertas.ko.
In the function lbs_hard_start_xmit(..), line 159, a socket buffer
is
written to priv->current_skb with a spin_lock protection.
In the function lbs_mac_event_disconnected(..), lines 50-51, the
field
current_skb is cleaned. There is no protection used. The
corresponding
handlers are activated at the same time in lbs_start_card(..) and
then
may be executed simultaneously. Note, there are two structures
lbs_netdev_ops and mesh_netdev_ops, which have the target handler
lbs_hard_start_xmit.
Is it a real race or I have missed something?

Yeah, it looks like it should be grabbing priv->driver_lock before
clearing priv->currenttxskb in lbs_mac_event_disconnected().  Care to
submit a patch after testing?  Do you have any of that hardware?

I've hardware, with serial console.

Can test any patch, on USB (8388) or SDIO (8686).

Hi!

I've prepare the patch for this issue. Could you test it?

Thank you.

Tested on OLPC XO-1 (usb8388) and XO-1.5 (sd8686) with v4.7-rc3.

Confirmed that lbs_mac_event_disconnected is being called on the
station when hostapd on access point is given SIGHUP.

Longer duration test was;

- SSH to station and run "top -d 0.2",

- send SIGHUP every six seconds, for 300 cycles,

You may add my;

Tested-by: James Cameron <redacted>

-- 
James Cameron
http://quozl.netrek.org/