Re: xennet_start_xmit assumptions

Re: xennet_start_xmit assumptions

[Konrad Rzeszutek Wilk]

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.

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

Re: xennet_start_xmit assumptions

[Paul Durrant]

> -----Original Message-----
> From: Konrad Rzeszutek Wilk [mailto:konrad.wilk@oracle.com]
> Sent: 18 January 2017 19:25
> To: Sowmini Varadhan <sowmini.varadhan@oracle.com>; Wei Liu
> <wei.liu2@citrix.com>; Paul Durrant <Paul.Durrant@citrix.com>
> 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:
> > 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

> >
> > --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

Re: xennet_start_xmit assumptions

[Sowmini Varadhan]

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


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

Re: xennet_start_xmit assumptions

[Paul Durrant]

> -----Original Message-----
> From: Sowmini Varadhan [mailto:sowmini.varadhan@oracle.com]
> Sent: 19 January 2017 11:14
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>; Wei Liu
> <wei.liu2@citrix.com>; 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:
> >
> > 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

Re: xennet_start_xmit assumptions

[Sowmini Varadhan]

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");
}


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

Re: xennet_start_xmit assumptions

[David Miller]

From: Sowmini Varadhan <sowmini.varadhan@oracle.com>
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

Re: xennet_start_xmit assumptions

[Sowmini Varadhan]

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

Re: xennet_start_xmit assumptions

[Sowmini Varadhan]

On (01/19/17 13:47), Sowmini Varadhan wrote:
> > 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


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

Re: xennet_start_xmit assumptions

[David Miller]

From: Sowmini Varadhan <sowmini.varadhan@oracle.com>
Date: Thu, 19 Jan 2017 17:41:23 -0500

> On (01/19/17 13:47), Sowmini Varadhan wrote:
>> > 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.

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

Re: xennet_start_xmit assumptions

[Sowmini Varadhan]

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


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

Re: xennet_start_xmit assumptions

[Paul Durrant]

> -----Original Message-----
> From: Sowmini Varadhan [mailto:sowmini.varadhan@oracle.com]
> Sent: 19 January 2017 11:14
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>; Wei Liu
> <wei.liu2@citrix.com>; 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:
> >
> > 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


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

Re: xennet_start_xmit assumptions

[Sowmini Varadhan]

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/


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

xennet_start_xmit assumptions

[Sowmini Varadhan]

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


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