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

[Pedro Tammela <pctammela@mojatatu.com>]

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?
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?

WDYT?

> 
>> User space then usually queries the required size and issues a subsequent
>> getsockopt call with the correct parameters[1].
>>
>> The current code has an unit mismatch between 'len' and 'pos', where
>> 'len' is the number of u32 in the passed array while 'pos' is the
>> number of bytes iterated in the groups bitset.
>> For netlink groups greater than 32, which from a quick glance
>> is a rare occasion, the mismatch causes the misreport of groups e.g.
>> if a rtnl socket is a member of group 34, it's reported as not a member
>> (all 0s).
> 
> IDK... I haven't tried to repro but looking at the code the more
> suspicious line of code is this one:
> 
> 		if (put_user(ALIGN(nlk->ngroups / 8, sizeof(u32)), optlen))
> 
> It's going to round down bytes, and I don't think it's intending to.
> It should be DIV_ROUND_UP(, 8) then ALIGN(, 4) right?

That indeed looks suspicious.
Your suggestions looks correct for optlen reported as bytes.
For optlen reported in __u32 chunks seems like BITS_TO_U32(nlk->ngroups) 
would be sufficient.