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: <41cd3168-c63e-4b6b-9085-07dfe95e48da@redhat.com>
Date: Thu, 1 Aug 2024 11:43:54 +0200
From: Paolo Abeni <pabeni@...hat.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>,
 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 7/30/24 02:27, Maciej Żenczykowski wrote:
> 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.

Makes sense to me.

It would be great if you could follow-up with a self-test covering this.

Thanks,

Paolo