netdev - Re: [PATCH net v2] ipv6: fix ndisc_is

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: <CANP3RGdKuZUxGe6o0yYpFoJi+KsVPbLUoEwpUFHTgrQHA6BzcQ@mail.gmail.com>
Date: Mon, 29 Jul 2024 17:27:17 -0700
From: Maciej Żenczykowski <zenczykowski@...il.com>
To: Maciej Żenczykowski <zenczykowski@...il.com>
Cc: Linux Network Development Mailing List <netdev@...r.kernel.org>, "David S . Miller" <davem@...emloft.net>, 
	Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, 
	Jen Linkova <furry@...gle.com>, Lorenzo Colitti <lorenzo@...gle.com>, Patrick Rohr <prohr@...gle.com>, 
	David Ahern <dsahern@...nel.org>, 
	YOSHIFUJI Hideaki / 吉藤英明 <yoshfuji@...ux-ipv6.org>
Subject: Re: [PATCH net v2] ipv6: fix ndisc_is_useropt() handling for PIO

On Mon, Jul 29, 2024 at 5:17 PM Maciej Żenczykowski <maze@...gle.com> wrote:
>
> The current logic only works if the PIO is between two
> other ND user options.  This fixes it so that the PIO
> can also be either before or after other ND user options
> (for example the first or last option in the RA).
>
> side note: there's actually Android tests verifying
> a portion of the old broken behaviour, so:
>   https://android-review.googlesource.com/c/kernel/tests/+/3196704
> fixes those up.
>
> Cc: Jen Linkova <furry@...gle.com>
> Cc: Lorenzo Colitti <lorenzo@...gle.com>
> Cc: Patrick Rohr <prohr@...gle.com>
> Cc: David Ahern <dsahern@...nel.org>
> Cc: YOSHIFUJI Hideaki / 吉藤英明 <yoshfuji@...ux-ipv6.org>
> Cc: Jakub Kicinski <kuba@...nel.org>
> Signed-off-by: Maciej Żenczykowski <maze@...gle.com>
> Fixes: 048c796beb6e ("ipv6: adjust ndisc_is_useropt() to also return true for PIO")
> ---
>  net/ipv6/ndisc.c | 34 ++++++++++++++++++----------------
>  1 file changed, 18 insertions(+), 16 deletions(-)
>
> diff --git a/net/ipv6/ndisc.c b/net/ipv6/ndisc.c
> index 70a0b2ad6bd7..b8eec1b6cc2c 100644
> --- a/net/ipv6/ndisc.c
> +++ b/net/ipv6/ndisc.c
> @@ -227,6 +227,7 @@ struct ndisc_options *ndisc_parse_options(const struct net_device *dev,
>                 return NULL;
>         memset(ndopts, 0, sizeof(*ndopts));
>         while (opt_len) {
> +               bool unknown = false;
>                 int l;
>                 if (opt_len < sizeof(struct nd_opt_hdr))
>                         return NULL;
> @@ -262,22 +263,23 @@ struct ndisc_options *ndisc_parse_options(const struct net_device *dev,
>                         break;
>  #endif
>                 default:
> -                       if (ndisc_is_useropt(dev, nd_opt)) {
> -                               ndopts->nd_useropts_end = nd_opt;
> -                               if (!ndopts->nd_useropts)
> -                                       ndopts->nd_useropts = nd_opt;
> -                       } else {
> -                               /*
> -                                * Unknown options must be silently ignored,
> -                                * to accommodate future extension to the
> -                                * protocol.
> -                                */
> -                               ND_PRINTK(2, notice,
> -                                         "%s: ignored unsupported option; type=%d, len=%d\n",
> -                                         __func__,
> -                                         nd_opt->nd_opt_type,
> -                                         nd_opt->nd_opt_len);
> -                       }
> +                       unknown = true;
> +               }
> +               if (ndisc_is_useropt(dev, nd_opt)) {
> +                       ndopts->nd_useropts_end = nd_opt;
> +                       if (!ndopts->nd_useropts)
> +                               ndopts->nd_useropts = nd_opt;
> +               } else if (unknown) {
> +                       /*
> +                        * Unknown options must be silently ignored,
> +                        * to accommodate future extension to the
> +                        * protocol.
> +                        */
> +                       ND_PRINTK(2, notice,
> +                                 "%s: ignored unsupported option; type=%d, len=%d\n",
> +                                 __func__,
> +                                 nd_opt->nd_opt_type,
> +                                 nd_opt->nd_opt_len);
>                 }
>  next_opt:
>                 opt_len -= l;
> --
> 2.46.0.rc1.232.g9752f9e123-goog
>

The diff on this second version is significantly bigger (although it's
just unindenting a block), but perhaps this is better as it is much
harder to screw things up.