> 2023年3月30日 11:09，Ondrej Zajicek <[email protected]> 写道： > > 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: [email protected]) > 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 <[email protected]> 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
