Re: [PATCH] net: nvidia: forcedeth: Fix two possible concurrency use-after-free bugs
From: Jia-Ju Bai Date: Tue, 8 Jan 2019 20:45:18 +0800 > In drivers/net/ethernet/nvidia/forcedeth.c, the functions > nv_start_xmit() and nv_start_xmit_optimized() can be concurrently > executed with nv_poll_controller(). > > nv_start_xmit > line 2321: prev_tx_ctx->skb = skb; > > nv_start_xmit_optimized > line 2479: prev_tx_ctx->skb = skb; > > nv_poll_controller > nv_do_nic_poll > line 4134: spin_lock(&np->lock); > nv_drain_rxtx > nv_drain_tx > nv_release_txskb > line 2004: dev_kfree_skb_any(tx_skb->skb); > > Thus, two possible concurrency use-after-free bugs may occur. I do not think so, the netif_tx_lock_bh() done will prevent the parallel execution.
Re: [PATCH] net: nvidia: forcedeth: Fix two possible concurrency use-after-free bugs
On 2019/1/9 11:24, Yanjun Zhu wrote: If you have forcedeth NIC, you can make tests with it.:-) Ah, I would like to, but I do not have the hardware... Best wishes, Jia-Ju Bai
Re: [PATCH] net: nvidia: forcedeth: Fix two possible concurrency use-after-free bugs
On 2019/1/9 11:20, Jia-Ju Bai wrote:
On 2019/1/9 10:35, Yanjun Zhu wrote:
On 2019/1/9 10:03, Jia-Ju Bai wrote:
On 2019/1/9 9:24, Yanjun Zhu wrote:
On 2019/1/8 20:57, Jia-Ju Bai wrote:
On 2019/1/8 20:54, Zhu Yanjun wrote:
在 2019/1/8 20:45, Jia-Ju Bai 写道:
In drivers/net/ethernet/nvidia/forcedeth.c, the functions
nv_start_xmit() and nv_start_xmit_optimized() can be concurrently
executed with nv_poll_controller().
nv_start_xmit
line 2321: prev_tx_ctx->skb = skb;
nv_start_xmit_optimized
line 2479: prev_tx_ctx->skb = skb;
nv_poll_controller
nv_do_nic_poll
line 4134: spin_lock(&np->lock);
nv_drain_rxtx
nv_drain_tx
nv_release_txskb
line 2004: dev_kfree_skb_any(tx_skb->skb);
Thus, two possible concurrency use-after-free bugs may occur.
To fix these possible bugs,
Does this really occur? Can you reproduce this ?
This bug is not found by the real execution.
It is found by a static tool written by myself, and then I check
it by manual code review.
Before "line 2004: dev_kfree_skb_any(tx_skb->skb); ",
"
nv_disable_irq(dev);
nv_napi_disable(dev);
netif_tx_lock_bh(dev);
netif_addr_lock(dev);
spin_lock(&np->lock);
/* stop engines */
nv_stop_rxtx(dev); <---this stop rxtx
nv_txrx_reset(dev);
"
In this case, does nv_start_xmit or nv_start_xmit_optimized still
work well?
nv_stop_rxtx() calls nv_stop_tx(dev).
static void nv_stop_tx(struct net_device *dev)
{
struct fe_priv *np = netdev_priv(dev);
u8 __iomem *base = get_hwbase(dev);
u32 tx_ctrl = readl(base + NvRegTransmitterControl);
if (!np->mac_in_use)
tx_ctrl &= ~NVREG_XMITCTL_START;
else
tx_ctrl |= NVREG_XMITCTL_TX_PATH_EN;
writel(tx_ctrl, base + NvRegTransmitterControl);
if (reg_delay(dev, NvRegTransmitterStatus, NVREG_XMITSTAT_BUSY, 0,
NV_TXSTOP_DELAY1, NV_TXSTOP_DELAY1MAX))
netdev_info(dev, "%s: TransmitterStatus remained busy\n",
__func__);
udelay(NV_TXSTOP_DELAY2);
if (!np->mac_in_use)
writel(readl(base + NvRegTransmitPoll) &
NVREG_TRANSMITPOLL_MAC_ADDR_REV,
base + NvRegTransmitPoll);
}
nv_stop_tx() seems to only write registers to stop transmitting for
hardware.
But it does not wait until nv_start_xmit() and
nv_start_xmit_optimized() finish execution.
There are 3 modes in forcedeth NIC.
In throughput mode (0), every tx & rx packet will generate an interrupt.
In CPU mode (1), interrupts are controlled by a timer.
In dynamic mode (2), the mode toggles between throughput and CPU mode
based on network load.
From the source code,
"np->recover_error = 1;" is related with CPU mode.
nv_start_xmit or nv_start_xmit_optimized seems related with
ghroughput mode.
In static void nv_do_nic_poll(struct timer_list *t),
when if (np->recover_error), line 2004:
dev_kfree_skb_any(tx_skb->skb); will run.
When "np->recover_error=1", do you think nv_start_xmit or
nv_start_xmit_optimized will be called?
Sorry, I do not know about these modes...
But I still think nv_start_xmit() or nv_start_xmit_optimized() can be
called here, in no matter which mode :)
:-P
If you have forcedeth NIC, you can make tests with it.:-)
Best wishes,
Jia-Ju Bai
Re: [PATCH] net: nvidia: forcedeth: Fix two possible concurrency use-after-free bugs
On 2019/1/9 10:35, Yanjun Zhu wrote:
On 2019/1/9 10:03, Jia-Ju Bai wrote:
On 2019/1/9 9:24, Yanjun Zhu wrote:
On 2019/1/8 20:57, Jia-Ju Bai wrote:
On 2019/1/8 20:54, Zhu Yanjun wrote:
在 2019/1/8 20:45, Jia-Ju Bai 写道:
In drivers/net/ethernet/nvidia/forcedeth.c, the functions
nv_start_xmit() and nv_start_xmit_optimized() can be concurrently
executed with nv_poll_controller().
nv_start_xmit
line 2321: prev_tx_ctx->skb = skb;
nv_start_xmit_optimized
line 2479: prev_tx_ctx->skb = skb;
nv_poll_controller
nv_do_nic_poll
line 4134: spin_lock(&np->lock);
nv_drain_rxtx
nv_drain_tx
nv_release_txskb
line 2004: dev_kfree_skb_any(tx_skb->skb);
Thus, two possible concurrency use-after-free bugs may occur.
To fix these possible bugs,
Does this really occur? Can you reproduce this ?
This bug is not found by the real execution.
It is found by a static tool written by myself, and then I check it
by manual code review.
Before "line 2004: dev_kfree_skb_any(tx_skb->skb); ",
"
nv_disable_irq(dev);
nv_napi_disable(dev);
netif_tx_lock_bh(dev);
netif_addr_lock(dev);
spin_lock(&np->lock);
/* stop engines */
nv_stop_rxtx(dev); <---this stop rxtx
nv_txrx_reset(dev);
"
In this case, does nv_start_xmit or nv_start_xmit_optimized still
work well?
nv_stop_rxtx() calls nv_stop_tx(dev).
static void nv_stop_tx(struct net_device *dev)
{
struct fe_priv *np = netdev_priv(dev);
u8 __iomem *base = get_hwbase(dev);
u32 tx_ctrl = readl(base + NvRegTransmitterControl);
if (!np->mac_in_use)
tx_ctrl &= ~NVREG_XMITCTL_START;
else
tx_ctrl |= NVREG_XMITCTL_TX_PATH_EN;
writel(tx_ctrl, base + NvRegTransmitterControl);
if (reg_delay(dev, NvRegTransmitterStatus, NVREG_XMITSTAT_BUSY, 0,
NV_TXSTOP_DELAY1, NV_TXSTOP_DELAY1MAX))
netdev_info(dev, "%s: TransmitterStatus remained busy\n",
__func__);
udelay(NV_TXSTOP_DELAY2);
if (!np->mac_in_use)
writel(readl(base + NvRegTransmitPoll) &
NVREG_TRANSMITPOLL_MAC_ADDR_REV,
base + NvRegTransmitPoll);
}
nv_stop_tx() seems to only write registers to stop transmitting for
hardware.
But it does not wait until nv_start_xmit() and
nv_start_xmit_optimized() finish execution.
There are 3 modes in forcedeth NIC.
In throughput mode (0), every tx & rx packet will generate an interrupt.
In CPU mode (1), interrupts are controlled by a timer.
In dynamic mode (2), the mode toggles between throughput and CPU mode
based on network load.
From the source code,
"np->recover_error = 1;" is related with CPU mode.
nv_start_xmit or nv_start_xmit_optimized seems related with ghroughput
mode.
In static void nv_do_nic_poll(struct timer_list *t),
when if (np->recover_error), line 2004:
dev_kfree_skb_any(tx_skb->skb); will run.
When "np->recover_error=1", do you think nv_start_xmit or
nv_start_xmit_optimized will be called?
Sorry, I do not know about these modes...
But I still think nv_start_xmit() or nv_start_xmit_optimized() can be
called here, in no matter which mode :)
Best wishes,
Jia-Ju Bai
Re: [PATCH] net: nvidia: forcedeth: Fix two possible concurrency use-after-free bugs
On 2019/1/9 10:03, Jia-Ju Bai wrote:
On 2019/1/9 9:24, Yanjun Zhu wrote:
On 2019/1/8 20:57, Jia-Ju Bai wrote:
On 2019/1/8 20:54, Zhu Yanjun wrote:
在 2019/1/8 20:45, Jia-Ju Bai 写道:
In drivers/net/ethernet/nvidia/forcedeth.c, the functions
nv_start_xmit() and nv_start_xmit_optimized() can be concurrently
executed with nv_poll_controller().
nv_start_xmit
line 2321: prev_tx_ctx->skb = skb;
nv_start_xmit_optimized
line 2479: prev_tx_ctx->skb = skb;
nv_poll_controller
nv_do_nic_poll
line 4134: spin_lock(&np->lock);
nv_drain_rxtx
nv_drain_tx
nv_release_txskb
line 2004: dev_kfree_skb_any(tx_skb->skb);
Thus, two possible concurrency use-after-free bugs may occur.
To fix these possible bugs,
Does this really occur? Can you reproduce this ?
This bug is not found by the real execution.
It is found by a static tool written by myself, and then I check it
by manual code review.
Before "line 2004: dev_kfree_skb_any(tx_skb->skb); ",
"
nv_disable_irq(dev);
nv_napi_disable(dev);
netif_tx_lock_bh(dev);
netif_addr_lock(dev);
spin_lock(&np->lock);
/* stop engines */
nv_stop_rxtx(dev); <---this stop rxtx
nv_txrx_reset(dev);
"
In this case, does nv_start_xmit or nv_start_xmit_optimized still
work well?
nv_stop_rxtx() calls nv_stop_tx(dev).
static void nv_stop_tx(struct net_device *dev)
{
struct fe_priv *np = netdev_priv(dev);
u8 __iomem *base = get_hwbase(dev);
u32 tx_ctrl = readl(base + NvRegTransmitterControl);
if (!np->mac_in_use)
tx_ctrl &= ~NVREG_XMITCTL_START;
else
tx_ctrl |= NVREG_XMITCTL_TX_PATH_EN;
writel(tx_ctrl, base + NvRegTransmitterControl);
if (reg_delay(dev, NvRegTransmitterStatus, NVREG_XMITSTAT_BUSY, 0,
NV_TXSTOP_DELAY1, NV_TXSTOP_DELAY1MAX))
netdev_info(dev, "%s: TransmitterStatus remained busy\n",
__func__);
udelay(NV_TXSTOP_DELAY2);
if (!np->mac_in_use)
writel(readl(base + NvRegTransmitPoll) &
NVREG_TRANSMITPOLL_MAC_ADDR_REV,
base + NvRegTransmitPoll);
}
nv_stop_tx() seems to only write registers to stop transmitting for
hardware.
But it does not wait until nv_start_xmit() and
nv_start_xmit_optimized() finish execution.
There are 3 modes in forcedeth NIC.
In throughput mode (0), every tx & rx packet will generate an interrupt.
In CPU mode (1), interrupts are controlled by a timer.
In dynamic mode (2), the mode toggles between throughput and CPU mode
based on network load.
From the source code,
"np->recover_error = 1;" is related with CPU mode.
nv_start_xmit or nv_start_xmit_optimized seems related with ghroughput mode.
In static void nv_do_nic_poll(struct timer_list *t),
when if (np->recover_error), line 2004: dev_kfree_skb_any(tx_skb->skb);
will run.
When "np->recover_error=1", do you think nv_start_xmit or
nv_start_xmit_optimized will be called?
Maybe netif_stop_queue() should be used here to stop transmitting for
network layer, but this function does not seem to wait, either.
Do you know any function that can wait until ".ndo_start_xmit" finish
execution?
Best wishes,
Jia-Ju Bai
Re: [PATCH] net: nvidia: forcedeth: Fix two possible concurrency use-after-free bugs
On 2019/1/9 9:24, Yanjun Zhu wrote:
On 2019/1/8 20:57, Jia-Ju Bai wrote:
On 2019/1/8 20:54, Zhu Yanjun wrote:
在 2019/1/8 20:45, Jia-Ju Bai 写道:
In drivers/net/ethernet/nvidia/forcedeth.c, the functions
nv_start_xmit() and nv_start_xmit_optimized() can be concurrently
executed with nv_poll_controller().
nv_start_xmit
line 2321: prev_tx_ctx->skb = skb;
nv_start_xmit_optimized
line 2479: prev_tx_ctx->skb = skb;
nv_poll_controller
nv_do_nic_poll
line 4134: spin_lock(&np->lock);
nv_drain_rxtx
nv_drain_tx
nv_release_txskb
line 2004: dev_kfree_skb_any(tx_skb->skb);
Thus, two possible concurrency use-after-free bugs may occur.
To fix these possible bugs,
Does this really occur? Can you reproduce this ?
This bug is not found by the real execution.
It is found by a static tool written by myself, and then I check it
by manual code review.
Before "line 2004: dev_kfree_skb_any(tx_skb->skb); ",
"
nv_disable_irq(dev);
nv_napi_disable(dev);
netif_tx_lock_bh(dev);
netif_addr_lock(dev);
spin_lock(&np->lock);
/* stop engines */
nv_stop_rxtx(dev); <---this stop rxtx
nv_txrx_reset(dev);
"
In this case, does nv_start_xmit or nv_start_xmit_optimized still work
well?
nv_stop_rxtx() calls nv_stop_tx(dev).
static void nv_stop_tx(struct net_device *dev)
{
struct fe_priv *np = netdev_priv(dev);
u8 __iomem *base = get_hwbase(dev);
u32 tx_ctrl = readl(base + NvRegTransmitterControl);
if (!np->mac_in_use)
tx_ctrl &= ~NVREG_XMITCTL_START;
else
tx_ctrl |= NVREG_XMITCTL_TX_PATH_EN;
writel(tx_ctrl, base + NvRegTransmitterControl);
if (reg_delay(dev, NvRegTransmitterStatus, NVREG_XMITSTAT_BUSY, 0,
NV_TXSTOP_DELAY1, NV_TXSTOP_DELAY1MAX))
netdev_info(dev, "%s: TransmitterStatus remained busy\n",
__func__);
udelay(NV_TXSTOP_DELAY2);
if (!np->mac_in_use)
writel(readl(base + NvRegTransmitPoll) &
NVREG_TRANSMITPOLL_MAC_ADDR_REV,
base + NvRegTransmitPoll);
}
nv_stop_tx() seems to only write registers to stop transmitting for
hardware.
But it does not wait until nv_start_xmit() and nv_start_xmit_optimized()
finish execution.
Maybe netif_stop_queue() should be used here to stop transmitting for
network layer, but this function does not seem to wait, either.
Do you know any function that can wait until ".ndo_start_xmit" finish
execution?
Best wishes,
Jia-Ju Bai
Re: [PATCH] net: nvidia: forcedeth: Fix two possible concurrency use-after-free bugs
On 2019/1/8 20:57, Jia-Ju Bai wrote: On 2019/1/8 20:54, Zhu Yanjun wrote: 在 2019/1/8 20:45, Jia-Ju Bai 写道: In drivers/net/ethernet/nvidia/forcedeth.c, the functions nv_start_xmit() and nv_start_xmit_optimized() can be concurrently executed with nv_poll_controller(). nv_start_xmit line 2321: prev_tx_ctx->skb = skb; nv_start_xmit_optimized line 2479: prev_tx_ctx->skb = skb; nv_poll_controller nv_do_nic_poll line 4134: spin_lock(&np->lock); nv_drain_rxtx nv_drain_tx nv_release_txskb line 2004: dev_kfree_skb_any(tx_skb->skb); Thus, two possible concurrency use-after-free bugs may occur. To fix these possible bugs, Does this really occur? Can you reproduce this ? This bug is not found by the real execution. It is found by a static tool written by myself, and then I check it by manual code review. Before "line 2004: dev_kfree_skb_any(tx_skb->skb); ", " nv_disable_irq(dev); nv_napi_disable(dev); netif_tx_lock_bh(dev); netif_addr_lock(dev); spin_lock(&np->lock); /* stop engines */ nv_stop_rxtx(dev); <---this stop rxtx nv_txrx_reset(dev); " In this case, does nv_start_xmit or nv_start_xmit_optimized still work well? Zhu Yanjun Best wishes, Jia-Ju Bai
Re: [PATCH] net: nvidia: forcedeth: Fix two possible concurrency use-after-free bugs
On 2019/1/8 20:54, Zhu Yanjun wrote: 在 2019/1/8 20:45, Jia-Ju Bai 写道: In drivers/net/ethernet/nvidia/forcedeth.c, the functions nv_start_xmit() and nv_start_xmit_optimized() can be concurrently executed with nv_poll_controller(). nv_start_xmit line 2321: prev_tx_ctx->skb = skb; nv_start_xmit_optimized line 2479: prev_tx_ctx->skb = skb; nv_poll_controller nv_do_nic_poll line 4134: spin_lock(&np->lock); nv_drain_rxtx nv_drain_tx nv_release_txskb line 2004: dev_kfree_skb_any(tx_skb->skb); Thus, two possible concurrency use-after-free bugs may occur. To fix these possible bugs, Does this really occur? Can you reproduce this ? This bug is not found by the real execution. It is found by a static tool written by myself, and then I check it by manual code review. Best wishes, Jia-Ju Bai
Re: [PATCH] net: nvidia: forcedeth: Fix two possible concurrency use-after-free bugs
在 2019/1/8 20:45, Jia-Ju Bai 写道:
In drivers/net/ethernet/nvidia/forcedeth.c, the functions
nv_start_xmit() and nv_start_xmit_optimized() can be concurrently
executed with nv_poll_controller().
nv_start_xmit
line 2321: prev_tx_ctx->skb = skb;
nv_start_xmit_optimized
line 2479: prev_tx_ctx->skb = skb;
nv_poll_controller
nv_do_nic_poll
line 4134: spin_lock(&np->lock);
nv_drain_rxtx
nv_drain_tx
nv_release_txskb
line 2004: dev_kfree_skb_any(tx_skb->skb);
Thus, two possible concurrency use-after-free bugs may occur.
To fix these possible bugs,
Does this really occur? Can you reproduce this ?
the calls to spin_lock_irqsave() in
nv_start_xmit() and nv_start_xmit_optimized() are moved to the
front of "prev_tx_ctx->skb = skb;"
Signed-off-by: Jia-Ju Bai
---
drivers/net/ethernet/nvidia/forcedeth.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/net/ethernet/nvidia/forcedeth.c
b/drivers/net/ethernet/nvidia/forcedeth.c
index 1d9b0d44ddb6..48fa5a0bd2cb 100644
--- a/drivers/net/ethernet/nvidia/forcedeth.c
+++ b/drivers/net/ethernet/nvidia/forcedeth.c
@@ -2317,6 +2317,8 @@ static netdev_tx_t nv_start_xmit(struct sk_buff *skb,
struct net_device *dev)
/* set last fragment flag */
prev_tx->flaglen |= cpu_to_le32(tx_flags_extra);
+ spin_lock_irqsave(&np->lock, flags);
+
/* save skb in this slot's context area */
prev_tx_ctx->skb = skb;
@@ -2326,8 +2328,6 @@ static netdev_tx_t nv_start_xmit(struct sk_buff *skb, struct net_device *dev)
tx_flags_extra = skb->ip_summed == CHECKSUM_PARTIAL ?
NV_TX2_CHECKSUM_L3 | NV_TX2_CHECKSUM_L4 : 0;
- spin_lock_irqsave(&np->lock, flags);
-
/* set tx flags */
start_tx->flaglen |= cpu_to_le32(tx_flags | tx_flags_extra);
@@ -2475,6 +2475,8 @@ static netdev_tx_t nv_start_xmit_optimized(struct sk_buff *skb,
/* set last fragment flag */
prev_tx->flaglen |= cpu_to_le32(NV_TX2_LASTPACKET);
+ spin_lock_irqsave(&np->lock, flags);
+
/* save skb in this slot's context area */
prev_tx_ctx->skb = skb;
@@ -2491,8 +2493,6 @@ static netdev_tx_t nv_start_xmit_optimized(struct sk_buff *skb,
else
start_tx->txvlan = 0;
- spin_lock_irqsave(&np->lock, flags);
-
if (np->tx_limit) {
/* Limit the number of outstanding tx. Setup all fragments, but
* do not set the VALID bit on the first descriptor. Save a
pointer
[PATCH] net: nvidia: forcedeth: Fix two possible concurrency use-after-free bugs
In drivers/net/ethernet/nvidia/forcedeth.c, the functions
nv_start_xmit() and nv_start_xmit_optimized() can be concurrently
executed with nv_poll_controller().
nv_start_xmit
line 2321: prev_tx_ctx->skb = skb;
nv_start_xmit_optimized
line 2479: prev_tx_ctx->skb = skb;
nv_poll_controller
nv_do_nic_poll
line 4134: spin_lock(&np->lock);
nv_drain_rxtx
nv_drain_tx
nv_release_txskb
line 2004: dev_kfree_skb_any(tx_skb->skb);
Thus, two possible concurrency use-after-free bugs may occur.
To fix these possible bugs, the calls to spin_lock_irqsave() in
nv_start_xmit() and nv_start_xmit_optimized() are moved to the
front of "prev_tx_ctx->skb = skb;"
Signed-off-by: Jia-Ju Bai
---
drivers/net/ethernet/nvidia/forcedeth.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/net/ethernet/nvidia/forcedeth.c
b/drivers/net/ethernet/nvidia/forcedeth.c
index 1d9b0d44ddb6..48fa5a0bd2cb 100644
--- a/drivers/net/ethernet/nvidia/forcedeth.c
+++ b/drivers/net/ethernet/nvidia/forcedeth.c
@@ -2317,6 +2317,8 @@ static netdev_tx_t nv_start_xmit(struct sk_buff *skb,
struct net_device *dev)
/* set last fragment flag */
prev_tx->flaglen |= cpu_to_le32(tx_flags_extra);
+ spin_lock_irqsave(&np->lock, flags);
+
/* save skb in this slot's context area */
prev_tx_ctx->skb = skb;
@@ -2326,8 +2328,6 @@ static netdev_tx_t nv_start_xmit(struct sk_buff *skb,
struct net_device *dev)
tx_flags_extra = skb->ip_summed == CHECKSUM_PARTIAL ?
NV_TX2_CHECKSUM_L3 | NV_TX2_CHECKSUM_L4 : 0;
- spin_lock_irqsave(&np->lock, flags);
-
/* set tx flags */
start_tx->flaglen |= cpu_to_le32(tx_flags | tx_flags_extra);
@@ -2475,6 +2475,8 @@ static netdev_tx_t nv_start_xmit_optimized(struct sk_buff
*skb,
/* set last fragment flag */
prev_tx->flaglen |= cpu_to_le32(NV_TX2_LASTPACKET);
+ spin_lock_irqsave(&np->lock, flags);
+
/* save skb in this slot's context area */
prev_tx_ctx->skb = skb;
@@ -2491,8 +2493,6 @@ static netdev_tx_t nv_start_xmit_optimized(struct sk_buff
*skb,
else
start_tx->txvlan = 0;
- spin_lock_irqsave(&np->lock, flags);
-
if (np->tx_limit) {
/* Limit the number of outstanding tx. Setup all fragments, but
* do not set the VALID bit on the first descriptor. Save a
pointer
--
2.17.0
