[PATCH] Fixed crash when requesting a BFD session on an unnumbered interface
Miao Wang
shankerwangmiao at gmail.com
Thu Mar 30 09:17:46 CEST 2023
> 2023年3月30日 11:09，Ondrej Zajicek <santiago at 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 at 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 at 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
More information about the Bird-users
mailing list