| Message ID | 20230525144609.503744-1-pctammela@mojatatu.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] net/netlink: fix NETLINK_LIST_MEMBERSHIPS group array length check | expand |
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>
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?
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.
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).
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
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);
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(-)