[PATCH] Fixed crash when requesting a BFD session on an unnumbered interface
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. 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. With below patch, I can confirm that bird won't crash and BFD packets can be successfully sent. Cheers, Miao Wang --- proto/static/static.c | 3 ++- sysdep/unix/io.c | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) 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); } diff --git a/sysdep/unix/io.c b/sysdep/unix/io.c index e131ca41..8075c2bd 100644 --- a/sysdep/unix/io.c +++ b/sysdep/unix/io.c @@ -996,7 +996,7 @@ sk_setup(sock *s) if (sk_is_ipv6(s)) { - if ((s->type == SK_TCP_PASSIVE) || (s->type == SK_TCP_ACTIVE) || (s->type == SK_UDP)) + if (((s->type == SK_TCP_PASSIVE) || (s->type == SK_TCP_ACTIVE) || (s->type == SK_UDP)) && (ipa_nonzero(s->saddr) || ipa_nonzero(s->daddr))) if (setsockopt(fd, SOL_IPV6, IPV6_V6ONLY, &y, sizeof(y)) < 0) ERR("IPV6_V6ONLY"); -- 2.39.0
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). -- 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."
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
On Thu, Mar 30, 2023 at 03:17:46PM +0800, Miao Wang wrote:
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.
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.
Hi That makes sense, will merge that change. But i am more sceptical about the whole idea of putting IPA_NONE as local address for BFD session. If the local IP is not well-defined (because it is some other address on some other interface chosen by kernel), how it can be configured on the other side? Also, it is incompatible with 'strict bind' option, but there is no check for it. For multihop BFD sessions, when local addresses cannot be determined from ifaces, we require them to be configured explicitly. What is your use case for this? -- 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."
2023年3月30日 23:23，Ondrej Zajicek <santiago@crfreenet.org> 写道：
On Thu, Mar 30, 2023 at 03:17:46PM +0800, Miao Wang wrote:
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.
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.
Hi
That makes sense, will merge that change.
But i am more sceptical about the whole idea of putting IPA_NONE as local address for BFD session. If the local IP is not well-defined (because it is some other address on some other interface chosen by kernel), how it can be configured on the other side? Also, it is incompatible with 'strict bind' option, but there is no check for it. For multihop BFD sessions, when local addresses cannot be determined from ifaces, we require them to be configured explicitly. What is your use case for this?
Hi, You are right about that. I currently have no idea about the use case. I raised the same question setting up a test environment for this. I just came across this bug when trying to set up a bfd controlled static route, in which case, I would like to set up a multi-hop session with a remote station to monitor the whole link path. After investigating into the code, I realized that currently bird cannot support this function, because the peer address of a bfd session requested by a static route is fixed to its next hop. I implemented a “bfd track” feature, letting a static route track an arbitrary bfd session requested by the manually configured bfd neighbors in the bfd protocol section. The patch of the bfd track feature was sent in my following e-mail. Cheers, Miao Wang
-- 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."
participants (2)
-
Miao Wang -
Ondrej Zajicek