2023年3月30日 11:09，Ondrej Zajicek <santiago@crfreenet.org> 写道：
On Thu, Mar 30, 2023 at 06:02:38AM +0800, Miao Wang wrote:
Hi,
The bug can be reproduced with the following steps:
1. create an unnumbered interface:
# ip link add dummy0 type dummy # ip link set dummy0 up # ip neigh replace 169.254.0.1 lladdr <some:mac:add:ress> dev dummy0
2. Create a static route via this interface and request a bfd session
protocol device { } ipv4 table main4; protocol bfd bfd1 { strict bind; accept direct; } protocol static { ipv4{ table main4; export all; }; route 1.2.3.4/32 via 169.254.0.1%dummy0 onlink bfd; }
The crash happens when dereferencing nb->ifa in static protocol module. nb->ifa is null pointer in this case.
Hi
Thanks for the bugreport. The code in static_update_bfd() has a clear bug, but i am not sure if just putting IPA_NONE to bfd_request_session() is a reasonable fix, i will have to check BFD code to see how much it assumes that the 'local' arg of bfd_request_session() is a valid address.
This patch also contains a fix in sk_setup(). When the source address and the destination address of a socket are both zero, the socket will be judged as an IPv6 socket since zero address is not a v4-mapped address. As a result, IPV6_V6ONLY is set on this socket. This patch prevents IPV6_V6ONLY from being set when both source address and destination address are zero.
I do not think this is a proper fix. sk_is_ipv6() uses sk->af, which should be here already assigned to proper value. If it is not, then the bug would be in the initial part of sk_open(), where AF value is decided based on subtype/saddr/daddr.
I think it is mostly missing ASSERT() and issue in a caller of sk_open() for such socket. For sockets that have saddr and daddr zero, caller should set subtype (AF) manually, so combination of all zeroes in subtype/saddr/daddr is invalid (as for such socket there is no way how to decide whether it is IPv4 or IPv6 socket).
Hi, according to your suggestion, I revised my patch. This time, bfd_get_iface is changed to accept an address family number, followed by bfd_open_rx_sk_bound and bfd_open_tx_sk, where sk->subtype is set explicitly, like that in bfd_open_rx_sk.
-- Elen sila lumenn' omentielvo
Ondrej 'Santiago' Zajicek (email: santiago@crfreenet.org) OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net) "To err is human -- to blame it on a computer is even more so."
From e496792443a995ce3ee5072dcb3024a9c60fbbba Mon Sep 17 00:00:00 2001 From: Miao Wang <shankerwangmiao@gmail.com> Date: Thu, 30 Mar 2023 15:09:41 +0800 Subject: [PATCH] Fixed crash when requesting a BFD session on an unnumbered interface
--- proto/bfd/bfd.c | 12 +++++++----- proto/bfd/bfd.h | 4 ++-- proto/bfd/packets.c | 6 ++++-- proto/static/static.c | 3 ++- 4 files changed, 15 insertions(+), 10 deletions(-) diff --git a/proto/bfd/bfd.c b/proto/bfd/bfd.c index 873e2ed5..a96c1c3f 100644 --- a/proto/bfd/bfd.c +++ b/proto/bfd/bfd.c @@ -119,7 +119,7 @@ static list STATIC_LIST_INIT(bfd_wait_list); const char *bfd_state_names[] = { "AdminDown", "Down", "Init", "Up" }; static void bfd_session_set_min_tx(struct bfd_session *s, u32 val); -static struct bfd_iface *bfd_get_iface(struct bfd_proto *p, ip_addr local, struct iface *iface); +static struct bfd_iface *bfd_get_iface(struct bfd_proto *p, ip_addr local, struct iface *iface, int af); static void bfd_free_iface(struct bfd_iface *ifa); static inline void bfd_notify_kick(struct bfd_proto *p); @@ -421,7 +421,9 @@ bfd_add_session(struct bfd_proto *p, ip_addr addr, ip_addr local, struct iface * { birdloop_enter(p->loop); - struct bfd_iface *ifa = bfd_get_iface(p, local, iface); + int af = ipa_is_ip4(addr) ? SK_IPV4 : SK_IPV6; + + struct bfd_iface *ifa = bfd_get_iface(p, local, iface, af); struct bfd_session *s = sl_allocz(p->session_slab); s->addr = addr; @@ -562,7 +564,7 @@ bfd_find_iface_config(struct bfd_config *cf, struct iface *iface) } static struct bfd_iface * -bfd_get_iface(struct bfd_proto *p, ip_addr local, struct iface *iface) +bfd_get_iface(struct bfd_proto *p, ip_addr local, struct iface *iface, int af) { struct bfd_iface *ifa; @@ -579,11 +581,11 @@ bfd_get_iface(struct bfd_proto *p, ip_addr local, struct iface *iface) ifa->cf = ic; ifa->bfd = p; - ifa->sk = bfd_open_tx_sk(p, local, iface); + ifa->sk = bfd_open_tx_sk(p, local, iface, af); ifa->uc = 1; if (cf->strict_bind) - ifa->rx = bfd_open_rx_sk_bound(p, local, iface); + ifa->rx = bfd_open_rx_sk_bound(p, local, iface, af); add_tail(&p->iface_list, &ifa->n); diff --git a/proto/bfd/bfd.h b/proto/bfd/bfd.h index 7caf9f66..011f91dc 100644 --- a/proto/bfd/bfd.h +++ b/proto/bfd/bfd.h @@ -223,8 +223,8 @@ void bfd_show_sessions(struct proto *P); /* packets.c */ void bfd_send_ctl(struct bfd_proto *p, struct bfd_session *s, int final); sock * bfd_open_rx_sk(struct bfd_proto *p, int multihop, int inet_version); -sock * bfd_open_rx_sk_bound(struct bfd_proto *p, ip_addr local, struct iface *ifa); -sock * bfd_open_tx_sk(struct bfd_proto *p, ip_addr local, struct iface *ifa); +sock * bfd_open_rx_sk_bound(struct bfd_proto *p, ip_addr local, struct iface *ifa, int af); +sock * bfd_open_tx_sk(struct bfd_proto *p, ip_addr local, struct iface *ifa, int af); #endif /* _BIRD_BFD_H_ */ diff --git a/proto/bfd/packets.c b/proto/bfd/packets.c index cb5f0d89..37a115a5 100644 --- a/proto/bfd/packets.c +++ b/proto/bfd/packets.c @@ -445,10 +445,11 @@ bfd_open_rx_sk(struct bfd_proto *p, int multihop, int af) } sock * -bfd_open_rx_sk_bound(struct bfd_proto *p, ip_addr local, struct iface *ifa) +bfd_open_rx_sk_bound(struct bfd_proto *p, ip_addr local, struct iface *ifa, int af) { sock *sk = sk_new(p->tpool); sk->type = SK_UDP; + sk->subtype = af; sk->saddr = local; sk->sport = ifa ? BFD_CONTROL_PORT : BFD_MULTI_CTL_PORT; sk->iface = ifa; @@ -477,10 +478,11 @@ bfd_open_rx_sk_bound(struct bfd_proto *p, ip_addr local, struct iface *ifa) } sock * -bfd_open_tx_sk(struct bfd_proto *p, ip_addr local, struct iface *ifa) +bfd_open_tx_sk(struct bfd_proto *p, ip_addr local, struct iface *ifa, int af) { sock *sk = sk_new(p->tpool); sk->type = SK_UDP; + sk->subtype = af; sk->saddr = local; sk->dport = ifa ? BFD_CONTROL_PORT : BFD_MULTI_CTL_PORT; sk->iface = ifa; diff --git a/proto/static/static.c b/proto/static/static.c index bb93305e..f2bb98c7 100644 --- a/proto/static/static.c +++ b/proto/static/static.c @@ -206,7 +206,8 @@ static_update_bfd(struct static_proto *p, struct static_route *r) if (bfd_up && !r->bfd_req) { // ip_addr local = ipa_nonzero(r->local) ? r->local : nb->ifa->ip; - r->bfd_req = bfd_request_session(p->p.pool, r->via, nb->ifa->ip, + r->bfd_req = bfd_request_session(p->p.pool, r->via, + nb->ifa ? nb->ifa->ip : IPA_NONE, nb->iface, p->p.vrf, static_bfd_notify, r, NULL); } -- 2.39.0