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

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

[Pedro Tammela]

For the socket option 'NETLINK_LIST_MEMBERSHIPS' the length is defined
as the number of u32 required to represent the whole bitset.
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).

[1] https://github.com/systemd/systemd/blob/9c9b9b89151c3e29f3665e306733957ee3979853/src/libsystemd/sd-netlink/netlink-socket.c#L26

Fixes: b42be38b2778 ("netlink: add API to retrieve all group memberships")
Signed-off-by: Pedro Tammela <pctammela@mojatatu.com>
---
 net/netlink/af_netlink.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index c87804112d0c..de21ddd5bf9a 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -1765,10 +1765,11 @@ static int netlink_getsockopt(struct socket *sock, int level, int optname,
 		break;
 	case NETLINK_LIST_MEMBERSHIPS: {
 		int pos, idx, shift, err = 0;
+		int blen = len * sizeof(u32);
 
 		netlink_lock_table();
 		for (pos = 0; pos * 8 < nlk->ngroups; pos += sizeof(u32)) {
-			if (len - pos < sizeof(u32))
+			if (blen - pos < sizeof(u32))
 				break;
 
 			idx = pos / sizeof(unsigned long);
-- 
2.39.2

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

[Simon Horman]

On Thu, May 25, 2023 at 11:46:09AM -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.
> 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).
> 
> [1] https://github.com/systemd/systemd/blob/9c9b9b89151c3e29f3665e306733957ee3979853/src/libsystemd/sd-netlink/netlink-socket.c#L26
> 
> Fixes: b42be38b2778 ("netlink: add API to retrieve all group memberships")
> Signed-off-by: Pedro Tammela <pctammela@mojatatu.com>

Reviewed-by: Simon Horman <simon.horman@corigine.com>

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

[Jakub Kicinski]

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.

> 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?
-- 
pw-bot: cr

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

[Pedro Tammela]

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.

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

[Jakub Kicinski]

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

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

[Pedro Tammela]

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