19.02.2021, 16:11, "Kristof Provost" <k...@freebsd.org>:
> On 19 Feb 2021, at 16:24, Kristof Provost wrote:
>> On 16 Feb 2021, at 21:31, Alexander V. Chernikov wrote:
>>> The branch main has been updated by melifaro:
>>>
>>> URL:
>>> https://cgit.FreeBSD.org/src/commit/?id=2fe5a79425c79f7b828acd91da66d97230925fc8
>>>
>>> commit 2fe5a79425c79f7b828acd91da66d97230925fc8
>>> Author: Alexander V. Chernikov <melif...@freebsd.org>
>>> AuthorDate: 2021-02-16 20:30:04 +0000
>>> Commit: Alexander V. Chernikov <melif...@freebsd.org>
>>> CommitDate: 2021-02-16 20:30:04 +0000
>>>
>>> Fix dst/netmask handling in routing socket code.
>>>
>>> Traditionally routing socket code did almost zero checks on
>>> the input message except for the most basic size checks.
>>>
>>> This resulted in the unclear KPI boundary for the routing system code
>>> (`rtrequest*` and now `rib_action()`) w.r.t message validness.
>>>
>>> Multiple potential problems and nuances exists:
>>> * Host bits in RTAX_DST sockaddr. Existing applications do send prefixes
>>> with hostbits uncleared. Even `route(8)` does this, as they hope the kernel
>>> would do the job of fixing it. Code inside `rib_action()` needs to handle
>>> it on its own (see `rt_maskedcopy()` ugly hack).
>>> * There are multiple way of adding the host route: it can be DST without
>>> netmask or DST with /32(/128) netmask. Also, RTF_HOST has to be set
>>> correspondingly.
>>> Currently, these 2 options create 2 DIFFERENT routes in the kernel.
>>> * no sockaddr length/content checking for the "secondary" fields exists:
>>> nothing
>>> stops rtsock application to send sockaddr_in with length of 25 (instead of
>>> 16).
>>> Kernel will accept it, install to RIB as is and propagate to all rtsock
>>> consumers,
>>> potentially triggering bugs in their code. Same goes for sin_port,
>>> sin_zero, etc.
>>>
>>> The goal of this change is to make rtsock verify all sockaddr and prefix
>>> consistency.
>>> Said differently, `rib_action()` or internals should NOT require to change
>>> any of the
>>> sockaddrs supplied by `rt_addrinfo` structure due to incorrectness.
>>>
>>> To be more specific, this change implements the following:
>>> * sockaddr cleanup/validation check is added immediately after getting
>>> sockaddrs from rtm.
>>> * Per-family dst/netmask checks clears host bits in dst and zeros all
>>> dst/netmask "secondary" fields.
>>> * The same netmask checking code converts /32(/128) netmasks to "host"
>>> route case
>>> (NULL netmask, RTF_HOST), removing the dualism.
>>> * Instead of allowing ANY "known" sockaddr families (0<..<AF_MAX), allow
>>> only actually
>>> supported ones (inet, inet6, link).
>>> * Automatically convert `sockaddr_sdl` (AF_LINK) gateways to
>>> `sockaddr_sdl_short`.
>>>
>>> Reported by: Guy Yur <guyyur at gmail.com>
>>> Reviewed By: donner
>>> Differential Revision: https://reviews.freebsd.org/D28668
>>> MFC after: 3 days
>>> ---
>>> sys/net/rtsock.c | 201 +++++++++++++++++++++++++++++++++-
>>> tests/sys/net/routing/rtsock_common.h | 4 -
>>> 2 files changed, 195 insertions(+), 10 deletions(-)
>>> +static int
>>> +cleanup_xaddrs_inet(struct rt_addrinfo *info)
>>> +{
>>> + struct sockaddr_in *dst_sa, *mask_sa;
>>> +
>>> + /* Check & fixup dst/netmask combination first */
>>> + dst_sa = (struct sockaddr_in *)info->rti_info[RTAX_DST];
>>> + mask_sa = (struct sockaddr_in *)info->rti_info[RTAX_NETMASK];
>>> +
>>> + struct in_addr mask = {
>>> + .s_addr = mask_sa ? mask_sa->sin_addr.s_addr : INADDR_BROADCAST,
>>> + };
>>> + struct in_addr dst = {
>>> + .s_addr = htonl(ntohl(dst_sa->sin_addr.s_addr) & ntohl(mask.s_addr))
>>> + };
>>> +
>>
>> This breaks things like `arp -d 10.0.2.1`. It always masks off the network
>> address, which is the right thing to do in the routing table, but not in the
>> arp table.
Thanks for catching it!
I've raised https://reviews.freebsd.org/D28804 to fix it.
(Also: yes, this should be covered by tests).
>>
>> I’ve worked around it for now with this hack:
>>
>> diff --git a/sys/net/rtsock.c b/sys/net/rtsock.c
>> index 3c1fea497af6..533076db99a5 100644
>> --- a/sys/net/rtsock.c
>> +++ b/sys/net/rtsock.c
>> @@ -638,9 +638,12 @@ fill_addrinfo(struct rt_msghdr *rtm, int len, u_int
>> fibnum, struct rt_addrinfo *
>> return (EINVAL);
>>
>> info->rti_flags = rtm->rtm_flags;
>> - error = cleanup_xaddrs(info);
>> - if (error != 0)
>> - return (error);
>> + /* XXX HACK */
>> + if (! (rtm->rtm_flags & RTF_LLDATA)) {
>> + error = cleanup_xaddrs(info);
>> + if (error != 0)
>> + return (error);
>> + }
>> saf = info->rti_info[RTAX_DST]->sa_family;
>> /*
>> * Verify that the caller has the appropriate privilege; RTM_GET
>>
>> But I’m not totally happy with this, obviously.
>
> This may be a bit more reasonable:
>
> diff --git a/sys/net/rtsock.c b/sys/net/rtsock.c index
> 3c1fea497af6..5147b92e95d5 100644 --- a/sys/net/rtsock.c +++
> b/sys/net/rtsock.c @@ -1393,6 +1393,10 @@ cleanup_xaddrs_inet(struct
> rt_addrinfo *info) .s_addr =
> htonl(ntohl(dst_sa->sin_addr.s_addr) & ntohl(mask.s_addr)) }; +
> /* Keep the address if we're LL */ + if (info->rti_flags & RTF_LLDATA)
> + dst.s_addr = dst_sa->sin_addr.s_addr; + if
> (dst_sa->sin_len < sizeof(struct sockaddr_in)) { printf("dst
> sin_len too small\n"); return (EINVAL); @@ -1431,7 +1435,10 @@
> cleanup_xaddrs_inet6(struct rt_addrinfo *info) mask_sa = (struct
> sockaddr_in6 *)info->rti_info[RTAX_NETMASK]; mask = mask_sa ?
> mask_sa->sin6_addr : in6mask128; - IN6_MASK_ADDR(&dst_sa->sin6_addr,
> &mask); + + /* Keep the address if we're LL */ + if (!
> (info->rti_flags & RTF_LLDATA)) +
> IN6_MASK_ADDR(&dst_sa->sin6_addr, &mask); if (dst_sa->sin6_len <
> sizeof(struct sockaddr_in6)) { printf("dst sin6_len too
> small\n");
>
> Best regards,
> Kristof
_______________________________________________
dev-commits-src-main@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/dev-commits-src-main
To unsubscribe, send any mail to "dev-commits-src-main-unsubscr...@freebsd.org"
