lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Sun, 6 Dec 2020 19:41:07 -0800 From: Shannon Nelson <snelson@...sando.io> To: Jesse Brandeburg <jesse.brandeburg@...el.com>, Xiaohui Zhang <ruc_zhangxiaohui@....com> Cc: Pensando Drivers <drivers@...sando.io>, "David S . Miller" <davem@...emloft.net>, Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org, linux-kernel@...r.kernel.org Subject: Re: [PATCH 1/1] ionic: fix array overflow on receiving too many fragments for a packet On 12/6/20 5:51 PM, Jesse Brandeburg wrote: > Xiaohui Zhang wrote: > >> From: Zhang Xiaohui <ruc_zhangxiaohui@....com> >> >> If the hardware receives an oversized packet with too many rx fragments, >> skb_shinfo(skb)->frags can overflow and corrupt memory of adjacent pages. >> This becomes especially visible if it corrupts the freelist pointer of >> a slab page. >> >> Signed-off-by: Zhang Xiaohui <ruc_zhangxiaohui@....com> > Hi, thanks for your patch. > > It appears this is a part of a series of patches (at least this one and > one to the ice driver) - please send as one series, with a cover letter > explanation. > > Please justify how this is a bug and how this is found / reproduced. > > I'll respond separately to the ice driver patch as I don't know this > hardware and it's limits, but I suspect that you've tried to fix a bug > where there was none. (It seems like something a code scanner might find > and be confused about) > >> --- >> drivers/net/ethernet/pensando/ionic/ionic_txrx.c | 6 +++++- >> 1 file changed, 5 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_txrx.c b/drivers/net/ethernet/pensando/ionic/ionic_txrx.c >> index 169ac4f54..a3e274c65 100644 >> --- a/drivers/net/ethernet/pensando/ionic/ionic_txrx.c >> +++ b/drivers/net/ethernet/pensando/ionic/ionic_txrx.c >> @@ -102,8 +102,12 @@ static struct sk_buff *ionic_rx_frags(struct ionic_queue *q, >> >> dma_unmap_page(dev, dma_unmap_addr(page_info, dma_addr), >> PAGE_SIZE, DMA_FROM_DEVICE); >> - skb_add_rx_frag(skb, skb_shinfo(skb)->nr_frags, >> + struct skb_shared_info *shinfo = skb_shinfo(skb); > you can't declare variables in the middle of a code flow in C, did you > compile this? > >> + >> + if (shinfo->nr_frags < ARRAY_SIZE(shinfo->frags)) { >> + skb_add_rx_frag(skb, shinfo->nr_frags, >> page_info->page, 0, frag_len, PAGE_SIZE); >> + } Is this just dropping the remaining frags without dropping the rest of the skb? Is this going to leave an incorrect length in the skb? A single statement after the 'if' doesn't need {}'s This might be better handled by making sure ahead of time in configuration that the HW doesn't do this, rather than add a test into the fast path. As it is, between the definitions of shinfo->frags[] and the ionic's rx sg list, I don't think this is a possible error. As Jesse suggests, I'd like to see the test case so i can add it to our internal testing. Thanks, sln >> page_info->page = NULL; >> page_info++; >> i--; >
Powered by blists - more mailing lists