* xennet_start_xmit assumptions
@ 2017-01-18 15:31 Sowmini Varadhan
2017-01-18 19:25 ` Konrad Rzeszutek Wilk
2017-01-18 19:25 ` [Xen-devel] " Konrad Rzeszutek Wilk
0 siblings, 2 replies; 21+ messages in thread
From: Sowmini Varadhan @ 2017-01-18 15:31 UTC (permalink / raw)
To: netdev, xen-devel
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Xen-devel] xennet_start_xmit assumptions
2017-01-18 15:31 xennet_start_xmit assumptions Sowmini Varadhan
2017-01-18 19:25 ` Konrad Rzeszutek Wilk
@ 2017-01-18 19:25 ` Konrad Rzeszutek Wilk
2017-01-19 9:36 ` Paul Durrant
1 sibling, 1 reply; 21+ messages in thread
From: Konrad Rzeszutek Wilk @ 2017-01-18 19:25 UTC (permalink / raw)
To: Sowmini Varadhan, wei.liu2, paul.durrant; +Cc: netdev, xen-devel
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.
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: xennet_start_xmit assumptions
2017-01-18 15:31 xennet_start_xmit assumptions Sowmini Varadhan
@ 2017-01-18 19:25 ` Konrad Rzeszutek Wilk
2017-01-18 19:25 ` [Xen-devel] " Konrad Rzeszutek Wilk
1 sibling, 0 replies; 21+ messages in thread
From: Konrad Rzeszutek Wilk @ 2017-01-18 19:25 UTC (permalink / raw)
To: Sowmini Varadhan, wei.liu2, paul.durrant; +Cc: netdev, xen-devel
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: xennet_start_xmit assumptions
2017-01-18 19:25 ` [Xen-devel] " Konrad Rzeszutek Wilk
@ 2017-01-19 9:36 ` Paul Durrant
2017-01-19 11:14 ` Sowmini Varadhan
2017-01-19 11:14 ` [Xen-devel] " Sowmini Varadhan
0 siblings, 2 replies; 21+ messages in thread
From: Paul Durrant @ 2017-01-19 9:36 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk, Sowmini Varadhan, Wei Liu; +Cc: netdev, xen-devel
> -----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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Xen-devel] xennet_start_xmit assumptions
2017-01-19 9:36 ` Paul Durrant
2017-01-19 11:14 ` Sowmini Varadhan
@ 2017-01-19 11:14 ` Sowmini Varadhan
2017-01-19 11:31 ` Paul Durrant
` (3 more replies)
1 sibling, 4 replies; 21+ messages in thread
From: Sowmini Varadhan @ 2017-01-19 11:14 UTC (permalink / raw)
To: Paul Durrant; +Cc: Konrad Rzeszutek Wilk, Wei Liu, netdev, 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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: xennet_start_xmit assumptions
2017-01-19 9:36 ` Paul Durrant
@ 2017-01-19 11:14 ` Sowmini Varadhan
2017-01-19 11:14 ` [Xen-devel] " Sowmini Varadhan
1 sibling, 0 replies; 21+ messages in thread
From: Sowmini Varadhan @ 2017-01-19 11:14 UTC (permalink / raw)
To: Paul Durrant; +Cc: netdev, Wei Liu, xen-devel, Konrad Rzeszutek Wilk
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: xennet_start_xmit assumptions
2017-01-19 11:14 ` [Xen-devel] " Sowmini Varadhan
@ 2017-01-19 11:31 ` Paul Durrant
2017-01-19 11:37 ` [Xen-devel] " Sowmini Varadhan
2017-01-19 11:37 ` Sowmini Varadhan
2017-01-19 16:37 ` David Miller
` (2 subsequent siblings)
3 siblings, 2 replies; 21+ messages in thread
From: Paul Durrant @ 2017-01-19 11:31 UTC (permalink / raw)
To: Sowmini Varadhan; +Cc: netdev, Wei Liu, xen-devel, Konrad Rzeszutek Wilk
> -----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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Xen-devel] xennet_start_xmit assumptions
2017-01-19 11:31 ` Paul Durrant
@ 2017-01-19 11:37 ` Sowmini Varadhan
2017-01-19 11:37 ` Sowmini Varadhan
1 sibling, 0 replies; 21+ messages in thread
From: Sowmini Varadhan @ 2017-01-19 11:37 UTC (permalink / raw)
To: Paul Durrant; +Cc: Konrad Rzeszutek Wilk, Wei Liu, netdev, 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");
}
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: xennet_start_xmit assumptions
2017-01-19 11:31 ` Paul Durrant
2017-01-19 11:37 ` [Xen-devel] " Sowmini Varadhan
@ 2017-01-19 11:37 ` Sowmini Varadhan
1 sibling, 0 replies; 21+ messages in thread
From: Sowmini Varadhan @ 2017-01-19 11:37 UTC (permalink / raw)
To: Paul Durrant; +Cc: netdev, Wei Liu, xen-devel, Konrad Rzeszutek Wilk
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: xennet_start_xmit assumptions
2017-01-19 11:14 ` [Xen-devel] " Sowmini Varadhan
2017-01-19 11:31 ` Paul Durrant
@ 2017-01-19 16:37 ` David Miller
2017-01-19 18:47 ` Sowmini Varadhan
2017-01-25 15:06 ` Paul Durrant
2017-01-25 15:06 ` [Xen-devel] " Paul Durrant
3 siblings, 1 reply; 21+ messages in thread
From: David Miller @ 2017-01-19 16:37 UTC (permalink / raw)
To: sowmini.varadhan; +Cc: netdev, Paul.Durrant, wei.liu2, xen-devel, konrad.wilk
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: xennet_start_xmit assumptions
2017-01-19 16:37 ` David Miller
@ 2017-01-19 18:47 ` Sowmini Varadhan
2017-01-19 22:41 ` [Xen-devel] " Sowmini Varadhan
2017-01-19 22:41 ` Sowmini Varadhan
0 siblings, 2 replies; 21+ messages in thread
From: Sowmini Varadhan @ 2017-01-19 18:47 UTC (permalink / raw)
To: David Miller; +Cc: netdev, Paul.Durrant, wei.liu2, xen-devel, konrad.wilk
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Xen-devel] xennet_start_xmit assumptions
2017-01-19 18:47 ` Sowmini Varadhan
@ 2017-01-19 22:41 ` Sowmini Varadhan
2017-01-20 19:30 ` David Miller
2017-01-20 19:30 ` David Miller
2017-01-19 22:41 ` Sowmini Varadhan
1 sibling, 2 replies; 21+ messages in thread
From: Sowmini Varadhan @ 2017-01-19 22:41 UTC (permalink / raw)
To: David Miller; +Cc: Paul.Durrant, konrad.wilk, wei.liu2, netdev, xen-devel
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: xennet_start_xmit assumptions
2017-01-19 18:47 ` Sowmini Varadhan
2017-01-19 22:41 ` [Xen-devel] " Sowmini Varadhan
@ 2017-01-19 22:41 ` Sowmini Varadhan
1 sibling, 0 replies; 21+ messages in thread
From: Sowmini Varadhan @ 2017-01-19 22:41 UTC (permalink / raw)
To: David Miller; +Cc: netdev, Paul.Durrant, wei.liu2, xen-devel, konrad.wilk
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Xen-devel] xennet_start_xmit assumptions
2017-01-19 22:41 ` [Xen-devel] " Sowmini Varadhan
@ 2017-01-20 19:30 ` David Miller
2017-01-20 20:03 ` Sowmini Varadhan
2017-01-20 20:03 ` [Xen-devel] " Sowmini Varadhan
2017-01-20 19:30 ` David Miller
1 sibling, 2 replies; 21+ messages in thread
From: David Miller @ 2017-01-20 19:30 UTC (permalink / raw)
To: sowmini.varadhan; +Cc: Paul.Durrant, konrad.wilk, wei.liu2, netdev, xen-devel
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.
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: xennet_start_xmit assumptions
2017-01-19 22:41 ` [Xen-devel] " Sowmini Varadhan
2017-01-20 19:30 ` David Miller
@ 2017-01-20 19:30 ` David Miller
1 sibling, 0 replies; 21+ messages in thread
From: David Miller @ 2017-01-20 19:30 UTC (permalink / raw)
To: sowmini.varadhan; +Cc: netdev, Paul.Durrant, wei.liu2, xen-devel, konrad.wilk
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Xen-devel] xennet_start_xmit assumptions
2017-01-20 19:30 ` David Miller
2017-01-20 20:03 ` Sowmini Varadhan
@ 2017-01-20 20:03 ` Sowmini Varadhan
1 sibling, 0 replies; 21+ messages in thread
From: Sowmini Varadhan @ 2017-01-20 20:03 UTC (permalink / raw)
To: David Miller; +Cc: Paul.Durrant, konrad.wilk, wei.liu2, netdev, xen-devel
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: xennet_start_xmit assumptions
2017-01-20 19:30 ` David Miller
@ 2017-01-20 20:03 ` Sowmini Varadhan
2017-01-20 20:03 ` [Xen-devel] " Sowmini Varadhan
1 sibling, 0 replies; 21+ messages in thread
From: Sowmini Varadhan @ 2017-01-20 20:03 UTC (permalink / raw)
To: David Miller; +Cc: netdev, Paul.Durrant, wei.liu2, xen-devel, konrad.wilk
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* RE: [Xen-devel] xennet_start_xmit assumptions
2017-01-19 11:14 ` [Xen-devel] " Sowmini Varadhan
` (2 preceding siblings ...)
2017-01-25 15:06 ` Paul Durrant
@ 2017-01-25 15:06 ` Paul Durrant
2017-01-25 15:45 ` Sowmini Varadhan
2017-01-25 15:45 ` Sowmini Varadhan
3 siblings, 2 replies; 21+ messages in thread
From: Paul Durrant @ 2017-01-25 15:06 UTC (permalink / raw)
To: Sowmini Varadhan; +Cc: Konrad Rzeszutek Wilk, Wei Liu, netdev, xen-devel
> -----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
^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: xennet_start_xmit assumptions
2017-01-19 11:14 ` [Xen-devel] " Sowmini Varadhan
2017-01-19 11:31 ` Paul Durrant
2017-01-19 16:37 ` David Miller
@ 2017-01-25 15:06 ` Paul Durrant
2017-01-25 15:06 ` [Xen-devel] " Paul Durrant
3 siblings, 0 replies; 21+ messages in thread
From: Paul Durrant @ 2017-01-25 15:06 UTC (permalink / raw)
To: Sowmini Varadhan; +Cc: netdev, Wei Liu, xen-devel
> -----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
^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [Xen-devel] xennet_start_xmit assumptions
2017-01-25 15:06 ` [Xen-devel] " Paul Durrant
@ 2017-01-25 15:45 ` Sowmini Varadhan
2017-01-25 15:45 ` Sowmini Varadhan
1 sibling, 0 replies; 21+ messages in thread
From: Sowmini Varadhan @ 2017-01-25 15:45 UTC (permalink / raw)
To: Paul Durrant; +Cc: Konrad Rzeszutek Wilk, Wei Liu, netdev, xen-devel
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/
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: xennet_start_xmit assumptions
2017-01-25 15:06 ` [Xen-devel] " Paul Durrant
2017-01-25 15:45 ` Sowmini Varadhan
@ 2017-01-25 15:45 ` Sowmini Varadhan
1 sibling, 0 replies; 21+ messages in thread
From: Sowmini Varadhan @ 2017-01-25 15:45 UTC (permalink / raw)
To: Paul Durrant; +Cc: netdev, Wei Liu, xen-devel
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
^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2017-01-25 15:46 UTC | newest]
Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-01-18 15:31 xennet_start_xmit assumptions Sowmini Varadhan
2017-01-18 19:25 ` Konrad Rzeszutek Wilk
2017-01-18 19:25 ` [Xen-devel] " Konrad Rzeszutek Wilk
2017-01-19 9:36 ` Paul Durrant
2017-01-19 11:14 ` Sowmini Varadhan
2017-01-19 11:14 ` [Xen-devel] " Sowmini Varadhan
2017-01-19 11:31 ` Paul Durrant
2017-01-19 11:37 ` [Xen-devel] " Sowmini Varadhan
2017-01-19 11:37 ` Sowmini Varadhan
2017-01-19 16:37 ` David Miller
2017-01-19 18:47 ` Sowmini Varadhan
2017-01-19 22:41 ` [Xen-devel] " Sowmini Varadhan
2017-01-20 19:30 ` David Miller
2017-01-20 20:03 ` Sowmini Varadhan
2017-01-20 20:03 ` [Xen-devel] " Sowmini Varadhan
2017-01-20 19:30 ` David Miller
2017-01-19 22:41 ` Sowmini Varadhan
2017-01-25 15:06 ` Paul Durrant
2017-01-25 15:06 ` [Xen-devel] " Paul Durrant
2017-01-25 15:45 ` Sowmini Varadhan
2017-01-25 15:45 ` Sowmini Varadhan
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.