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: <e09ea6b5-fe0c-4f90-8943-83aa410ccc1f@openvpn.net>
Date: Mon, 7 Oct 2024 12:04:22 +0200
From: Antonio Quartulli <antonio@...nvpn.net>
To: Jakub Kicinski <kuba@...nel.org>, Donald Hunter <donald.hunter@...il.com>
Cc: 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

Hi,

On 04/10/2024 15:38, Jakub Kicinski 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?

We already perform the same check in the 'else' branch below.
It'd be about moving it at the beginning of the function and bail out if 
true, right?

Should I modify this patch and move the check above?


Cheers,

> 
>>>           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.
> 
> Agreed that we should probably clean this up, but no bug AFAICT.

-- 
Antonio Quartulli
OpenVPN Inc.