From: Dongli Zhang <dongli.zhang@oracle.com>
To: <xen-devel@lists.xenproject.org>
Cc: <netdev@vger.kernel.org>, <jgross@suse.com>,
Joe Jin <joe.jin@oracle.com>
Subject: Question on xen-netfront code to fix a potential ring buffer corruption
Date: Sun, 18 Aug 2019 01:31:00 -0700 (PDT) [thread overview]
Message-ID: <c45b306e-c67b-49f5-8fe8-3913557a8774@default> (raw)
Hi,
Would you please help confirm why the condition at line 908 is ">="?
In my opinion, we would only hit "skb_shinfo(skb)->nr_frag == MAX_SKB_FRAGS" at
line 908.
890 static RING_IDX xennet_fill_frags(struct netfront_queue *queue,
891 struct sk_buff *skb,
892 struct sk_buff_head *list)
893 {
894 RING_IDX cons = queue->rx.rsp_cons;
895 struct sk_buff *nskb;
896
897 while ((nskb = __skb_dequeue(list))) {
898 struct xen_netif_rx_response *rx =
899 RING_GET_RESPONSE(&queue->rx, ++cons);
900 skb_frag_t *nfrag = &skb_shinfo(nskb)->frags[0];
901
902 if (skb_shinfo(skb)->nr_frags == MAX_SKB_FRAGS) {
903 unsigned int pull_to = NETFRONT_SKB_CB(skb)->pull_to;
904
905 BUG_ON(pull_to < skb_headlen(skb));
906 __pskb_pull_tail(skb, pull_to - skb_headlen(skb));
907 }
908 if (unlikely(skb_shinfo(skb)->nr_frags >= MAX_SKB_FRAGS)) {
909 queue->rx.rsp_cons = ++cons;
910 kfree_skb(nskb);
911 return ~0U;
912 }
913
914 skb_add_rx_frag(skb, skb_shinfo(skb)->nr_frags,
915 skb_frag_page(nfrag),
916 rx->offset, rx->status, PAGE_SIZE);
917
918 skb_shinfo(nskb)->nr_frags = 0;
919 kfree_skb(nskb);
920 }
921
922 return cons;
923 }
The reason that I ask about this is because I am considering below patch to
avoid a potential xen-netfront ring buffer corruption.
diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c
index 8d33970..48a2162 100644
--- a/drivers/net/xen-netfront.c
+++ b/drivers/net/xen-netfront.c
@@ -906,7 +906,7 @@ static RING_IDX xennet_fill_frags(struct netfront_queue *queue,
__pskb_pull_tail(skb, pull_to - skb_headlen(skb));
}
if (unlikely(skb_shinfo(skb)->nr_frags >= MAX_SKB_FRAGS)) {
- queue->rx.rsp_cons = ++cons;
+ queue->rx.rsp_cons = cons + skb_queue_len(list) + 1;
kfree_skb(nskb);
return ~0U;
}
If there is skb left in list when we return ~0U, queue->rx.rsp_cons may be set
incorrectly.
While I am trying to make up a case that would hit the corruption, I could not
explain why (unlikely(skb_shinfo(skb)->nr_frags >= MAX_SKB_FRAGS)), but not
just "==". Perhaps __pskb_pull_tail() may fail although pull_to is less than
skb_headlen(skb).
Thank you very much!
Dongli Zhang
next reply other threads:[~2019-08-18 8:31 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-08-18 8:31 Dongli Zhang [this message]
2019-08-27 6:13 ` Question on xen-netfront code to fix a potential ring buffer corruption Juergen Gross
2019-08-27 6:43 ` [Xen-devel] " Dongli Zhang
2019-08-27 7:04 ` Juergen Gross
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=c45b306e-c67b-49f5-8fe8-3913557a8774@default \
--to=dongli.zhang@oracle.com \
--cc=jgross@suse.com \
--cc=joe.jin@oracle.com \
--cc=netdev@vger.kernel.org \
--cc=xen-devel@lists.xenproject.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).