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

Message-ID: <1be298c3-ce57-548e-e0af-937971fe58e9@mojatatu.com>
Date: Sat, 27 May 2023 12:01:25 -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 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.