xennet_start_xmit assumptions

As I was playing around with pf_packet, I accidentally wrote
a buggy application program that bzero'ed the msghdr, then set
up the msg_name, msg_namelen correctly, and then did a sendmsg
on the pf_packet/SOCK_RAW fd.

This causes packet_snd to set up an skb with a lot of issues,
e.g., skb->len = 0, skb_headlen(skb) is 0, etc. I think we can/should
drop the packet in packet_snd if the skb->len is 0, but there
may be other driver bugs going on:

Turns out that ixgbe and sunvnet handle this problematic
skb correctly (they drop it and system remains stable), 
but it creates a panic in xen_netfront (xennet_start_xmit()
hits a null pointer deref when xennet_make_first_txreq() returns 
NULL)

I'm new to the xen driver code, so I'm hoping that
the experts can comment here: reading the code in xennet_start_xmit,
it seems like it mandatorily requires the skb_headlen() to be
non-zero in order to create the first_tx? That may not always be
true, how does the code recover for purely non-linear skbs?

--Sowmini

On Wed, Jan 18, 2017 at 10:31:32AM -0500, Sowmini Varadhan wrote:

As I was playing around with pf_packet, I accidentally wrote
a buggy application program that bzero'ed the msghdr, then set
up the msg_name, msg_namelen correctly, and then did a sendmsg
on the pf_packet/SOCK_RAW fd.

This causes packet_snd to set up an skb with a lot of issues,
e.g., skb->len = 0, skb_headlen(skb) is 0, etc. I think we can/should
drop the packet in packet_snd if the skb->len is 0, but there
may be other driver bugs going on:

Turns out that ixgbe and sunvnet handle this problematic
skb correctly (they drop it and system remains stable), 
but it creates a panic in xen_netfront (xennet_start_xmit()
hits a null pointer deref when xennet_make_first_txreq() returns 
NULL)

I'm new to the xen driver code, so I'm hoping that
the experts can comment here: reading the code in xennet_start_xmit,
it seems like it mandatorily requires the skb_headlen() to be
non-zero in order to create the first_tx? That may not always be
true, how does the code recover for purely non-linear skbs?

--Sowmini

CC-ing the two folks from the MAINTAINERS file.

-----Original Message-----
From: Konrad Rzeszutek Wilk [mailto:konrad.wilk@oracle.com]
Sent: 18 January 2017 19:25
To: Sowmini Varadhan <redacted>; Wei Liu
[off-list ref]; Paul Durrant [off-list ref]
Cc: netdev@vger.kernel.org; xen-devel@lists.xenproject.org
Subject: Re: [Xen-devel] xennet_start_xmit assumptions

On Wed, Jan 18, 2017 at 10:31:32AM -0500, Sowmini Varadhan wrote:

quoted

As I was playing around with pf_packet, I accidentally wrote
a buggy application program that bzero'ed the msghdr, then set
up the msg_name, msg_namelen correctly, and then did a sendmsg
on the pf_packet/SOCK_RAW fd.

This causes packet_snd to set up an skb with a lot of issues,
e.g., skb->len = 0, skb_headlen(skb) is 0, etc. I think we can/should
drop the packet in packet_snd if the skb->len is 0, but there
may be other driver bugs going on:

Turns out that ixgbe and sunvnet handle this problematic
skb correctly (they drop it and system remains stable),
but it creates a panic in xen_netfront (xennet_start_xmit()
hits a null pointer deref when xennet_make_first_txreq() returns
NULL)

I'm new to the xen driver code, so I'm hoping that
the experts can comment here: reading the code in xennet_start_xmit,
it seems like it mandatorily requires the skb_headlen() to be
non-zero in order to create the first_tx? That may not always be
true, how does the code recover for purely non-linear skbs?

Hi Sowmini,

  Sounds like a straightforward bug to me... netfront should be able to handle an empty skb and clearly, if it's relying on skb_headlen() being non-zero, that's not the case.

  Paul

quoted

--Sowmini

CC-ing the two folks from the MAINTAINERS file.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

On (01/19/17 09:36), Paul Durrant wrote:

Hi Sowmini,

  Sounds like a straightforward bug to me... netfront should be able
to handle an empty skb and clearly, if it's relying on skb_headlen()
being non-zero, that's not the case.

  Paul

I see. Seems like there are 2 things broken here: recovering
from skb->len = 0, and recovering from  the more complex
case of (skb->len > 0 && skb_headlen(skb) == 0)

Do you folks want to take a shot at fixing this,
since you know the code better? If you are interested,
I can share my test program to help you reproduce the 
simpler skb->len == 0 case, but it's the fully non-linear
skbs that may be more interesting to reproduce/fix. 

I'll probably work on fixing packet_snd to return -EINVAL 
or similar when the len is zero this week.

--Sowmini

-----Original Message-----
From: Sowmini Varadhan [mailto:sowmini.varadhan@oracle.com]
Sent: 19 January 2017 11:14
To: Paul Durrant <redacted>
Cc: Konrad Rzeszutek Wilk <redacted>; Wei Liu
[off-list ref]; netdev@vger.kernel.org; xen-
devel@lists.xenproject.org
Subject: Re: [Xen-devel] xennet_start_xmit assumptions

