netdev - Re: [net-next] net: change accept_ra_min_rtr

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CANLD9C1aV3U+GZ3hUE-_AgbeSyCNgUvJPmOPcFEDDgD_fQWJ0A@mail.gmail.com>
Date: Wed, 26 Jul 2023 10:33:07 -0700
From: Patrick Rohr <prohr@...gle.com>
To: David Ahern <dsahern@...nel.org>
Cc: "David S . Miller" <davem@...emloft.net>, 
	Linux Network Development Mailing List <netdev@...r.kernel.org>, Maciej Żenczykowski <maze@...gle.com>, 
	Lorenzo Colitti <lorenzo@...gle.com>
Subject: Re: [net-next] net: change accept_ra_min_rtr_lft to affect all RA lifetimes

On Tue, Jul 25, 2023 at 4:28 PM David Ahern <dsahern@...nel.org> wrote:
>
> On 7/25/23 12:31 PM, Patrick Rohr wrote:
> > @@ -2727,6 +2727,11 @@ void addrconf_prefix_rcv(struct net_device *dev, u8 *opt, int len, bool sllao)
> >               return;
> >       }
> >
> > +     if (valid_lft != 0 && valid_lft < in6_dev->cnf.accept_ra_min_lft) {
> > +             net_info_ratelimited("addrconf: prefix option lifetime too short\n");
>
> The error message does not really provide any value besides spamming the
> logs. Similar comment applies to existing error logging in that function
> too. I think a counter for invalid prefix packets would be more useful.
>

Agreed. Let me remove the error log in this commit and clean up the
entire function in a follow up.

> The commit mentioned in the Fixes was just applied and you are already
> sending a follow up moving the same code around again.

I got feedback off of the mailing list after the patch was applied. In order for
the sysctl to be useful to Android, it should really apply to all lifetimes in
the RA, since that is what determines the minimum frequency at which RAs must be
processed by the kernel. Android uses hardware offloads to drop RAs
for a fraction of the
minimum of all lifetimes present in the RA (some networks have very
frequent RAs (5s) with high lifetimes (2h)). Despite this, we have
encountered
networks that set the router lifetime to 30s which results in very frequent CPU
wakeups. Instead of disabling IPv6 (and dropping IPv6 ethertype in the
WiFi firmware)
entirely on such networks, it seems better to ignore such routers
while still processing RAs from other IPv6 routers on the same network
(i.e. to support IoT applications).
The previous implementation dropped the entire RA
based on router lifetime. This turned out to be hard to expand to the other
lifetimes present in the RA in a consistent manner -- dropping the
entire RA based on
RIO/PIO lifetimes would essentially require parsing the whole thing twice. I am
sending this follow up patch now to fix 1671bcfd76fd before it is released.

>
> >  #ifdef CONFIG_IPV6_ROUTE_INFO
> >       if (!in6_dev->cnf.accept_ra_from_local &&
> >           ipv6_chk_addr(dev_net(in6_dev->dev), &ipv6_hdr(skb)->saddr,
> > @@ -1530,6 +1522,9 @@ static enum skb_drop_reason ndisc_router_discovery(struct sk_buff *skb)
> >                       if (ri->prefix_len == 0 &&
> >                           !in6_dev->cnf.accept_ra_defrtr)
> >                               continue;
> > +                     if (ri->lifetime != 0 &&
> > +                         ntohl(ri->lifetime) < in6_dev->cnf.accept_ra_min_lft)
> > +                             continue;
> >                       if (ri->prefix_len < in6_dev->cnf.accept_ra_rt_info_min_plen)
> >                               continue;
> >                       if (ri->prefix_len > in6_dev->cnf.accept_ra_rt_info_max_plen)
>