netdev - Re: [PATCH net] net/netlink: fix NETLINK_LIST_MEMBERSHIPS group array length check

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] [day] [month] [year] [list]

Date: Mon, 29 May 2023 11:37:12 -0300
From: Pedro Tammela <pctammela@...atatu.com>
To: Jakub Kicinski <kuba@...nel.org>
Cc: netdev@...r.kernel.org, davem@...emloft.net, edumazet@...gle.com,
 pabeni@...hat.com, kuniyu@...zon.com, dh.herrmann@...il.com, jhs@...atatu.com
Subject: Re: [PATCH net] net/netlink: fix NETLINK_LIST_MEMBERSHIPS group array
 length check

On 29/05/2023 03:40, Jakub Kicinski wrote:
> On Sat, 27 May 2023 12:01:25 -0300 Pedro Tammela wrote:
>> On 27/05/2023 00:33, Jakub Kicinski wrote:
>>> On Thu, 25 May 2023 11:46:09 -0300 Pedro Tammela wrote:
>>>> For the socket option 'NETLINK_LIST_MEMBERSHIPS' the length is defined
>>>> as the number of u32 required to represent the whole bitset.
>>>
>>> I don't think it is, it's a getsockopt() len is in bytes.
>>
>> Unfortunately the man page seems to be ambiguous (Emphasis added):
>> 	
>>          NETLINK_LIST_MEMBERSHIPS (since Linux 4.2)
>>                 Retrieve all groups a socket is a member of.  optval is a
>>                 pointer to __u32 and *optlen is the size of the array*.  The
>>                 array is filled with the full membership set of the
>>                 socket, and the required array size is returned in optlen.
>>
>> Size of the array in bytes? in __u32?
> 
> Indeed ambiguous, in C "size of array" could as well refer to sizeof()
> or ARRAY_SIZE()..
> 
>> SystemD seems to be expecting the size in __u32 chunks:
>> https://github.com/systemd/systemd/blob/9c9b9b89151c3e29f3665e306733957ee3979853/src/libsystemd/sd-netlink/netlink-socket.c#L37
>>
>> But then looking into the getsockopt manpage we see (Ubuntu 23.04):
>>
>>          int getsockopt(int sockfd, int level, int optname,
>>                         void optval[restrict *.optlen],
>>                         socklen_t *restrict optlen);
>>
>>
>> So it seems like getsockopt() asks for optlen to be, in this case, __u32
>> chunks?
> 
> Why so?

It's a far fetched interpretation of the function signature in the man 
page but
someone could argue that it's trying to emulate a VLA style function 
prototype over a generic optval.
But let's not waste precious time in this discussion.

> 
>> [...]
> 
> I don't know of any other case where socklen_t would refer to something
> else than bytes, I'm leaning towards addressing the truncation (and if
> systemd thinks the value is in u32s potentially also fixing system, not
> that over-allocating will hurt its correctness).

OK! Will re-spin to net-next so people have plenty of time to adjust