On (01/19/17 09:36), Paul Durrant wrote:

quoted

Hi Sowmini,

  Sounds like a straightforward bug to me... netfront should be able
to handle an empty skb and clearly, if it's relying on skb_headlen()
being non-zero, that's not the case.

  Paul

I see. Seems like there are 2 things broken here: recovering
from skb->len = 0, and recovering from  the more complex
case of (skb->len > 0 && skb_headlen(skb) == 0)

Do you folks want to take a shot at fixing this,
since you know the code better? If you are interested,
I can share my test program to help you reproduce the
simpler skb->len == 0 case, but it's the fully non-linear
skbs that may be more interesting to reproduce/fix.

Sowmini,

Yeah, it would be useful to verify any change fixes the particular issue you're seeing so please share the program. For the non-empty non-linear case I'd hope that catching this and doing a pull of some sensible amount of header (which might coincide with the least amount that netback expects to see in the first frag) would be enough.
I can take a shot at a patch for this in the next few days; I'll add your 'Reported-by' so you should get cc-ed.

Cheers,

  Paul

I'll probably work on fixing packet_snd to return -EINVAL
or similar when the len is zero this week.

--Sowmini

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

On (01/19/17 11:31), Paul Durrant wrote:

Sowmini,

Yeah, it would be useful to verify any change fixes the particular
issue you're seeing so please share the program. For the non-empty
non-linear case I'd hope that catching this and doing a pull of some
sensible amount of header (which might coincide with the least amount
that netback expects to see in the first frag) would be enough.
I can take a shot at a patch for this in the next few days; I'll add
your 'Reported-by' so you should get cc-ed.

Sounds good, here is the test program (very basic, no error checks!).
I think you can probably massage the pf_packet code to send down a 
purely non-linear skb, I can take a look at what this involves and  
get back to you. 


#include <stdio.h>
#include <sys/types.h>
#include <sys/socket.h>
#include <sys/ioctl.h>
#include <netinet/in.h>
#include <net/if.h>
#include <stdio.h>
#include <errno.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>
#include <ctype.h>
#include <string.h>
#include <linux/if_packet.h>
#include <linux/if_ether.h>

int main(int argc, char *argv[])
{
        struct ifreq ifr;
        int sockfd = socket(AF_INET, SOCK_DGRAM, 0);
        struct sockaddr_ll sll;
        int fd;
        struct msghdr msg;

        bzero(&ifr, sizeof (ifr));
        strncpy(ifr.ifr_name, argv[1], sizeof(ifr.ifr_name));
        ioctl(sockfd, SIOCGIFHWADDR, &ifr);

        bzero(&sll, sizeof (sll));
        sll.sll_family = PF_PACKET;
        sll.sll_halen = ETH_ALEN;
        sll.sll_ifindex = if_nametoindex(argv[1]);
        memcpy(sll.sll_addr,  (struct sockaddr *)&(ifr.ifr_hwaddr), ETH_ALEN);

        fd = socket(PF_PACKET, SOCK_RAW, htons(ETH_P_ALL));
        bind(fd, (struct sockaddr *)&sll, sizeof(sll));
        bzero(&msg, sizeof (msg));
        msg.msg_name = (void *)&sll;
        msg.msg_namelen = sizeof (struct sockaddr_ll);
        sendmsg(fd, &msg, 0);
        fprintf(stderr, "sent!\n");
}

From: Sowmini Varadhan <redacted>
Date: Thu, 19 Jan 2017 06:14:26 -0500

I'll probably work on fixing packet_snd to return -EINVAL 
or similar when the len is zero this week.

I thought we had code which made sure that at least a minimal
link layer header was present in the SKB?

Specifically I'm talking about the dev_validate_header() check.
That is supposed to protect us from these kinds of situations.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

On (01/19/17 11:37), David Miller wrote:

I thought we had code which made sure that at least a minimal
link layer header was present in the SKB?

Specifically I'm talking about the dev_validate_header() check.
That is supposed to protect us from these kinds of situations.

ah, but I run my pf_packet application as root, so I have 
capable(CAP_SYS_RAWIO), so I slip through the dev_validate_header()
check.

--Sowmini

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

On (01/19/17 13:47), Sowmini Varadhan wrote:

quoted

Specifically I'm talking about the dev_validate_header() check.
That is supposed to protect us from these kinds of situations.

ah, but I run my pf_packet application as root, so I have 
capable(CAP_SYS_RAWIO), so I slip through the dev_validate_header()
check.

and in that light, should dev_validate_header()
always return false if len == 0?

that will take care of all the send paths in af_packet.c
but it impacts all drivers as well (even though it is the
logically correct thing to do..)

--Sowmini

From: Sowmini Varadhan <redacted>
Date: Thu, 19 Jan 2017 17:41:23 -0500

On (01/19/17 13:47), Sowmini Varadhan wrote:

quoted

