| Message ID | dc0d0b1bc3c67e2a1346b0dd1f68428eb956fbb7.1565649789.git.sbrivio@redhat.com |
|---|---|
| State | Accepted |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net] ipv6: Fix return value of ipv6_mc_may_pull() for malformed packets | expand |
On Tue, Aug 13, 2019 at 12:46:01AM +0200, Stefano Brivio wrote: > Commit ba5ea614622d ("bridge: simplify ip_mc_check_igmp() and > ipv6_mc_check_mld() calls") replaces direct calls to pskb_may_pull() > in br_ipv6_multicast_mld2_report() with calls to ipv6_mc_may_pull(), > that returns -EINVAL on buffers too short to be valid IPv6 packets, > while maintaining the previous handling of the return code. > > This leads to the direct opposite of the intended effect: if the > packet is malformed, -EINVAL evaluates as true, and we'll happily > proceed with the processing. > > Return 0 if the packet is too short, in the same way as this was > fixed for IPv4 by commit 083b78a9ed64 ("ip: fix ip_mc_may_pull() > return value"). > > I don't have a reproducer for this, unlike the one referred to by > the IPv4 commit, but this is clearly broken. > > Fixes: ba5ea614622d ("bridge: simplify ip_mc_check_igmp() and ipv6_mc_check_mld() calls") > Signed-off-by: Stefano Brivio <sbrivio@redhat.com> > --- > include/net/addrconf.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/include/net/addrconf.h b/include/net/addrconf.h > index becdad576859..3f62b347b04a 100644 > --- a/include/net/addrconf.h > +++ b/include/net/addrconf.h > @@ -206,7 +206,7 @@ static inline int ipv6_mc_may_pull(struct sk_buff *skb, > unsigned int len) > { > if (skb_transport_offset(skb) + ipv6_transport_len(skb) < len) > - return -EINVAL; > + return 0; > > return pskb_may_pull(skb, len); > } Acked-by: Guillaume Nault <gnault@redhat.com>
From: Stefano Brivio <sbrivio@redhat.com> Date: Tue, 13 Aug 2019 00:46:01 +0200 > Commit ba5ea614622d ("bridge: simplify ip_mc_check_igmp() and > ipv6_mc_check_mld() calls") replaces direct calls to pskb_may_pull() > in br_ipv6_multicast_mld2_report() with calls to ipv6_mc_may_pull(), > that returns -EINVAL on buffers too short to be valid IPv6 packets, > while maintaining the previous handling of the return code. > > This leads to the direct opposite of the intended effect: if the > packet is malformed, -EINVAL evaluates as true, and we'll happily > proceed with the processing. > > Return 0 if the packet is too short, in the same way as this was > fixed for IPv4 by commit 083b78a9ed64 ("ip: fix ip_mc_may_pull() > return value"). > > I don't have a reproducer for this, unlike the one referred to by > the IPv4 commit, but this is clearly broken. > > Fixes: ba5ea614622d ("bridge: simplify ip_mc_check_igmp() and ipv6_mc_check_mld() calls") > Signed-off-by: Stefano Brivio <sbrivio@redhat.com> Applied and queued up for -stable.
On Wed, Aug 14, 2019 at 12:58:58PM -0400, David Miller wrote: > From: Stefano Brivio <sbrivio@redhat.com> > Date: Tue, 13 Aug 2019 00:46:01 +0200 > > > Commit ba5ea614622d ("bridge: simplify ip_mc_check_igmp() and > > ipv6_mc_check_mld() calls") replaces direct calls to pskb_may_pull() > > in br_ipv6_multicast_mld2_report() with calls to ipv6_mc_may_pull(), > > that returns -EINVAL on buffers too short to be valid IPv6 packets, > > while maintaining the previous handling of the return code. > > > > This leads to the direct opposite of the intended effect: if the > > packet is malformed, -EINVAL evaluates as true, and we'll happily > > proceed with the processing. > > > > Return 0 if the packet is too short, in the same way as this was > > fixed for IPv4 by commit 083b78a9ed64 ("ip: fix ip_mc_may_pull() > > return value"). > > > > I don't have a reproducer for this, unlike the one referred to by > > the IPv4 commit, but this is clearly broken. > > > > Fixes: ba5ea614622d ("bridge: simplify ip_mc_check_igmp() and ipv6_mc_check_mld() calls") > > Signed-off-by: Stefano Brivio <sbrivio@redhat.com> > > Applied and queued up for -stable. Urgh, sorry... and thanks for the fix(es), absolutely right...
Hi, On Wed, 14 Aug 2019 12:58:58 -0400 (EDT) David Miller <davem@davemloft.net> wrote: > From: Stefano Brivio <sbrivio@redhat.com> > Date: Tue, 13 Aug 2019 00:46:01 +0200 > > > Commit ba5ea614622d ("bridge: simplify ip_mc_check_igmp() and > > ipv6_mc_check_mld() calls") replaces direct calls to pskb_may_pull() > > in br_ipv6_multicast_mld2_report() with calls to ipv6_mc_may_pull(), > > that returns -EINVAL on buffers too short to be valid IPv6 packets, > > while maintaining the previous handling of the return code. > > > > This leads to the direct opposite of the intended effect: if the > > packet is malformed, -EINVAL evaluates as true, and we'll happily > > proceed with the processing. > > > > Return 0 if the packet is too short, in the same way as this was > > fixed for IPv4 by commit 083b78a9ed64 ("ip: fix ip_mc_may_pull() > > return value"). > > > > I don't have a reproducer for this, unlike the one referred to by > > the IPv4 commit, but this is clearly broken. > > > > Fixes: ba5ea614622d ("bridge: simplify ip_mc_check_igmp() and ipv6_mc_check_mld() calls") > > Signed-off-by: Stefano Brivio <sbrivio@redhat.com> > > Applied and queued up for -stable. I don't see this on net.git, but it's in your stable bundle on Patchwork. Should I resend? Thanks.
From: Stefano Brivio <sbrivio@redhat.com> Date: Mon, 19 Aug 2019 12:12:52 +0200 > I don't see this on net.git, but it's in your stable bundle on > Patchwork. Should I resend? Thanks. I applied it on my laptop while travelling and never pushed it out so it just rot there, sorry. Fixed, should be in 'net' now.
diff --git a/include/net/addrconf.h b/include/net/addrconf.h index becdad576859..3f62b347b04a 100644 --- a/include/net/addrconf.h +++ b/include/net/addrconf.h @@ -206,7 +206,7 @@ static inline int ipv6_mc_may_pull(struct sk_buff *skb, unsigned int len) { if (skb_transport_offset(skb) + ipv6_transport_len(skb) < len) - return -EINVAL; + return 0; return pskb_may_pull(skb, len); }
Commit ba5ea614622d ("bridge: simplify ip_mc_check_igmp() and ipv6_mc_check_mld() calls") replaces direct calls to pskb_may_pull() in br_ipv6_multicast_mld2_report() with calls to ipv6_mc_may_pull(), that returns -EINVAL on buffers too short to be valid IPv6 packets, while maintaining the previous handling of the return code. This leads to the direct opposite of the intended effect: if the packet is malformed, -EINVAL evaluates as true, and we'll happily proceed with the processing. Return 0 if the packet is too short, in the same way as this was fixed for IPv4 by commit 083b78a9ed64 ("ip: fix ip_mc_may_pull() return value"). I don't have a reproducer for this, unlike the one referred to by the IPv4 commit, but this is clearly broken. Fixes: ba5ea614622d ("bridge: simplify ip_mc_check_igmp() and ipv6_mc_check_mld() calls") Signed-off-by: Stefano Brivio <sbrivio@redhat.com> --- include/net/addrconf.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)