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.
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-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/dev-commits-src-all
To unsubscribe, send any mail to "dev-commits-src-all-unsubscr...@freebsd.org"
