netdev - Re: [PATCH net-next v8 01/24] netlink: add NLA_POLICY_MAX

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: <CAD4GDZxR5LzEo0ksFd3FbhVpeoeEMSWg2dL_RNyFRfB2bx052g@mail.gmail.com>
Date: Fri, 4 Oct 2024 15:41:16 +0100
From: Donald Hunter <donald.hunter@...il.com>
To: Jakub Kicinski <kuba@...nel.org>
Cc: Antonio Quartulli <antonio@...nvpn.net>, Eric Dumazet <edumazet@...gle.com>, 
	Paolo Abeni <pabeni@...hat.com>, Shuah Khan <shuah@...nel.org>, netdev@...r.kernel.org, 
	linux-kernel@...r.kernel.org, linux-kselftest@...r.kernel.org, 
	sd@...asysnail.net, ryazanov.s.a@...il.com
Subject: Re: [PATCH net-next v8 01/24] netlink: add NLA_POLICY_MAX_LEN macro

On Fri, 4 Oct 2024 at 14:38, Jakub Kicinski <kuba@...nel.org> wrote:
>
> On Fri, 04 Oct 2024 13:58:04 +0100 Donald Hunter wrote:
> > > @@ -466,6 +466,8 @@ class TypeBinary(Type):
> > >      def _attr_policy(self, policy):
> > >          if 'exact-len' in self.checks:
> > >              mem = 'NLA_POLICY_EXACT_LEN(' + str(self.get_limit('exact-len')) + ')'
> > > +        elif 'max-len' in self.checks:
> > > +            mem = 'NLA_POLICY_MAX_LEN(' + str(self.get_limit('max-len')) + ')'
> >
> > This takes precedence over min-length. What if both are set? The logic
> > should probably check and use NLA_POLICY_RANGE
>
> Or we could check if len(self.checks) <= 1 early and throw our hands up
> if there is more, for now?
>
> > >          else:
> > >              mem = '{ '
> > >              if len(self.checks) == 1 and 'min-len' in self.checks:
> >
> > Perhaps this should use NLA_POLICY_MIN_LEN ? In fact the current code
> > looks broken to me because the NLA_BINARY len check in validate_nla() is
> > a max length check, right?
> >
> > https://elixir.bootlin.com/linux/v6.11.1/source/lib/nlattr.c#L499
> >
> > The alternative is you emit an explicit initializer that includes the
> > correct NLA_VALIDATE_* type and sets type, min and/or max.
>
> Yeah, this code leads to endless confusion. We use NLA_UNSPEC (0)
> if min-len is set (IOW we don't set .type to NLA_BINARY). NLA_UNSPEC
> has different semantics for len.

Oh, I see it now. So it's dropping through to here:

https://elixir.bootlin.com/linux/v6.11.1/source/lib/nlattr.c#L555

> Agreed that we should probably clean this up, but no bug AFAICT.

Yeah, it's definitely surprising that the meaning of .len varies.