quoted

Specifically I'm talking about the dev_validate_header() check.
That is supposed to protect us from these kinds of situations.

ah, but I run my pf_packet application as root, so I have 
capable(CAP_SYS_RAWIO), so I slip through the dev_validate_header()
check.

and in that light, should dev_validate_header()
always return false if len == 0?

that will take care of all the send paths in af_packet.c
but it impacts all drivers as well (even though it is the
logically correct thing to do..)

I think dev_validate_header() almost does the correct thing in
the SYS_RAWIO case.

It clears out the not-provided hard header bytes, but it doesn't
adjust the skb->len.  I think that is a real requirement in this
situation.

CAP_SYS_RAWIO or not, the contract we have with the device is that
there will be at least enough bytes to cover a link layer header.

This probably requires a little bit of an adjustment to the calling
convention.  Perhaps:

	int dev_validate_header(const struct net_device *dev,
				char *ll_header, int len);

So then you can go:

	new_len = dev_validate_header(dev, skb->data, len);
	if (new_len < 0)
		goto out_cleanup_err;
	if (new_len > len)
		__skb_put(skb, new_len - len);

Or something like that.

On (01/20/17 14:30), David Miller wrote:

CAP_SYS_RAWIO or not, the contract we have with the device is that
there will be at least enough bytes to cover a link layer header.

I see. If that's the case (for all the kernel-driver interfaces), 
then the xen_netfront driver is probably not required to check for
variants of sk_buffs like pure-non-linear etc.

This probably requires a little bit of an adjustment to the calling
convention.  Perhaps:

	int dev_validate_header(const struct net_device *dev,
				char *ll_header, int len);

So then you can go:

	new_len = dev_validate_header(dev, skb->data, len);
	if (new_len < 0)
		goto out_cleanup_err;
	if (new_len > len)
		__skb_put(skb, new_len - len);

Or something like that.

ok let me work with that and get back (hopefully with an 
RFC patch).

--Sowmini

-----Original Message-----
From: Sowmini Varadhan [mailto:sowmini.varadhan@oracle.com]
Sent: 19 January 2017 11:14
To: Paul Durrant <redacted>
Cc: Konrad Rzeszutek Wilk <redacted>; Wei Liu
[off-list ref]; netdev@vger.kernel.org; xen-
devel@lists.xenproject.org
Subject: Re: [Xen-devel] xennet_start_xmit assumptions

On (01/19/17 09:36), Paul Durrant wrote:

quoted

Hi Sowmini,

  Sounds like a straightforward bug to me... netfront should be able
to handle an empty skb and clearly, if it's relying on skb_headlen()
being non-zero, that's not the case.

  Paul

I see. Seems like there are 2 things broken here: recovering
from skb->len = 0, and recovering from  the more complex
case of (skb->len > 0 && skb_headlen(skb) == 0)

Do you folks want to take a shot at fixing this,
since you know the code better? If you are interested,
I can share my test program to help you reproduce the
simpler skb->len == 0 case, but it's the fully non-linear
skbs that may be more interesting to reproduce/fix.

I'll probably work on fixing packet_snd to return -EINVAL
or similar when the len is zero this week.

Sowmini,

  I knocked together the following patch, which seems to work for me:

---8<---

diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c
index 40f26b6..a957c89 100644
--- a/drivers/net/xen-netfront.c
+++ b/drivers/net/xen-netfront.c

@@ -567,6 +567,10 @@ static int xennet_start_xmit(struct sk_buff *skb, struct net_device *dev)
        u16 queue_index;
        struct sk_buff *nskb;

+       /* Drop packets that are not at least ETH_HLEN in length */
+       if (skb->len < ETH_HLEN)
+               goto drop;
+
        /* Drop the packet if no queues are set up */
        if (num_queues < 1)
                goto drop;

@@ -609,6 +613,8 @@ static int xennet_start_xmit(struct sk_buff *skb, struct net_device *dev)
        }

        len = skb_headlen(skb);
+       if ((len < ETH_HLEN) && !__pskb_pull_tail(skb, ETH_HLEN))
+               goto drop;

        spin_lock_irqsave(&queue->tx_lock, flags);

---8<---

  Making netfront cope with a fully non-linear skb looks like it would be quite intrusive and probably not worth it so I opted for just doing the ETH_HLEN pull-tail if necessary. Can you check it works for you?

  Paul

On (01/25/17 15:06), Paul Durrant wrote:

  Making netfront cope with a fully non-linear skb looks like it would
be quite intrusive and probably not worth it so I opted for just doing
the ETH_HLEN pull-tail if necessary. Can you check it works for you?

I tested it, and it works fine, but note that DaveM's comments in 
this thread: the DKI is that we *must* provide at least the hard_header_len
in the non-paged part of the skb. So might not even be necessary to handle
the fully non-linear skb (though it's probably prudent to check
and bail for this, as your patch does)

I just posted an RFC patch for fixing the pf_packet layer,
just in case other drivers like xen_netfront dont explicitly
check for this
   http://patchwork.ozlabs.org/patch/719236/