| Message ID | 20201206133537.30135-1-ruc_zhangxiaohui@163.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [1/1] ionic: fix array overflow on receiving too many fragments for a packet | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Guessed tree name to be net-next |
| netdev/subject_prefix | warning | Target tree name not specified in the subject |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | warning | CHECK: Alignment should match open parenthesis |
| netdev/build_allmodconfig_warn | fail | Errors and warnings before: 0 this patch: 2 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
Xiaohui Zhang wrote: > From: Zhang Xiaohui <ruc_zhangxiaohui@163.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@163.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); > + } > page_info->page = NULL; > page_info++; > i--;
On 12/6/20 5:51 PM, Jesse Brandeburg wrote: > Xiaohui Zhang wrote: > >> From: Zhang Xiaohui <ruc_zhangxiaohui@163.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@163.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--; >
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); + + if (shinfo->nr_frags < ARRAY_SIZE(shinfo->frags)) { + skb_add_rx_frag(skb, shinfo->nr_frags, page_info->page, 0, frag_len, PAGE_SIZE); + } page_info->page = NULL; page_info++; i--;