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

[Pedro Tammela <pctammela@mojatatu.com>]

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