| 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
| ||
|
Message-ID: <CANP3RGfYiAyXTp4yPX42eOSsob0Hzt50+6X6UwRpwYajPvdUqw@mail.gmail.com>
Date: Thu, 27 Jul 2023 14:51:55 +0200
From: Maciej Żenczykowski <maze@...gle.com>
To: Patrick Rohr <prohr@...gle.com>
Cc: "David S . Miller" <davem@...emloft.net>,
Linux Network Development Mailing List <netdev@...r.kernel.org>, Lorenzo Colitti <lorenzo@...gle.com>,
David Ahern <dsahern@...nel.org>
Subject: Re: [net-next v2] net: change accept_ra_min_rtr_lft to affect all RA lifetimes
On Thu, Jul 27, 2023 at 1:07 AM Patrick Rohr <prohr@...gle.com> wrote:
>
> accept_ra_min_rtr_lft only considered the lifetime of the default route
> and discarded entire RAs accordingly.
>
> This change renames accept_ra_min_rtr_lft to accept_ra_min_lft, and
> applies the value to individual RA sections; in particular, router
> lifetime, PIO preferred lifetime, and RIO lifetime. If any of those
> lifetimes are lower than the configured value, the specific RA section
> is ignored.
>
> 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 the
> misconfigured 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.
>
> Fixes: 1671bcfd76fd ("net: add sysctl accept_ra_min_rtr_lft")
> Cc: Maciej Żenczykowski <maze@...gle.com>
> Cc: Lorenzo Colitti <lorenzo@...gle.com>
> Cc: David Ahern <dsahern@...nel.org>
> Signed-off-by: Patrick Rohr <prohr@...gle.com>
> ---
> Documentation/networking/ip-sysctl.rst | 8 ++++----
> include/linux/ipv6.h | 2 +-
> include/uapi/linux/ipv6.h | 2 +-
> net/ipv6/addrconf.c | 14 ++++++++-----
> net/ipv6/ndisc.c | 27 +++++++++++---------------
> 5 files changed, 26 insertions(+), 27 deletions(-)
>
> diff --git a/Documentation/networking/ip-sysctl.rst b/Documentation/networking/ip-sysctl.rst
> index 37603ad6126b..a66054d0763a 100644
> --- a/Documentation/networking/ip-sysctl.rst
> +++ b/Documentation/networking/ip-sysctl.rst
> @@ -2288,11 +2288,11 @@ accept_ra_min_hop_limit - INTEGER
>
> Default: 1
>
> -accept_ra_min_rtr_lft - INTEGER
> - Minimum acceptable router lifetime in Router Advertisement.
> +accept_ra_min_lft - INTEGER
> + Minimum acceptable lifetime value in Router Advertisement.
>
> - RAs with a router lifetime less than this value shall be
> - ignored. RAs with a router lifetime of 0 are unaffected.
> + RA sections with a lifetime less than this value shall be
> + ignored. Zero lifetimes stay unaffected.
>
> Default: 0
>
> diff --git a/include/linux/ipv6.h b/include/linux/ipv6.h
> index 0295b47c10a3..5883551b1ee8 100644
> --- a/include/linux/ipv6.h
> +++ b/include/linux/ipv6.h
> @@ -33,7 +33,7 @@ struct ipv6_devconf {
> __s32 accept_ra_defrtr;
> __u32 ra_defrtr_metric;
> __s32 accept_ra_min_hop_limit;
> - __s32 accept_ra_min_rtr_lft;
> + __s32 accept_ra_min_lft;
> __s32 accept_ra_pinfo;
> __s32 ignore_routes_with_linkdown;
> #ifdef CONFIG_IPV6_ROUTER_PREF
> diff --git a/include/uapi/linux/ipv6.h b/include/uapi/linux/ipv6.h
> index 8b6bcbf6ed4a..cf592d7b630f 100644
> --- a/include/uapi/linux/ipv6.h
> +++ b/include/uapi/linux/ipv6.h
> @@ -198,7 +198,7 @@ enum {
> DEVCONF_IOAM6_ID_WIDE,
> DEVCONF_NDISC_EVICT_NOCARRIER,
> DEVCONF_ACCEPT_UNTRACKED_NA,
> - DEVCONF_ACCEPT_RA_MIN_RTR_LFT,
> + DEVCONF_ACCEPT_RA_MIN_LFT,
> DEVCONF_MAX
> };
>
> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
> index 19eb4b3d26ea..7f7d2b677711 100644
> --- a/net/ipv6/addrconf.c
> +++ b/net/ipv6/addrconf.c
> @@ -202,7 +202,7 @@ static struct ipv6_devconf ipv6_devconf __read_mostly = {
> .ra_defrtr_metric = IP6_RT_PRIO_USER,
> .accept_ra_from_local = 0,
> .accept_ra_min_hop_limit= 1,
> - .accept_ra_min_rtr_lft = 0,
> + .accept_ra_min_lft = 0,
> .accept_ra_pinfo = 1,
> #ifdef CONFIG_IPV6_ROUTER_PREF
> .accept_ra_rtr_pref = 1,
> @@ -263,7 +263,7 @@ static struct ipv6_devconf ipv6_devconf_dflt __read_mostly = {
> .ra_defrtr_metric = IP6_RT_PRIO_USER,
> .accept_ra_from_local = 0,
> .accept_ra_min_hop_limit= 1,
> - .accept_ra_min_rtr_lft = 0,
> + .accept_ra_min_lft = 0,
> .accept_ra_pinfo = 1,
> #ifdef CONFIG_IPV6_ROUTER_PREF
> .accept_ra_rtr_pref = 1,
> @@ -2727,6 +2727,10 @@ 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) {
> + return;
> + }
> +
> /*
> * Two things going on here:
> * 1) Add routes for on-link prefixes
> @@ -5598,7 +5602,7 @@ static inline void ipv6_store_devconf(struct ipv6_devconf *cnf,
> array[DEVCONF_IOAM6_ID_WIDE] = cnf->ioam6_id_wide;
> array[DEVCONF_NDISC_EVICT_NOCARRIER] = cnf->ndisc_evict_nocarrier;
> array[DEVCONF_ACCEPT_UNTRACKED_NA] = cnf->accept_untracked_na;
> - array[DEVCONF_ACCEPT_RA_MIN_RTR_LFT] = cnf->accept_ra_min_rtr_lft;
> + array[DEVCONF_ACCEPT_RA_MIN_LFT] = cnf->accept_ra_min_lft;
> }
>
> static inline size_t inet6_ifla6_size(void)
> @@ -6793,8 +6797,8 @@ static const struct ctl_table addrconf_sysctl[] = {
> .proc_handler = proc_dointvec,
> },
> {
> - .procname = "accept_ra_min_rtr_lft",
> - .data = &ipv6_devconf.accept_ra_min_rtr_lft,
> + .procname = "accept_ra_min_lft",
> + .data = &ipv6_devconf.accept_ra_min_lft,
> .maxlen = sizeof(int),
> .mode = 0644,
> .proc_handler = proc_dointvec,
> diff --git a/net/ipv6/ndisc.c b/net/ipv6/ndisc.c
> index 29ddad1c1a2f..eeb60888187f 100644
> --- a/net/ipv6/ndisc.c
> +++ b/net/ipv6/ndisc.c
> @@ -1280,8 +1280,6 @@ static enum skb_drop_reason ndisc_router_discovery(struct sk_buff *skb)
> if (!ndisc_parse_options(skb->dev, opt, optlen, &ndopts))
> return SKB_DROP_REASON_IPV6_NDISC_BAD_OPTIONS;
>
> - lifetime = ntohs(ra_msg->icmph.icmp6_rt_lifetime);
> -
> if (!ipv6_accept_ra(in6_dev)) {
> ND_PRINTK(2, info,
> "RA: %s, did not accept ra for dev: %s\n",
> @@ -1289,13 +1287,6 @@ static enum skb_drop_reason ndisc_router_discovery(struct sk_buff *skb)
> goto skip_linkparms;
> }
>
> - if (lifetime != 0 && lifetime < in6_dev->cnf.accept_ra_min_rtr_lft) {
> - ND_PRINTK(2, info,
> - "RA: router lifetime (%ds) is too short: %s\n",
> - lifetime, skb->dev->name);
> - goto skip_linkparms;
> - }
> -
> #ifdef CONFIG_IPV6_NDISC_NODETYPE
> /* skip link-specific parameters from interior routers */
> if (skb->ndisc_nodetype == NDISC_NODETYPE_NODEFAULT) {
> @@ -1336,6 +1327,14 @@ static enum skb_drop_reason ndisc_router_discovery(struct sk_buff *skb)
> goto skip_defrtr;
> }
>
> + lifetime = ntohs(ra_msg->icmph.icmp6_rt_lifetime);
> + if (lifetime != 0 && lifetime < in6_dev->cnf.accept_ra_min_lft) {
> + ND_PRINTK(2, info,
> + "RA: router lifetime (%ds) is too short: %s\n",
> + lifetime, skb->dev->name);
> + goto skip_defrtr;
> + }
> +
> /* Do not accept RA with source-addr found on local machine unless
> * accept_ra_from_local is set to true.
> */
> @@ -1499,13 +1498,6 @@ static enum skb_drop_reason ndisc_router_discovery(struct sk_buff *skb)
> goto out;
> }
>
> - if (lifetime != 0 && lifetime < in6_dev->cnf.accept_ra_min_rtr_lft) {
> - ND_PRINTK(2, info,
> - "RA: router lifetime (%ds) is too short: %s\n",
> - lifetime, skb->dev->name);
> - goto out;
> - }
> -
> #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)
> --
> 2.41.0.487.g6d72f3e995-goog
Reviewed-by: Maciej Żenczykowski <maze@...gle.com>
Patrick and I have spoken about this at length, and this (ignoring low
lifetime portions of the RA) seems like the best approach...
(though I will admit that I'm not super knowledgeable about IPv6 RAs
and this particular code)
Powered by blists - more mailing lists