Message ID | 67762c5cd115c74d743ba184c97def9a4734eebd.1537021802.git.igor.russkikh@aquantia.com |
---|---|
State | Accepted, archived |
Delegated to: | David Miller |
Headers | show |
Series | [v2,net] net: aquantia: memory corruption on jumbo frames | expand |
From: Igor Russkikh <igor.russkikh@aquantia.com> Date: Sat, 15 Sep 2018 18:03:39 +0300 > From: Friedemann Gerold <f.gerold@b-c-s.de> > > This patch fixes skb_shared area, which will be corrupted > upon reception of 4K jumbo packets. > > Originally build_skb usage purpose was to reuse page for skb to eliminate > needs of extra fragments. But that logic does not take into account that > skb_shared_info should be reserved at the end of skb data area. > > In case packet data consumes all the page (4K), skb_shinfo location > overflows the page. As a consequence, __build_skb zeroed shinfo data above > the allocated page, corrupting next page. > > The issue is rarely seen in real life because jumbo are normally larger > than 4K and that causes another code path to trigger. > But it 100% reproducible with simple scapy packet, like: > > sendp(IP(dst="192.168.100.3") / TCP(dport=443) \ > / Raw(RandString(size=(4096-40))), iface="enp1s0") > > Fixes: 018423e90bee ("net: ethernet: aquantia: Add ring support code") > > Reported-by: Friedemann Gerold <f.gerold@b-c-s.de> > Reported-by: Michael Rauch <michael@rauch.be> > Signed-off-by: Friedemann Gerold <f.gerold@b-c-s.de> > Tested-by: Nikita Danilov <nikita.danilov@aquantia.com> > Signed-off-by: Igor Russkikh <igor.russkikh@aquantia.com> APplied and queued up for -stable.
>> Fixes: 018423e90bee ("net: ethernet: aquantia: Add ring support code") >> >> Reported-by: Friedemann Gerold <f.gerold@b-c-s.de> >> Reported-by: Michael Rauch <michael@rauch.be> >> Signed-off-by: Friedemann Gerold <f.gerold@b-c-s.de> >> Tested-by: Nikita Danilov <nikita.danilov@aquantia.com> >> Signed-off-by: Igor Russkikh <igor.russkikh@aquantia.com> > > APplied and queued up for -stable. Hi David, I see this was applied to net-next tree only. Can we have it in net? We consider it as critical flaw. BR, Igor
From: Igor Russkikh <igor.russkikh@aquantia.com> Date: Tue, 18 Sep 2018 11:28:55 +0300 > >>> Fixes: 018423e90bee ("net: ethernet: aquantia: Add ring support code") >>> >>> Reported-by: Friedemann Gerold <f.gerold@b-c-s.de> >>> Reported-by: Michael Rauch <michael@rauch.be> >>> Signed-off-by: Friedemann Gerold <f.gerold@b-c-s.de> >>> Tested-by: Nikita Danilov <nikita.danilov@aquantia.com> >>> Signed-off-by: Igor Russkikh <igor.russkikh@aquantia.com> >> >> APplied and queued up for -stable. > > Hi David, I see this was applied to net-next tree only. > Can we have it in net? We consider it as critical flaw. Sorry about that, fixed.
diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_ring.c b/drivers/net/ethernet/aquantia/atlantic/aq_ring.c index b5f1f62e8e25..d1e1a0ba8615 100644 --- a/drivers/net/ethernet/aquantia/atlantic/aq_ring.c +++ b/drivers/net/ethernet/aquantia/atlantic/aq_ring.c @@ -225,9 +225,10 @@ int aq_ring_rx_clean(struct aq_ring_s *self, } /* for single fragment packets use build_skb() */ - if (buff->is_eop) { + if (buff->is_eop && + buff->len <= AQ_CFG_RX_FRAME_MAX - AQ_SKB_ALIGN) { skb = build_skb(page_address(buff->page), - buff->len + AQ_SKB_ALIGN); + AQ_CFG_RX_FRAME_MAX); if (unlikely(!skb)) { err = -ENOMEM; goto err_exit; @@ -247,18 +248,21 @@ int aq_ring_rx_clean(struct aq_ring_s *self, buff->len - ETH_HLEN, SKB_TRUESIZE(buff->len - ETH_HLEN)); - for (i = 1U, next_ = buff->next, - buff_ = &self->buff_ring[next_]; true; - next_ = buff_->next, - buff_ = &self->buff_ring[next_], ++i) { - skb_add_rx_frag(skb, i, buff_->page, 0, - buff_->len, - SKB_TRUESIZE(buff->len - - ETH_HLEN)); - buff_->is_cleaned = 1; - - if (buff_->is_eop) - break; + if (!buff->is_eop) { + for (i = 1U, next_ = buff->next, + buff_ = &self->buff_ring[next_]; + true; next_ = buff_->next, + buff_ = &self->buff_ring[next_], ++i) { + skb_add_rx_frag(skb, i, + buff_->page, 0, + buff_->len, + SKB_TRUESIZE(buff->len - + ETH_HLEN)); + buff_->is_cleaned = 1; + + if (buff_->is_eop) + break; + } } }