ACK: [UBUNTU B, F, J, K 1/1] xen/netback: Ensure protocol headers don't fall in the non-linear area
Ian May
ian.may at canonical.com
Wed Dec 14 22:51:00 UTC 2022
Acked-by: Ian May <ian.may at canonical.com>
On 2022-12-14 19:21:41 , Thadeu Lima de Souza Cascardo wrote:
> From: Ross Lagerwall <ross.lagerwall at citrix.com>
>
> In some cases, the frontend may send a packet where the protocol headers
> are spread across multiple slots. This would result in netback creating
> an skb where the protocol headers spill over into the non-linear area.
> Some drivers and NICs don't handle this properly resulting in an
> interface reset or worse.
>
> This issue was introduced by the removal of an unconditional skb pull in
> the tx path to improve performance. Fix this without reintroducing the
> pull by setting up grant copy ops for as many slots as needed to reach
> the XEN_NETBACK_TX_COPY_LEN size. Adjust the rest of the code to handle
> multiple copy operations per skb.
>
> This is XSA-423 / CVE-2022-3643.
>
> Fixes: 7e5d7753956b ("xen-netback: remove unconditional __pskb_pull_tail() in guest Tx path")
> Signed-off-by: Ross Lagerwall <ross.lagerwall at citrix.com>
> Reviewed-by: Paul Durrant <paul at xen.org>
> Signed-off-by: Juergen Gross <jgross at suse.com>
> (cherry picked from commit ad7f402ae4f466647c3a669b8a6f3e5d4271c84a)
> CVE-2022-3643
> Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo at canonical.com>
> ---
> drivers/net/xen-netback/netback.c | 223 ++++++++++++++++--------------
> 1 file changed, 123 insertions(+), 100 deletions(-)
>
> diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c
> index 995566a2785f..5706a1694c9c 100644
> --- a/drivers/net/xen-netback/netback.c
> +++ b/drivers/net/xen-netback/netback.c
> @@ -323,10 +323,13 @@ static int xenvif_count_requests(struct xenvif_queue *queue,
>
>
> struct xenvif_tx_cb {
> - u16 pending_idx;
> + u16 copy_pending_idx[XEN_NETBK_LEGACY_SLOTS_MAX + 1];
> + u8 copy_count;
> };
>
> #define XENVIF_TX_CB(skb) ((struct xenvif_tx_cb *)(skb)->cb)
> +#define copy_pending_idx(skb, i) (XENVIF_TX_CB(skb)->copy_pending_idx[i])
> +#define copy_count(skb) (XENVIF_TX_CB(skb)->copy_count)
>
> static inline void xenvif_tx_create_map_op(struct xenvif_queue *queue,
> u16 pending_idx,
> @@ -361,31 +364,93 @@ static inline struct sk_buff *xenvif_alloc_skb(unsigned int size)
> return skb;
> }
>
> -static struct gnttab_map_grant_ref *xenvif_get_requests(struct xenvif_queue *queue,
> - struct sk_buff *skb,
> - struct xen_netif_tx_request *txp,
> - struct gnttab_map_grant_ref *gop,
> - unsigned int frag_overflow,
> - struct sk_buff *nskb)
> +static void xenvif_get_requests(struct xenvif_queue *queue,
> + struct sk_buff *skb,
> + struct xen_netif_tx_request *first,
> + struct xen_netif_tx_request *txfrags,
> + unsigned *copy_ops,
> + unsigned *map_ops,
> + unsigned int frag_overflow,
> + struct sk_buff *nskb,
> + unsigned int extra_count,
> + unsigned int data_len)
> {
> struct skb_shared_info *shinfo = skb_shinfo(skb);
> skb_frag_t *frags = shinfo->frags;
> - u16 pending_idx = XENVIF_TX_CB(skb)->pending_idx;
> - int start;
> + u16 pending_idx;
> pending_ring_idx_t index;
> unsigned int nr_slots;
> + struct gnttab_copy *cop = queue->tx_copy_ops + *copy_ops;
> + struct gnttab_map_grant_ref *gop = queue->tx_map_ops + *map_ops;
> + struct xen_netif_tx_request *txp = first;
> +
> + nr_slots = shinfo->nr_frags + 1;
> +
> + copy_count(skb) = 0;
> +
> + /* Create copy ops for exactly data_len bytes into the skb head. */
> + __skb_put(skb, data_len);
> + while (data_len > 0) {
> + int amount = data_len > txp->size ? txp->size : data_len;
> +
> + cop->source.u.ref = txp->gref;
> + cop->source.domid = queue->vif->domid;
> + cop->source.offset = txp->offset;
> +
> + cop->dest.domid = DOMID_SELF;
> + cop->dest.offset = (offset_in_page(skb->data +
> + skb_headlen(skb) -
> + data_len)) & ~XEN_PAGE_MASK;
> + cop->dest.u.gmfn = virt_to_gfn(skb->data + skb_headlen(skb)
> + - data_len);
> +
> + cop->len = amount;
> + cop->flags = GNTCOPY_source_gref;
>
> - nr_slots = shinfo->nr_frags;
> + index = pending_index(queue->pending_cons);
> + pending_idx = queue->pending_ring[index];
> + callback_param(queue, pending_idx).ctx = NULL;
> + copy_pending_idx(skb, copy_count(skb)) = pending_idx;
> + copy_count(skb)++;
> +
> + cop++;
> + data_len -= amount;
>
> - /* Skip first skb fragment if it is on same page as header fragment. */
> - start = (frag_get_pending_idx(&shinfo->frags[0]) == pending_idx);
> + if (amount == txp->size) {
> + /* The copy op covered the full tx_request */
> +
> + memcpy(&queue->pending_tx_info[pending_idx].req,
> + txp, sizeof(*txp));
> + queue->pending_tx_info[pending_idx].extra_count =
> + (txp == first) ? extra_count : 0;
> +
> + if (txp == first)
> + txp = txfrags;
> + else
> + txp++;
> + queue->pending_cons++;
> + nr_slots--;
> + } else {
> + /* The copy op partially covered the tx_request.
> + * The remainder will be mapped.
> + */
> + txp->offset += amount;
> + txp->size -= amount;
> + }
> + }
>
> - for (shinfo->nr_frags = start; shinfo->nr_frags < nr_slots;
> - shinfo->nr_frags++, txp++, gop++) {
> + for (shinfo->nr_frags = 0; shinfo->nr_frags < nr_slots;
> + shinfo->nr_frags++, gop++) {
> index = pending_index(queue->pending_cons++);
> pending_idx = queue->pending_ring[index];
> - xenvif_tx_create_map_op(queue, pending_idx, txp, 0, gop);
> + xenvif_tx_create_map_op(queue, pending_idx, txp,
> + txp == first ? extra_count : 0, gop);
> frag_set_pending_idx(&frags[shinfo->nr_frags], pending_idx);
> +
> + if (txp == first)
> + txp = txfrags;
> + else
> + txp++;
> }
>
> if (frag_overflow) {
> @@ -406,7 +471,8 @@ static struct gnttab_map_grant_ref *xenvif_get_requests(struct xenvif_queue *que
> skb_shinfo(skb)->frag_list = nskb;
> }
>
> - return gop;
> + (*copy_ops) = cop - queue->tx_copy_ops;
> + (*map_ops) = gop - queue->tx_map_ops;
> }
>
> static inline void xenvif_grant_handle_set(struct xenvif_queue *queue,
> @@ -442,7 +508,7 @@ static int xenvif_tx_check_gop(struct xenvif_queue *queue,
> struct gnttab_copy **gopp_copy)
> {
> struct gnttab_map_grant_ref *gop_map = *gopp_map;
> - u16 pending_idx = XENVIF_TX_CB(skb)->pending_idx;
> + u16 pending_idx;
> /* This always points to the shinfo of the skb being checked, which
> * could be either the first or the one on the frag_list
> */
> @@ -453,24 +519,37 @@ static int xenvif_tx_check_gop(struct xenvif_queue *queue,
> struct skb_shared_info *first_shinfo = NULL;
> int nr_frags = shinfo->nr_frags;
> const bool sharedslot = nr_frags &&
> - frag_get_pending_idx(&shinfo->frags[0]) == pending_idx;
> + frag_get_pending_idx(&shinfo->frags[0]) ==
> + copy_pending_idx(skb, copy_count(skb) - 1);
> int i, err;
>
> - /* Check status of header. */
> - err = (*gopp_copy)->status;
> - if (unlikely(err)) {
> - if (net_ratelimit())
> - netdev_dbg(queue->vif->dev,
> - "Grant copy of header failed! status: %d pending_idx: %u ref: %u\n",
> - (*gopp_copy)->status,
> - pending_idx,
> - (*gopp_copy)->source.u.ref);
> - /* The first frag might still have this slot mapped */
> - if (!sharedslot)
> - xenvif_idx_release(queue, pending_idx,
> - XEN_NETIF_RSP_ERROR);
> + for (i = 0; i < copy_count(skb); i++) {
> + int newerr;
> +
> + /* Check status of header. */
> + pending_idx = copy_pending_idx(skb, i);
> +
> + newerr = (*gopp_copy)->status;
> + if (likely(!newerr)) {
> + /* The first frag might still have this slot mapped */
> + if (i < copy_count(skb) - 1 || !sharedslot)
> + xenvif_idx_release(queue, pending_idx,
> + XEN_NETIF_RSP_OKAY);
> + } else {
> + err = newerr;
> + if (net_ratelimit())
> + netdev_dbg(queue->vif->dev,
> + "Grant copy of header failed! status: %d pending_idx: %u ref: %u\n",
> + (*gopp_copy)->status,
> + pending_idx,
> + (*gopp_copy)->source.u.ref);
> + /* The first frag might still have this slot mapped */
> + if (i < copy_count(skb) - 1 || !sharedslot)
> + xenvif_idx_release(queue, pending_idx,
> + XEN_NETIF_RSP_ERROR);
> + }
> + (*gopp_copy)++;
> }
> - (*gopp_copy)++;
>
> check_frags:
> for (i = 0; i < nr_frags; i++, gop_map++) {
> @@ -517,14 +596,6 @@ static int xenvif_tx_check_gop(struct xenvif_queue *queue,
> if (err)
> continue;
>
> - /* First error: if the header haven't shared a slot with the
> - * first frag, release it as well.
> - */
> - if (!sharedslot)
> - xenvif_idx_release(queue,
> - XENVIF_TX_CB(skb)->pending_idx,
> - XEN_NETIF_RSP_OKAY);
> -
> /* Invalidate preceding fragments of this skb. */
> for (j = 0; j < i; j++) {
> pending_idx = frag_get_pending_idx(&shinfo->frags[j]);
> @@ -794,7 +865,6 @@ static void xenvif_tx_build_gops(struct xenvif_queue *queue,
> unsigned *copy_ops,
> unsigned *map_ops)
> {
> - struct gnttab_map_grant_ref *gop = queue->tx_map_ops;
> struct sk_buff *skb, *nskb;
> int ret;
> unsigned int frag_overflow;
> @@ -876,8 +946,12 @@ static void xenvif_tx_build_gops(struct xenvif_queue *queue,
> continue;
> }
>
> + data_len = (txreq.size > XEN_NETBACK_TX_COPY_LEN) ?
> + XEN_NETBACK_TX_COPY_LEN : txreq.size;
> +
> ret = xenvif_count_requests(queue, &txreq, extra_count,
> txfrags, work_to_do);
> +
> if (unlikely(ret < 0))
> break;
>
> @@ -903,9 +977,8 @@ static void xenvif_tx_build_gops(struct xenvif_queue *queue,
> index = pending_index(queue->pending_cons);
> pending_idx = queue->pending_ring[index];
>
> - data_len = (txreq.size > XEN_NETBACK_TX_COPY_LEN &&
> - ret < XEN_NETBK_LEGACY_SLOTS_MAX) ?
> - XEN_NETBACK_TX_COPY_LEN : txreq.size;
> + if (ret >= XEN_NETBK_LEGACY_SLOTS_MAX - 1 && data_len < txreq.size)
> + data_len = txreq.size;
>
> skb = xenvif_alloc_skb(data_len);
> if (unlikely(skb == NULL)) {
> @@ -916,8 +989,6 @@ static void xenvif_tx_build_gops(struct xenvif_queue *queue,
> }
>
> skb_shinfo(skb)->nr_frags = ret;
> - if (data_len < txreq.size)
> - skb_shinfo(skb)->nr_frags++;
> /* At this point shinfo->nr_frags is in fact the number of
> * slots, which can be as large as XEN_NETBK_LEGACY_SLOTS_MAX.
> */
> @@ -979,54 +1050,19 @@ static void xenvif_tx_build_gops(struct xenvif_queue *queue,
> type);
> }
>
> - XENVIF_TX_CB(skb)->pending_idx = pending_idx;
> -
> - __skb_put(skb, data_len);
> - queue->tx_copy_ops[*copy_ops].source.u.ref = txreq.gref;
> - queue->tx_copy_ops[*copy_ops].source.domid = queue->vif->domid;
> - queue->tx_copy_ops[*copy_ops].source.offset = txreq.offset;
> -
> - queue->tx_copy_ops[*copy_ops].dest.u.gmfn =
> - virt_to_gfn(skb->data);
> - queue->tx_copy_ops[*copy_ops].dest.domid = DOMID_SELF;
> - queue->tx_copy_ops[*copy_ops].dest.offset =
> - offset_in_page(skb->data) & ~XEN_PAGE_MASK;
> -
> - queue->tx_copy_ops[*copy_ops].len = data_len;
> - queue->tx_copy_ops[*copy_ops].flags = GNTCOPY_source_gref;
> -
> - (*copy_ops)++;
> -
> - if (data_len < txreq.size) {
> - frag_set_pending_idx(&skb_shinfo(skb)->frags[0],
> - pending_idx);
> - xenvif_tx_create_map_op(queue, pending_idx, &txreq,
> - extra_count, gop);
> - gop++;
> - } else {
> - frag_set_pending_idx(&skb_shinfo(skb)->frags[0],
> - INVALID_PENDING_IDX);
> - memcpy(&queue->pending_tx_info[pending_idx].req,
> - &txreq, sizeof(txreq));
> - queue->pending_tx_info[pending_idx].extra_count =
> - extra_count;
> - }
> -
> - queue->pending_cons++;
> -
> - gop = xenvif_get_requests(queue, skb, txfrags, gop,
> - frag_overflow, nskb);
> + xenvif_get_requests(queue, skb, &txreq, txfrags, copy_ops,
> + map_ops, frag_overflow, nskb, extra_count,
> + data_len);
>
> __skb_queue_tail(&queue->tx_queue, skb);
>
> queue->tx.req_cons = idx;
>
> - if (((gop-queue->tx_map_ops) >= ARRAY_SIZE(queue->tx_map_ops)) ||
> + if ((*map_ops >= ARRAY_SIZE(queue->tx_map_ops)) ||
> (*copy_ops >= ARRAY_SIZE(queue->tx_copy_ops)))
> break;
> }
>
> - (*map_ops) = gop - queue->tx_map_ops;
> return;
> }
>
> @@ -1105,9 +1141,8 @@ static int xenvif_tx_submit(struct xenvif_queue *queue)
> while ((skb = __skb_dequeue(&queue->tx_queue)) != NULL) {
> struct xen_netif_tx_request *txp;
> u16 pending_idx;
> - unsigned data_len;
>
> - pending_idx = XENVIF_TX_CB(skb)->pending_idx;
> + pending_idx = copy_pending_idx(skb, 0);
> txp = &queue->pending_tx_info[pending_idx].req;
>
> /* Check the remap error code. */
> @@ -1126,18 +1161,6 @@ static int xenvif_tx_submit(struct xenvif_queue *queue)
> continue;
> }
>
> - data_len = skb->len;
> - callback_param(queue, pending_idx).ctx = NULL;
> - if (data_len < txp->size) {
> - /* Append the packet payload as a fragment. */
> - txp->offset += data_len;
> - txp->size -= data_len;
> - } else {
> - /* Schedule a response immediately. */
> - xenvif_idx_release(queue, pending_idx,
> - XEN_NETIF_RSP_OKAY);
> - }
> -
> if (txp->flags & XEN_NETTXF_csum_blank)
> skb->ip_summed = CHECKSUM_PARTIAL;
> else if (txp->flags & XEN_NETTXF_data_validated)
> @@ -1323,7 +1346,7 @@ static inline void xenvif_tx_dealloc_action(struct xenvif_queue *queue)
> /* Called after netfront has transmitted */
> int xenvif_tx_action(struct xenvif_queue *queue, int budget)
> {
> - unsigned nr_mops, nr_cops = 0;
> + unsigned nr_mops = 0, nr_cops = 0;
> int work_done, ret;
>
> if (unlikely(!tx_work_todo(queue)))
> --
> 2.34.1
>
>
> --
> kernel-team mailing list
> kernel-team at lists.ubuntu.com
> https://lists.ubuntu.com/mailman/listinfo/kernel-team
More information about the kernel-team
mailing list