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

[Jakub Kicinski <kuba@kernel.org>]

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?

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

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).