| Message ID | 1317058869-19276-1-git-send-email-levinsasha928@gmail.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On Mon, Sep 26, 2011 at 08:41:08PM +0300, Sasha Levin wrote: > This patch verifies that the length of a buffer stored in a linked list > of pages is small enough to fit into a skb. > > If the size is larger than a max size of a skb, it means that we shouldn't > go ahead building skbs anyway since we won't be able to send the buffer as > the user requested. > > Cc: Rusty Russell <rusty@rustcorp.com.au> > Cc: "Michael S. Tsirkin" <mst@redhat.com> > Cc: virtualization@lists.linux-foundation.org > Cc: netdev@vger.kernel.org > Cc: kvm@vger.kernel.org > Signed-off-by: Sasha Levin <levinsasha928@gmail.com> Interesting. This is a theoretical issue, correct? Not a crash you actually see. This crash would mean device is giving us packets that are way too large. Avoiding crashes even in the face of a misbehaved device is a good idea, but should we print a diagnostic to a system log? Maybe rate-limited or print once to avoid filling up the disk. Other places in driver print with pr_debug I'm not sure that's right but better than nothing. > --- > drivers/net/virtio_net.c | 3 +++ > 1 files changed, 3 insertions(+), 0 deletions(-) > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > index 0c7321c..64e0717 100644 > --- a/drivers/net/virtio_net.c > +++ b/drivers/net/virtio_net.c > @@ -165,6 +165,9 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi, > unsigned int copy, hdr_len, offset; > char *p; > > + if (len > MAX_SKB_FRAGS * PAGE_SIZE) unlikely()? Also, this seems too aggressive: at this point len includes the header and the linear part. The right place for this test is probably where we fill in the frags, just before while (len) The whole can only happen when mergeable buffers are disabled, right? > + return NULL; > + > p = page_address(page); > > /* copy small packet so we can reuse these pages for small data */ > -- > 1.7.6.1 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2011-09-26 at 21:44 +0300, Michael S. Tsirkin wrote: > On Mon, Sep 26, 2011 at 08:41:08PM +0300, Sasha Levin wrote: > > This patch verifies that the length of a buffer stored in a linked list > > of pages is small enough to fit into a skb. > > > > If the size is larger than a max size of a skb, it means that we shouldn't > > go ahead building skbs anyway since we won't be able to send the buffer as > > the user requested. > > > > Cc: Rusty Russell <rusty@rustcorp.com.au> > > Cc: "Michael S. Tsirkin" <mst@redhat.com> > > Cc: virtualization@lists.linux-foundation.org > > Cc: netdev@vger.kernel.org > > Cc: kvm@vger.kernel.org > > Signed-off-by: Sasha Levin <levinsasha928@gmail.com> > > Interesting. This is a theoretical issue, correct? > Not a crash you actually see. Actually it was an actual crash caused when our virtio-net driver in kvm tools did funny things and passed '(u32)-1' length as a buffer length to the guest kernel. > This crash would mean device is giving us packets > that are way too large. Avoiding crashes even in the face of > a misbehaved device is a good idea, but should > we print a diagnostic to a system log? > Maybe rate-limited or print once to avoid filling > up the disk. Other places in driver print with pr_debug > I'm not sure that's right but better than nothing. Yup, I'll add some debug info. > > --- > > drivers/net/virtio_net.c | 3 +++ > > 1 files changed, 3 insertions(+), 0 deletions(-) > > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > > index 0c7321c..64e0717 100644 > > --- a/drivers/net/virtio_net.c > > +++ b/drivers/net/virtio_net.c > > @@ -165,6 +165,9 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi, > > unsigned int copy, hdr_len, offset; > > char *p; > > > > + if (len > MAX_SKB_FRAGS * PAGE_SIZE) > > unlikely()? > > Also, this seems too aggressive: at this point len includes the header > and the linear part. The right place for this > test is probably where we fill in the frags, just before > while (len) > > The whole can only happen when mergeable buffers > are disabled, right? From what I understand it can happen whenever you're going to build a skb longer than PAGE_SIZE.
On Mon, Sep 26, 2011 at 10:37 PM, Sasha Levin <levinsasha928@gmail.com> wrote: >> Interesting. This is a theoretical issue, correct? >> Not a crash you actually see. > > Actually it was an actual crash caused when our virtio-net driver in kvm > tools did funny things and passed '(u32)-1' length as a buffer length to > the guest kernel. I'm not sure what Michael means with "theoretical issue" here. Can the guest driver assume that the hypervisor doesn't attempt to do nasty things? Pekka -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Sep 26, 2011 at 10:37:22PM +0300, Sasha Levin wrote: > On Mon, 2011-09-26 at 21:44 +0300, Michael S. Tsirkin wrote: > > On Mon, Sep 26, 2011 at 08:41:08PM +0300, Sasha Levin wrote: > > > This patch verifies that the length of a buffer stored in a linked list > > > of pages is small enough to fit into a skb. > > > > > > If the size is larger than a max size of a skb, it means that we shouldn't > > > go ahead building skbs anyway since we won't be able to send the buffer as > > > the user requested. > > > > > > Cc: Rusty Russell <rusty@rustcorp.com.au> > > > Cc: "Michael S. Tsirkin" <mst@redhat.com> > > > Cc: virtualization@lists.linux-foundation.org > > > Cc: netdev@vger.kernel.org > > > Cc: kvm@vger.kernel.org > > > Signed-off-by: Sasha Levin <levinsasha928@gmail.com> > > > > Interesting. This is a theoretical issue, correct? > > Not a crash you actually see. > > Actually it was an actual crash caused when our virtio-net driver in kvm > tools did funny things and passed '(u32)-1' length as a buffer length to > the guest kernel. > > > This crash would mean device is giving us packets > > that are way too large. Avoiding crashes even in the face of > > a misbehaved device is a good idea, but should > > we print a diagnostic to a system log? > > Maybe rate-limited or print once to avoid filling > > up the disk. Other places in driver print with pr_debug > > I'm not sure that's right but better than nothing. > > Yup, I'll add some debug info. > > > > --- > > > drivers/net/virtio_net.c | 3 +++ > > > 1 files changed, 3 insertions(+), 0 deletions(-) > > > > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > > > index 0c7321c..64e0717 100644 > > > --- a/drivers/net/virtio_net.c > > > +++ b/drivers/net/virtio_net.c > > > @@ -165,6 +165,9 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi, > > > unsigned int copy, hdr_len, offset; > > > char *p; > > > > > > + if (len > MAX_SKB_FRAGS * PAGE_SIZE) > > > > unlikely()? > > > > Also, this seems too aggressive: at this point len includes the header > > and the linear part. The right place for this > > test is probably where we fill in the frags, just before > > while (len) > > > > The whole can only happen when mergeable buffers > > are disabled, right? > > >From what I understand it can happen whenever you're going to build a > skb longer than PAGE_SIZE. Hmm how exactly? With mergeable buffers this only gets the length of the 1st chunk which is up to 4K unless the driver is buggy ... > -- > > Sasha. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Sep 26, 2011 at 10:45:35PM +0300, Pekka Enberg wrote: > On Mon, Sep 26, 2011 at 10:37 PM, Sasha Levin <levinsasha928@gmail.com> wrote: > >> Interesting. This is a theoretical issue, correct? > >> Not a crash you actually see. > > > > Actually it was an actual crash caused when our virtio-net driver in kvm > > tools did funny things and passed '(u32)-1' length as a buffer length to > > the guest kernel. > > I'm not sure what Michael means with "theoretical issue" here. Can the guest > driver assume that the hypervisor doesn't attempt to do nasty things? > > Pekka IMO yes, hypervisor has full access to guest memory so it's a safe assumption. But surviving in the face of hypervisor bugs is laudable goal, bugs do happen.
On Mon, 2011-09-26 at 22:45 +0300, Pekka Enberg wrote: > On Mon, Sep 26, 2011 at 10:37 PM, Sasha Levin <levinsasha928@gmail.com> wrote: > >> Interesting. This is a theoretical issue, correct? > >> Not a crash you actually see. > > > > Actually it was an actual crash caused when our virtio-net driver in kvm > > tools did funny things and passed '(u32)-1' length as a buffer length to > > the guest kernel. > > I'm not sure what Michael means with "theoretical issue" here. Can the guest > driver assume that the hypervisor doesn't attempt to do nasty things? afaik if the hypervisor can access the vcpus and the memory of the guest, this shouldn't be a security issue - more of a bug prevention issue. I guess it'll be interesting the other way around, when it's the guest that passes this buggy information to the hypervisor.
On Mon, Sep 26, 2011 at 10:45:35PM +0300, Pekka Enberg wrote: >> I'm not sure what Michael means with "theoretical issue" here. Can the guest >> driver assume that the hypervisor doesn't attempt to do nasty things? On Mon, Sep 26, 2011 at 10:57 PM, Michael S. Tsirkin <mst@redhat.com> wrote: > IMO yes, hypervisor has full access to guest memory so it's a safe > assumption. But surviving in the face of hypervisor bugs > is laudable goal, bugs do happen. I was thinking of a compromised guest that is able to trick the hypervisor into doing nasty things to other guests without taking over the hypervisor completely. So for something like virtio networking, that's by definition exposed to rest of the world, I think it's very important not to be robust against hypervisor bugs. In any case, we were able to trigger this particular case rather easily with our buggy tool, so it's definitely worth fixing. ;-) FWIW, Acked-by: Pekka Enberg <penberg@kernel.org> Pekka -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2011-09-26 at 22:55 +0300, Michael S. Tsirkin wrote: > On Mon, Sep 26, 2011 at 10:37:22PM +0300, Sasha Levin wrote: > > On Mon, 2011-09-26 at 21:44 +0300, Michael S. Tsirkin wrote: > > > On Mon, Sep 26, 2011 at 08:41:08PM +0300, Sasha Levin wrote: > > > > This patch verifies that the length of a buffer stored in a linked list > > > > of pages is small enough to fit into a skb. > > > > > > > > If the size is larger than a max size of a skb, it means that we shouldn't > > > > go ahead building skbs anyway since we won't be able to send the buffer as > > > > the user requested. > > > > > > > > Cc: Rusty Russell <rusty@rustcorp.com.au> > > > > Cc: "Michael S. Tsirkin" <mst@redhat.com> > > > > Cc: virtualization@lists.linux-foundation.org > > > > Cc: netdev@vger.kernel.org > > > > Cc: kvm@vger.kernel.org > > > > Signed-off-by: Sasha Levin <levinsasha928@gmail.com> > > > > > > Interesting. This is a theoretical issue, correct? > > > Not a crash you actually see. > > > > Actually it was an actual crash caused when our virtio-net driver in kvm > > tools did funny things and passed '(u32)-1' length as a buffer length to > > the guest kernel. > > > > > This crash would mean device is giving us packets > > > that are way too large. Avoiding crashes even in the face of > > > a misbehaved device is a good idea, but should > > > we print a diagnostic to a system log? > > > Maybe rate-limited or print once to avoid filling > > > up the disk. Other places in driver print with pr_debug > > > I'm not sure that's right but better than nothing. > > > > Yup, I'll add some debug info. > > > > > > --- > > > > drivers/net/virtio_net.c | 3 +++ > > > > 1 files changed, 3 insertions(+), 0 deletions(-) > > > > > > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > > > > index 0c7321c..64e0717 100644 > > > > --- a/drivers/net/virtio_net.c > > > > +++ b/drivers/net/virtio_net.c > > > > @@ -165,6 +165,9 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi, > > > > unsigned int copy, hdr_len, offset; > > > > char *p; > > > > > > > > + if (len > MAX_SKB_FRAGS * PAGE_SIZE) > > > > > > unlikely()? > > > > > > Also, this seems too aggressive: at this point len includes the header > > > and the linear part. The right place for this > > > test is probably where we fill in the frags, just before > > > while (len) > > > > > > The whole can only happen when mergeable buffers > > > are disabled, right? > > > > >From what I understand it can happen whenever you're going to build a > > skb longer than PAGE_SIZE. > > Hmm how exactly? With mergeable buffers this only gets > the length of the 1st chunk which is up to 4K unless the driver > is buggy ... What about the case where TSO or ECN features are set? The code flow suggests that mergeable would get ignored in that case: /* If we can receive ANY GSO packets, we must allocate large ones. */ if (virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_TSO4) || virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_TSO6) || virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_ECN)) vi->big_packets = true; [...] if (!vi->mergeable_rx_bufs && !vi->big_packets) { skb = buf; len -= sizeof(struct virtio_net_hdr); skb_trim(skb, len); } else { page = buf; skb = page_to_skb(vi, page, len); ... I haven't actually tested it with mergeable buffers enabled, but could do it later today.
On Tue, Sep 27, 2011 at 09:44:02AM +0300, Sasha Levin wrote: > On Mon, 2011-09-26 at 22:55 +0300, Michael S. Tsirkin wrote: > > On Mon, Sep 26, 2011 at 10:37:22PM +0300, Sasha Levin wrote: > > > On Mon, 2011-09-26 at 21:44 +0300, Michael S. Tsirkin wrote: > > > > On Mon, Sep 26, 2011 at 08:41:08PM +0300, Sasha Levin wrote: > > > > > This patch verifies that the length of a buffer stored in a linked list > > > > > of pages is small enough to fit into a skb. > > > > > > > > > > If the size is larger than a max size of a skb, it means that we shouldn't > > > > > go ahead building skbs anyway since we won't be able to send the buffer as > > > > > the user requested. > > > > > > > > > > Cc: Rusty Russell <rusty@rustcorp.com.au> > > > > > Cc: "Michael S. Tsirkin" <mst@redhat.com> > > > > > Cc: virtualization@lists.linux-foundation.org > > > > > Cc: netdev@vger.kernel.org > > > > > Cc: kvm@vger.kernel.org > > > > > Signed-off-by: Sasha Levin <levinsasha928@gmail.com> > > > > > > > > Interesting. This is a theoretical issue, correct? > > > > Not a crash you actually see. > > > > > > Actually it was an actual crash caused when our virtio-net driver in kvm > > > tools did funny things and passed '(u32)-1' length as a buffer length to > > > the guest kernel. > > > > > > > This crash would mean device is giving us packets > > > > that are way too large. Avoiding crashes even in the face of > > > > a misbehaved device is a good idea, but should > > > > we print a diagnostic to a system log? > > > > Maybe rate-limited or print once to avoid filling > > > > up the disk. Other places in driver print with pr_debug > > > > I'm not sure that's right but better than nothing. > > > > > > Yup, I'll add some debug info. > > > > > > > > --- > > > > > drivers/net/virtio_net.c | 3 +++ > > > > > 1 files changed, 3 insertions(+), 0 deletions(-) > > > > > > > > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > > > > > index 0c7321c..64e0717 100644 > > > > > --- a/drivers/net/virtio_net.c > > > > > +++ b/drivers/net/virtio_net.c > > > > > @@ -165,6 +165,9 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi, > > > > > unsigned int copy, hdr_len, offset; > > > > > char *p; > > > > > > > > > > + if (len > MAX_SKB_FRAGS * PAGE_SIZE) > > > > > > > > unlikely()? > > > > > > > > Also, this seems too aggressive: at this point len includes the header > > > > and the linear part. The right place for this > > > > test is probably where we fill in the frags, just before > > > > while (len) > > > > > > > > The whole can only happen when mergeable buffers > > > > are disabled, right? > > > > > > >From what I understand it can happen whenever you're going to build a > > > skb longer than PAGE_SIZE. > > > > Hmm how exactly? With mergeable buffers this only gets > > the length of the 1st chunk which is up to 4K unless the driver > > is buggy ... > > What about the case where TSO or ECN features are set? The code flow > suggests that mergeable would get ignored in that case: > > /* If we can receive ANY GSO packets, we must allocate large ones. */ > if (virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_TSO4) || > virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_TSO6) || > virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_ECN)) > vi->big_packets = true; > > [...] > > if (!vi->mergeable_rx_bufs && !vi->big_packets) { > skb = buf; > len -= sizeof(struct virtio_net_hdr); > skb_trim(skb, len); > } else { > page = buf; > skb = page_to_skb(vi, page, len); > ... Sorry I don't get it yet. Where is mergeable ignored here? > I haven't actually tested it with mergeable buffers enabled, but could > do it later today. Please do. > -- > > Sasha. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2011-09-27 at 10:00 +0300, Michael S. Tsirkin wrote: > > skb = page_to_skb(vi, page, len); > > ... > > Sorry I don't get it yet. Where is mergeable ignored here? The NULL deref happens in page_to_skb(), before merge buffers are handled. I'll test it and see if it's really the case.
On Tue, Sep 27, 2011 at 02:20:29PM +0300, Sasha Levin wrote: > On Tue, 2011-09-27 at 10:00 +0300, Michael S. Tsirkin wrote: > > > skb = page_to_skb(vi, page, len); > > > ... > > > > Sorry I don't get it yet. Where is mergeable ignored here? > > The NULL deref happens in page_to_skb(), before merge buffers are > handled. The len here is a single buffer length, so for mergeable buffers it is <= the size of the buffer we gave to hardware, which is PAGE_SIZE. err = virtqueue_add_buf_single(vi->rvq, page_address(page), PAGE_SIZE, false, page, gfp); Unless of course we are trying to work around broken hardware again, which I don't have a problem with, but should probably get appropriate comments in code and trigger a warning. > I'll test it and see if it's really the case. > > -- > > Sasha. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2011-09-27 at 15:37 +0300, Michael S. Tsirkin wrote: > On Tue, Sep 27, 2011 at 02:20:29PM +0300, Sasha Levin wrote: > > On Tue, 2011-09-27 at 10:00 +0300, Michael S. Tsirkin wrote: > > > > skb = page_to_skb(vi, page, len); > > > > ... > > > > > > Sorry I don't get it yet. Where is mergeable ignored here? > > > > The NULL deref happens in page_to_skb(), before merge buffers are > > handled. > > The len here is a single buffer length, so for mergeable > buffers it is <= the size of the buffer we gave to hardware, > which is PAGE_SIZE. > > err = virtqueue_add_buf_single(vi->rvq, page_address(page), > PAGE_SIZE, false, page, gfp); > > > Unless of course we are trying to work around broken hardware again, > which I don't have a problem with, but should probably > get appropriate comments in code and trigger a warning. > > > I'll test it and see if it's really the case. I've verified it with VIRTIO_NET_F_MRG_RXBUF set, and we still get the NULL deref.
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c index 0c7321c..64e0717 100644 --- a/drivers/net/virtio_net.c +++ b/drivers/net/virtio_net.c @@ -165,6 +165,9 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi, unsigned int copy, hdr_len, offset; char *p; + if (len > MAX_SKB_FRAGS * PAGE_SIZE) + return NULL; + p = page_address(page); /* copy small packet so we can reuse these pages for small data */
This patch verifies that the length of a buffer stored in a linked list of pages is small enough to fit into a skb. If the size is larger than a max size of a skb, it means that we shouldn't go ahead building skbs anyway since we won't be able to send the buffer as the user requested. Cc: Rusty Russell <rusty@rustcorp.com.au> Cc: "Michael S. Tsirkin" <mst@redhat.com> Cc: virtualization@lists.linux-foundation.org Cc: netdev@vger.kernel.org Cc: kvm@vger.kernel.org Signed-off-by: Sasha Levin <levinsasha928@gmail.com> --- drivers/net/virtio_net.c | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-)