* [PATCH net] bareudp: Fix invalid read beyond skb's linear data
@ 2021-08-06 15:52 Guillaume Nault
2021-08-06 23:22 ` Jakub Kicinski
2021-08-09 22:40 ` patchwork-bot+netdevbpf
0 siblings, 2 replies; 6+ messages in thread
From: Guillaume Nault @ 2021-08-06 15:52 UTC (permalink / raw)
To: David Miller, Jakub Kicinski; +Cc: netdev, Martin Varghese, Willem de Bruijn
Data beyond the UDP header might not be part of the skb's linear data.
Use skb_copy_bits() instead of direct access to skb->data+X, so that
we read the correct bytes even on a fragmented skb.
Fixes: 4b5f67232d95 ("net: Special handling for IP & MPLS.")
Signed-off-by: Guillaume Nault <gnault@redhat.com>
---
drivers/net/bareudp.c | 16 +++++++++++-----
1 file changed, 11 insertions(+), 5 deletions(-)
diff --git a/drivers/net/bareudp.c b/drivers/net/bareudp.c
index a7ee0af1af90..54e321a695ce 100644
--- a/drivers/net/bareudp.c
+++ b/drivers/net/bareudp.c
@@ -71,12 +71,18 @@ static int bareudp_udp_encap_recv(struct sock *sk, struct sk_buff *skb)
family = AF_INET6;
if (bareudp->ethertype == htons(ETH_P_IP)) {
- struct iphdr *iphdr;
+ __u8 ipversion;
- iphdr = (struct iphdr *)(skb->data + BAREUDP_BASE_HLEN);
- if (iphdr->version == 4) {
- proto = bareudp->ethertype;
- } else if (bareudp->multi_proto_mode && (iphdr->version == 6)) {
+ if (skb_copy_bits(skb, BAREUDP_BASE_HLEN, &ipversion,
+ sizeof(ipversion))) {
+ bareudp->dev->stats.rx_dropped++;
+ goto drop;
+ }
+ ipversion >>= 4;
+
+ if (ipversion == 4) {
+ proto = htons(ETH_P_IP);
+ } else if (ipversion == 6 && bareudp->multi_proto_mode) {
proto = htons(ETH_P_IPV6);
} else {
bareudp->dev->stats.rx_dropped++;
--
2.21.3
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH net] bareudp: Fix invalid read beyond skb's linear data
2021-08-06 15:52 [PATCH net] bareudp: Fix invalid read beyond skb's linear data Guillaume Nault
@ 2021-08-06 23:22 ` Jakub Kicinski
2021-08-08 16:16 ` Guillaume Nault
2021-08-09 22:40 ` patchwork-bot+netdevbpf
1 sibling, 1 reply; 6+ messages in thread
From: Jakub Kicinski @ 2021-08-06 23:22 UTC (permalink / raw)
To: Guillaume Nault; +Cc: David Miller, netdev, Martin Varghese, Willem de Bruijn
On Fri, 6 Aug 2021 17:52:06 +0200 Guillaume Nault wrote:
> Data beyond the UDP header might not be part of the skb's linear data.
> Use skb_copy_bits() instead of direct access to skb->data+X, so that
> we read the correct bytes even on a fragmented skb.
>
> Fixes: 4b5f67232d95 ("net: Special handling for IP & MPLS.")
> Signed-off-by: Guillaume Nault <gnault@redhat.com>
> ---
> drivers/net/bareudp.c | 16 +++++++++++-----
> 1 file changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/net/bareudp.c b/drivers/net/bareudp.c
> index a7ee0af1af90..54e321a695ce 100644
> --- a/drivers/net/bareudp.c
> +++ b/drivers/net/bareudp.c
> @@ -71,12 +71,18 @@ static int bareudp_udp_encap_recv(struct sock *sk, struct sk_buff *skb)
> family = AF_INET6;
>
> if (bareudp->ethertype == htons(ETH_P_IP)) {
> - struct iphdr *iphdr;
> + __u8 ipversion;
>
> - iphdr = (struct iphdr *)(skb->data + BAREUDP_BASE_HLEN);
> - if (iphdr->version == 4) {
> - proto = bareudp->ethertype;
> - } else if (bareudp->multi_proto_mode && (iphdr->version == 6)) {
> + if (skb_copy_bits(skb, BAREUDP_BASE_HLEN, &ipversion,
> + sizeof(ipversion))) {
No preference just curious - could skb_header_pointer() be better suited?
> + bareudp->dev->stats.rx_dropped++;
> + goto drop;
> + }
> + ipversion >>= 4;
> +
> + if (ipversion == 4) {
> + proto = htons(ETH_P_IP);
> + } else if (ipversion == 6 && bareudp->multi_proto_mode) {
> proto = htons(ETH_P_IPV6);
> } else {
> bareudp->dev->stats.rx_dropped++;
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH net] bareudp: Fix invalid read beyond skb's linear data
2021-08-06 23:22 ` Jakub Kicinski
@ 2021-08-08 16:16 ` Guillaume Nault
2021-08-09 16:19 ` Jakub Kicinski
0 siblings, 1 reply; 6+ messages in thread
From: Guillaume Nault @ 2021-08-08 16:16 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: David Miller, netdev, Martin Varghese, Willem de Bruijn
On Fri, Aug 06, 2021 at 04:22:34PM -0700, Jakub Kicinski wrote:
> On Fri, 6 Aug 2021 17:52:06 +0200 Guillaume Nault wrote:
> > Data beyond the UDP header might not be part of the skb's linear data.
> > Use skb_copy_bits() instead of direct access to skb->data+X, so that
> > we read the correct bytes even on a fragmented skb.
> >
> > Fixes: 4b5f67232d95 ("net: Special handling for IP & MPLS.")
> > Signed-off-by: Guillaume Nault <gnault@redhat.com>
> > ---
> > drivers/net/bareudp.c | 16 +++++++++++-----
> > 1 file changed, 11 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/net/bareudp.c b/drivers/net/bareudp.c
> > index a7ee0af1af90..54e321a695ce 100644
> > --- a/drivers/net/bareudp.c
> > +++ b/drivers/net/bareudp.c
> > @@ -71,12 +71,18 @@ static int bareudp_udp_encap_recv(struct sock *sk, struct sk_buff *skb)
> > family = AF_INET6;
> >
> > if (bareudp->ethertype == htons(ETH_P_IP)) {
> > - struct iphdr *iphdr;
> > + __u8 ipversion;
> >
> > - iphdr = (struct iphdr *)(skb->data + BAREUDP_BASE_HLEN);
> > - if (iphdr->version == 4) {
> > - proto = bareudp->ethertype;
> > - } else if (bareudp->multi_proto_mode && (iphdr->version == 6)) {
> > + if (skb_copy_bits(skb, BAREUDP_BASE_HLEN, &ipversion,
> > + sizeof(ipversion))) {
>
> No preference just curious - could skb_header_pointer() be better suited?
I have no preference either. I just used skb_copy_bits() because it
didn't seem useful to get a pointer to the buffer (just to read one
byte of data).
But I don't mind reposting with skb_header_pointer() if anyone prefers
that solution.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH net] bareudp: Fix invalid read beyond skb's linear data
2021-08-08 16:16 ` Guillaume Nault
@ 2021-08-09 16:19 ` Jakub Kicinski
2021-08-10 9:33 ` Guillaume Nault
0 siblings, 1 reply; 6+ messages in thread
From: Jakub Kicinski @ 2021-08-09 16:19 UTC (permalink / raw)
To: Guillaume Nault; +Cc: David Miller, netdev, Martin Varghese, Willem de Bruijn
On Sun, 8 Aug 2021 18:16:25 +0200 Guillaume Nault wrote:
> On Fri, Aug 06, 2021 at 04:22:34PM -0700, Jakub Kicinski wrote:
> > On Fri, 6 Aug 2021 17:52:06 +0200 Guillaume Nault wrote:
> > > Data beyond the UDP header might not be part of the skb's linear data.
> > > Use skb_copy_bits() instead of direct access to skb->data+X, so that
> > > we read the correct bytes even on a fragmented skb.
> > >
> > > Fixes: 4b5f67232d95 ("net: Special handling for IP & MPLS.")
> > > Signed-off-by: Guillaume Nault <gnault@redhat.com>
> > > ---
> > > drivers/net/bareudp.c | 16 +++++++++++-----
> > > 1 file changed, 11 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/net/bareudp.c b/drivers/net/bareudp.c
> > > index a7ee0af1af90..54e321a695ce 100644
> > > --- a/drivers/net/bareudp.c
> > > +++ b/drivers/net/bareudp.c
> > > @@ -71,12 +71,18 @@ static int bareudp_udp_encap_recv(struct sock *sk, struct sk_buff *skb)
> > > family = AF_INET6;
> > >
> > > if (bareudp->ethertype == htons(ETH_P_IP)) {
> > > - struct iphdr *iphdr;
> > > + __u8 ipversion;
> > >
> > > - iphdr = (struct iphdr *)(skb->data + BAREUDP_BASE_HLEN);
> > > - if (iphdr->version == 4) {
> > > - proto = bareudp->ethertype;
> > > - } else if (bareudp->multi_proto_mode && (iphdr->version == 6)) {
> > > + if (skb_copy_bits(skb, BAREUDP_BASE_HLEN, &ipversion,
> > > + sizeof(ipversion))) {
> >
> > No preference just curious - could skb_header_pointer() be better suited?
>
> I have no preference either. I just used skb_copy_bits() because it
> didn't seem useful to get a pointer to the buffer (just to read one
> byte of data).
Right, the advantage would be in the "fast" case of skb_header_pointer()
being inlined.
> But I don't mind reposting with skb_header_pointer() if anyone prefers
> that solution.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH net] bareudp: Fix invalid read beyond skb's linear data
2021-08-09 16:19 ` Jakub Kicinski
@ 2021-08-10 9:33 ` Guillaume Nault
0 siblings, 0 replies; 6+ messages in thread
From: Guillaume Nault @ 2021-08-10 9:33 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: David Miller, netdev, Martin Varghese, Willem de Bruijn
On Mon, Aug 09, 2021 at 09:19:18AM -0700, Jakub Kicinski wrote:
> On Sun, 8 Aug 2021 18:16:25 +0200 Guillaume Nault wrote:
> > On Fri, Aug 06, 2021 at 04:22:34PM -0700, Jakub Kicinski wrote:
> > > On Fri, 6 Aug 2021 17:52:06 +0200 Guillaume Nault wrote:
> > > > Data beyond the UDP header might not be part of the skb's linear data.
> > > > Use skb_copy_bits() instead of direct access to skb->data+X, so that
> > > > we read the correct bytes even on a fragmented skb.
> > > >
> > > > Fixes: 4b5f67232d95 ("net: Special handling for IP & MPLS.")
> > > > Signed-off-by: Guillaume Nault <gnault@redhat.com>
> > > > ---
> > > > drivers/net/bareudp.c | 16 +++++++++++-----
> > > > 1 file changed, 11 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/drivers/net/bareudp.c b/drivers/net/bareudp.c
> > > > index a7ee0af1af90..54e321a695ce 100644
> > > > --- a/drivers/net/bareudp.c
> > > > +++ b/drivers/net/bareudp.c
> > > > @@ -71,12 +71,18 @@ static int bareudp_udp_encap_recv(struct sock *sk, struct sk_buff *skb)
> > > > family = AF_INET6;
> > > >
> > > > if (bareudp->ethertype == htons(ETH_P_IP)) {
> > > > - struct iphdr *iphdr;
> > > > + __u8 ipversion;
> > > >
> > > > - iphdr = (struct iphdr *)(skb->data + BAREUDP_BASE_HLEN);
> > > > - if (iphdr->version == 4) {
> > > > - proto = bareudp->ethertype;
> > > > - } else if (bareudp->multi_proto_mode && (iphdr->version == 6)) {
> > > > + if (skb_copy_bits(skb, BAREUDP_BASE_HLEN, &ipversion,
> > > > + sizeof(ipversion))) {
> > >
> > > No preference just curious - could skb_header_pointer() be better suited?
> >
> > I have no preference either. I just used skb_copy_bits() because it
> > didn't seem useful to get a pointer to the buffer (just to read one
> > byte of data).
>
> Right, the advantage would be in the "fast" case of skb_header_pointer()
> being inlined.
Yes indeed. The problem was found because of some automated functionnal
tests, not because of any practical use cases. So I didn't consider the
possible performance differences.
I see that you've applied the patch as is already. I can switch to
skb_header_pointer() in the future, if anyone sees any practical
benefit for it.
Thanks for the review.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net] bareudp: Fix invalid read beyond skb's linear data
2021-08-06 15:52 [PATCH net] bareudp: Fix invalid read beyond skb's linear data Guillaume Nault
2021-08-06 23:22 ` Jakub Kicinski
@ 2021-08-09 22:40 ` patchwork-bot+netdevbpf
1 sibling, 0 replies; 6+ messages in thread
From: patchwork-bot+netdevbpf @ 2021-08-09 22:40 UTC (permalink / raw)
To: Guillaume Nault; +Cc: davem, kuba, netdev, martin.varghese, willemb
Hello:
This patch was applied to netdev/net.git (refs/heads/master):
On Fri, 6 Aug 2021 17:52:06 +0200 you wrote:
> Data beyond the UDP header might not be part of the skb's linear data.
> Use skb_copy_bits() instead of direct access to skb->data+X, so that
> we read the correct bytes even on a fragmented skb.
>
> Fixes: 4b5f67232d95 ("net: Special handling for IP & MPLS.")
> Signed-off-by: Guillaume Nault <gnault@redhat.com>
>
> [...]
Here is the summary with links:
- [net] bareudp: Fix invalid read beyond skb's linear data
https://git.kernel.org/netdev/net/c/143a8526ab5f
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-08-10 9:33 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-06 15:52 [PATCH net] bareudp: Fix invalid read beyond skb's linear data Guillaume Nault
2021-08-06 23:22 ` Jakub Kicinski
2021-08-08 16:16 ` Guillaume Nault
2021-08-09 16:19 ` Jakub Kicinski
2021-08-10 9:33 ` Guillaume Nault
2021-08-09 22:40 ` patchwork-bot+netdevbpf
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).