* [PATCH 1/4] net: atlantic: fix "frag[0] not initialized"
2022-05-10 2:28 [PATCH 0/4 V2] net: atlantic: more fuzzing fixes Grant Grundler
@ 2022-05-10 2:28 ` Grant Grundler
2022-05-10 2:28 ` [PATCH 2/4] net: atlantic: reduce scope of is_rsc_complete Grant Grundler
` (3 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Grant Grundler @ 2022-05-10 2:28 UTC (permalink / raw)
To: Igor Russkikh
Cc: Jakub Kicinski, Paolo Abeni, netdev, David S . Miller, LKML,
Aashay Shringarpure, Yi Chou, Shervin Oloumi, Grant Grundler
In aq_ring_rx_clean(), if buff->is_eop is not set AND
buff->len < AQ_CFG_RX_HDR_SIZE, then hdr_len remains equal to
buff->len and skb_add_rx_frag(xxx, *0*, ...) is not called.
The loop following this code starts calling skb_add_rx_frag() starting
with i=1 and thus frag[0] is never initialized. Since i is initialized
to zero at the top of the primary loop, we can just reference and
post-increment i instead of hardcoding the 0 when calling
skb_add_rx_frag() the first time.
Reported-by: Aashay Shringarpure <aashay@google.com>
Reported-by: Yi Chou <yich@google.com>
Reported-by: Shervin Oloumi <enlightened@google.com>
Signed-off-by: Grant Grundler <grundler@chromium.org>
---
drivers/net/ethernet/aquantia/atlantic/aq_ring.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_ring.c b/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
index 77e76c9efd32..440423b0e8ea 100644
--- a/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
+++ b/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
@@ -446,7 +446,7 @@ int aq_ring_rx_clean(struct aq_ring_s *self,
ALIGN(hdr_len, sizeof(long)));
if (buff->len - hdr_len > 0) {
- skb_add_rx_frag(skb, 0, buff->rxdata.page,
+ skb_add_rx_frag(skb, i++, buff->rxdata.page,
buff->rxdata.pg_off + hdr_len,
buff->len - hdr_len,
AQ_CFG_RX_FRAME_MAX);
@@ -455,7 +455,6 @@ int aq_ring_rx_clean(struct aq_ring_s *self,
if (!buff->is_eop) {
buff_ = buff;
- i = 1U;
do {
next_ = buff_->next;
buff_ = &self->buff_ring[next_];
--
2.36.0.512.ge40c2bad7a-goog
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/4] net: atlantic: reduce scope of is_rsc_complete
2022-05-10 2:28 [PATCH 0/4 V2] net: atlantic: more fuzzing fixes Grant Grundler
2022-05-10 2:28 ` [PATCH 1/4] net: atlantic: fix "frag[0] not initialized" Grant Grundler
@ 2022-05-10 2:28 ` Grant Grundler
2022-05-10 2:28 ` [PATCH 3/4] net: atlantic: add check for MAX_SKB_FRAGS Grant Grundler
` (2 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Grant Grundler @ 2022-05-10 2:28 UTC (permalink / raw)
To: Igor Russkikh
Cc: Jakub Kicinski, Paolo Abeni, netdev, David S . Miller, LKML,
Aashay Shringarpure, Yi Chou, Shervin Oloumi, Grant Grundler
Don't defer handling the err case outside the loop. That's pointless.
And since is_rsc_complete is only used inside this loop, declare
it inside the loop to reduce it's scope.
Signed-off-by: Grant Grundler <grundler@chromium.org>
---
drivers/net/ethernet/aquantia/atlantic/aq_ring.c | 13 ++++++-------
1 file changed, 6 insertions(+), 7 deletions(-)
diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_ring.c b/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
index 440423b0e8ea..bc1952131799 100644
--- a/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
+++ b/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
@@ -346,7 +346,6 @@ int aq_ring_rx_clean(struct aq_ring_s *self,
int budget)
{
struct net_device *ndev = aq_nic_get_ndev(self->aq_nic);
- bool is_rsc_completed = true;
int err = 0;
for (; (self->sw_head != self->hw_head) && budget;
@@ -366,6 +365,8 @@ int aq_ring_rx_clean(struct aq_ring_s *self,
if (!buff->is_eop) {
buff_ = buff;
do {
+ bool is_rsc_completed = true;
+
if (buff_->next >= self->size) {
err = -EIO;
goto err_exit;
@@ -377,18 +378,16 @@ int aq_ring_rx_clean(struct aq_ring_s *self,
next_,
self->hw_head);
- if (unlikely(!is_rsc_completed))
- break;
+ if (unlikely(!is_rsc_completed)) {
+ err = 0;
+ goto err_exit;
+ }
buff->is_error |= buff_->is_error;
buff->is_cso_err |= buff_->is_cso_err;
} while (!buff_->is_eop);
- if (!is_rsc_completed) {
- err = 0;
- goto err_exit;
- }
if (buff->is_error ||
(buff->is_lro && buff->is_cso_err)) {
buff_ = buff;
--
2.36.0.512.ge40c2bad7a-goog
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 3/4] net: atlantic: add check for MAX_SKB_FRAGS
2022-05-10 2:28 [PATCH 0/4 V2] net: atlantic: more fuzzing fixes Grant Grundler
2022-05-10 2:28 ` [PATCH 1/4] net: atlantic: fix "frag[0] not initialized" Grant Grundler
2022-05-10 2:28 ` [PATCH 2/4] net: atlantic: reduce scope of is_rsc_complete Grant Grundler
@ 2022-05-10 2:28 ` Grant Grundler
2022-05-10 2:28 ` [PATCH 4/4] net: atlantic: verify hw_head_ lies within TX buffer ring Grant Grundler
2022-05-11 11:30 ` [PATCH 0/4 V2] net: atlantic: more fuzzing fixes patchwork-bot+netdevbpf
4 siblings, 0 replies; 6+ messages in thread
From: Grant Grundler @ 2022-05-10 2:28 UTC (permalink / raw)
To: Igor Russkikh
Cc: Jakub Kicinski, Paolo Abeni, netdev, David S . Miller, LKML,
Aashay Shringarpure, Yi Chou, Shervin Oloumi, Grant Grundler
Enforce that the CPU can not get stuck in an infinite loop.
Reported-by: Aashay Shringarpure <aashay@google.com>
Reported-by: Yi Chou <yich@google.com>
Reported-by: Shervin Oloumi <enlightened@google.com>
Signed-off-by: Grant Grundler <grundler@chromium.org>
---
drivers/net/ethernet/aquantia/atlantic/aq_ring.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_ring.c b/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
index bc1952131799..8201ce7adb77 100644
--- a/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
+++ b/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
@@ -363,6 +363,7 @@ int aq_ring_rx_clean(struct aq_ring_s *self,
continue;
if (!buff->is_eop) {
+ unsigned int frag_cnt = 0U;
buff_ = buff;
do {
bool is_rsc_completed = true;
@@ -371,6 +372,8 @@ int aq_ring_rx_clean(struct aq_ring_s *self,
err = -EIO;
goto err_exit;
}
+
+ frag_cnt++;
next_ = buff_->next,
buff_ = &self->buff_ring[next_];
is_rsc_completed =
@@ -378,7 +381,8 @@ int aq_ring_rx_clean(struct aq_ring_s *self,
next_,
self->hw_head);
- if (unlikely(!is_rsc_completed)) {
+ if (unlikely(!is_rsc_completed) ||
+ frag_cnt > MAX_SKB_FRAGS) {
err = 0;
goto err_exit;
}
--
2.36.0.512.ge40c2bad7a-goog
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 4/4] net: atlantic: verify hw_head_ lies within TX buffer ring
2022-05-10 2:28 [PATCH 0/4 V2] net: atlantic: more fuzzing fixes Grant Grundler
` (2 preceding siblings ...)
2022-05-10 2:28 ` [PATCH 3/4] net: atlantic: add check for MAX_SKB_FRAGS Grant Grundler
@ 2022-05-10 2:28 ` Grant Grundler
2022-05-11 11:30 ` [PATCH 0/4 V2] net: atlantic: more fuzzing fixes patchwork-bot+netdevbpf
4 siblings, 0 replies; 6+ messages in thread
From: Grant Grundler @ 2022-05-10 2:28 UTC (permalink / raw)
To: Igor Russkikh
Cc: Jakub Kicinski, Paolo Abeni, netdev, David S . Miller, LKML,
Aashay Shringarpure, Yi Chou, Shervin Oloumi, Grant Grundler
Bounds check hw_head index provided by NIC to verify it lies
within the TX buffer ring.
Reported-by: Aashay Shringarpure <aashay@google.com>
Reported-by: Yi Chou <yich@google.com>
Reported-by: Shervin Oloumi <enlightened@google.com>
Signed-off-by: Grant Grundler <grundler@chromium.org>
---
drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_b0.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_b0.c b/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_b0.c
index d875ce3ec759..15ede7285fb5 100644
--- a/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_b0.c
+++ b/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_b0.c
@@ -889,6 +889,13 @@ int hw_atl_b0_hw_ring_tx_head_update(struct aq_hw_s *self,
err = -ENXIO;
goto err_exit;
}
+
+ /* Validate that the new hw_head_ is reasonable. */
+ if (hw_head_ >= ring->size) {
+ err = -ENXIO;
+ goto err_exit;
+ }
+
ring->hw_head = hw_head_;
err = aq_hw_err_from_flags(self);
--
2.36.0.512.ge40c2bad7a-goog
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 0/4 V2] net: atlantic: more fuzzing fixes
2022-05-10 2:28 [PATCH 0/4 V2] net: atlantic: more fuzzing fixes Grant Grundler
` (3 preceding siblings ...)
2022-05-10 2:28 ` [PATCH 4/4] net: atlantic: verify hw_head_ lies within TX buffer ring Grant Grundler
@ 2022-05-11 11:30 ` patchwork-bot+netdevbpf
4 siblings, 0 replies; 6+ messages in thread
From: patchwork-bot+netdevbpf @ 2022-05-11 11:30 UTC (permalink / raw)
To: Grant Grundler
Cc: irusskikh, kuba, pabeni, netdev, davem, linux-kernel, aashay,
yich, enlightened
Hello:
This series was applied to netdev/net.git (master)
by David S. Miller <davem@davemloft.net>:
On Mon, 9 May 2022 19:28:22 -0700 you wrote:
> The Chrome OS fuzzing team posted a "Fuzzing" report for atlantic driver
> in Q4 2021 using Chrome OS v5.4 kernel and "Cable Matters
> Thunderbolt 3 to 10 Gb Ethernet" (b0 version):
> https://docs.google.com/document/d/e/2PACX-1vT4oCGNhhy_AuUqpu6NGnW0N9HF_jxf2kS7raOpOlNRqJNiTHAtjiHRthXYSeXIRTgfeVvsEt0qK9qK/pub
>
> It essentially describes four problems:
> 1) validate rxd_wb->next_desc_ptr before populating buff->next
> 2) "frag[0] not initialized" case in aq_ring_rx_clean()
> 3) limit iterations handling fragments in aq_ring_rx_clean()
> 4) validate hw_head_ in hw_atl_b0_hw_ring_tx_head_update()
>
> [...]
Here is the summary with links:
- [1/4] net: atlantic: fix "frag[0] not initialized"
https://git.kernel.org/netdev/net/c/62e0ae0f4020
- [2/4] net: atlantic: reduce scope of is_rsc_complete
https://git.kernel.org/netdev/net/c/79784d77ebbd
- [3/4] net: atlantic: add check for MAX_SKB_FRAGS
https://git.kernel.org/netdev/net/c/6aecbba12b5c
- [4/4] net: atlantic: verify hw_head_ lies within TX buffer ring
https://git.kernel.org/netdev/net/c/2120b7f4d128
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 6+ messages in thread