Message ID | d21f4a12e70bf50ff8f719f3d16a9ddc1b6cd0bb.1551785347.git.pabeni@redhat.com |
---|---|
State | Changes Requested |
Delegated to: | David Miller |
Headers | show |
Series | [net] ipv4/route: fail early when inet dev is missing | expand |
On 3/5/19 5:49 AM, Paolo Abeni wrote: > If a non local multicast packet reaches ip_route_input_rcu() while > the ingress device is NULL, we end up doing a NULL pointer A clarification: the ingress device is not NULL; it's the IPv4 private data (in_device). > dereference in IN_DEV_MFORWARD(). > > Since the later call to ip_route_input_mc() is going to fail if > !in_dev, we can fail early in such scenario and avoid the dangerous > code path. > > Reported-by: Tianhao Zhao <tizhao@redhat.com> > Fixes: e58e41596811 ("net: Enable support for VRF with ipv4 multicast") > Signed-off-by: Paolo Abeni <pabeni@redhat.com> > --- > net/ipv4/route.c | 9 +++++---- > 1 file changed, 5 insertions(+), 4 deletions(-) > > diff --git a/net/ipv4/route.c b/net/ipv4/route.c > index 7bb9128c8363..e40e56e014a0 100644 > --- a/net/ipv4/route.c > +++ b/net/ipv4/route.c > @@ -2144,12 +2144,13 @@ int ip_route_input_rcu(struct sk_buff *skb, __be32 daddr, __be32 saddr, > int our = 0; > int err = -EINVAL; > > - if (in_dev) > - our = ip_check_mc_rcu(in_dev, daddr, saddr, > - ip_hdr(skb)->protocol); > + if (!in_dev) > + return err; The point of e58e41596811 was to allow multicast address on the VRF device for Rx (I believe Allied Tellesis relies on that). I think the logic needs another tweak to properly handle that case and account for no ipv4 struct on the ingress device. I will have to come back to it when I have time to setup a test case. For now, tracing ip_check_mc_rcu to ip_mc_validate_source, you are correct. We can bail here if in_dev is NULL on the real ingress device. Reviewed-by: David Ahern <dsahern@gmail.com> > + our = ip_check_mc_rcu(in_dev, daddr, saddr, > + ip_hdr(skb)->protocol); > > /* check l3 master if no match yet */ > - if ((!in_dev || !our) && netif_is_l3_slave(dev)) { > + if (!our && netif_is_l3_slave(dev)) { > struct in_device *l3_in_dev; > > l3_in_dev = __in_dev_get_rcu(skb->dev); >
From: Paolo Abeni <pabeni@redhat.com> Date: Tue, 5 Mar 2019 13:49:39 +0100 > If a non local multicast packet reaches ip_route_input_rcu() while > the ingress device is NULL, we end up doing a NULL pointer > dereference in IN_DEV_MFORWARD(). > > Since the later call to ip_route_input_mc() is going to fail if > !in_dev, we can fail early in such scenario and avoid the dangerous > code path. > > Reported-by: Tianhao Zhao <tizhao@redhat.com> > Fixes: e58e41596811 ("net: Enable support for VRF with ipv4 multicast") > Signed-off-by: Paolo Abeni <pabeni@redhat.com> Please adjust your commit log message based upon David Ahern's feedback, thank you.
diff --git a/net/ipv4/route.c b/net/ipv4/route.c index 7bb9128c8363..e40e56e014a0 100644 --- a/net/ipv4/route.c +++ b/net/ipv4/route.c @@ -2144,12 +2144,13 @@ int ip_route_input_rcu(struct sk_buff *skb, __be32 daddr, __be32 saddr, int our = 0; int err = -EINVAL; - if (in_dev) - our = ip_check_mc_rcu(in_dev, daddr, saddr, - ip_hdr(skb)->protocol); + if (!in_dev) + return err; + our = ip_check_mc_rcu(in_dev, daddr, saddr, + ip_hdr(skb)->protocol); /* check l3 master if no match yet */ - if ((!in_dev || !our) && netif_is_l3_slave(dev)) { + if (!our && netif_is_l3_slave(dev)) { struct in_device *l3_in_dev; l3_in_dev = __in_dev_get_rcu(skb->dev);
If a non local multicast packet reaches ip_route_input_rcu() while the ingress device is NULL, we end up doing a NULL pointer dereference in IN_DEV_MFORWARD(). Since the later call to ip_route_input_mc() is going to fail if !in_dev, we can fail early in such scenario and avoid the dangerous code path. Reported-by: Tianhao Zhao <tizhao@redhat.com> Fixes: e58e41596811 ("net: Enable support for VRF with ipv4 multicast") Signed-off-by: Paolo Abeni <pabeni@redhat.com> --- net/ipv4/route.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)