| From 71ca5b09bc71e8cbe38177cf41e83fe164e52eee Mon Sep 17 00:00:00 2001 |
| From: Mark Stapp <mstapp@nvidia.com> |
| Date: Thu, 8 Sep 2022 16:14:36 -0400 |
| Subject: [PATCH] bgpd: avoid notify race between io and main pthreads |
| |
| The "bgp_notify_" apis in bgp_packet.c generate a notification |
| to a peer, usually during error handling. The io pthread wants |
| to send notifications in a couple of cases during early |
| received-packet validation - but the existing api interacts |
| with the peer struct itself, and that's not safe. |
| |
| Add a new api for use by the io pthread, and adjust the main |
| notify api so that it can avoid touching the peer struct. |
| |
| Signed-off-by: Mark Stapp <mstapp@nvidia.com> |
| |
| CVE: CVE-2022-37035 |
| |
| Upstream-Status: Backport |
| [https://github.com/FRRouting/frr/commit/71ca5b09bc71e8cbe38177cf41e83fe164e52eee] |
| |
| Signed-off-by: Yi Zhao <yi.zhao@windriver.com> |
| --- |
| bgpd/bgp_io.c | 17 ++++++++--------- |
| bgpd/bgp_packet.c | 32 ++++++++++++++++++++++++++++---- |
| bgpd/bgp_packet.h | 2 ++ |
| 3 files changed, 38 insertions(+), 13 deletions(-) |
| |
| diff --git a/bgpd/bgp_io.c b/bgpd/bgp_io.c |
| index 7af1fae28..f9bb8d518 100644 |
| --- a/bgpd/bgp_io.c |
| +++ b/bgpd/bgp_io.c |
| @@ -37,7 +37,7 @@ |
| #include "bgpd/bgp_debug.h" // for bgp_debug_neighbor_events, bgp_type_str |
| #include "bgpd/bgp_errors.h" // for expanded error reference information |
| #include "bgpd/bgp_fsm.h" // for BGP_EVENT_ADD, bgp_event |
| -#include "bgpd/bgp_packet.h" // for bgp_notify_send_with_data, bgp_notify... |
| +#include "bgpd/bgp_packet.h" // for bgp_notify_io_invalid... |
| #include "bgpd/bgp_trace.h" // for frrtraces |
| #include "bgpd/bgpd.h" // for peer, BGP_MARKER_SIZE, bgp_master, bm |
| /* clang-format on */ |
| @@ -526,8 +526,8 @@ static bool validate_header(struct peer *peer) |
| return false; |
| |
| if (memcmp(m_correct, m_rx, BGP_MARKER_SIZE) != 0) { |
| - bgp_notify_send(peer, BGP_NOTIFY_HEADER_ERR, |
| - BGP_NOTIFY_HEADER_NOT_SYNC); |
| + bgp_notify_io_invalid(peer, BGP_NOTIFY_HEADER_ERR, |
| + BGP_NOTIFY_HEADER_NOT_SYNC, NULL, 0); |
| return false; |
| } |
| |
| @@ -547,9 +547,8 @@ static bool validate_header(struct peer *peer) |
| zlog_debug("%s unknown message type 0x%02x", peer->host, |
| type); |
| |
| - bgp_notify_send_with_data(peer, BGP_NOTIFY_HEADER_ERR, |
| - BGP_NOTIFY_HEADER_BAD_MESTYPE, &type, |
| - 1); |
| + bgp_notify_io_invalid(peer, BGP_NOTIFY_HEADER_ERR, |
| + BGP_NOTIFY_HEADER_BAD_MESTYPE, &type, 1); |
| return false; |
| } |
| |
| @@ -574,9 +573,9 @@ static bool validate_header(struct peer *peer) |
| |
| uint16_t nsize = htons(size); |
| |
| - bgp_notify_send_with_data(peer, BGP_NOTIFY_HEADER_ERR, |
| - BGP_NOTIFY_HEADER_BAD_MESLEN, |
| - (unsigned char *)&nsize, 2); |
| + bgp_notify_io_invalid(peer, BGP_NOTIFY_HEADER_ERR, |
| + BGP_NOTIFY_HEADER_BAD_MESLEN, |
| + (unsigned char *)&nsize, 2); |
| return false; |
| } |
| |
| diff --git a/bgpd/bgp_packet.c b/bgpd/bgp_packet.c |
| index 7daac4494..90695219a 100644 |
| --- a/bgpd/bgp_packet.c |
| +++ b/bgpd/bgp_packet.c |
| @@ -871,8 +871,9 @@ bool bgp_notify_received_hard_reset(struct peer *peer, uint8_t code, |
| * @param data Data portion |
| * @param datalen length of data portion |
| */ |
| -void bgp_notify_send_with_data(struct peer *peer, uint8_t code, |
| - uint8_t sub_code, uint8_t *data, size_t datalen) |
| +static void bgp_notify_send_internal(struct peer *peer, uint8_t code, |
| + uint8_t sub_code, uint8_t *data, |
| + size_t datalen, bool use_curr) |
| { |
| struct stream *s; |
| bool hard_reset = bgp_notify_send_hard_reset(peer, code, sub_code); |
| @@ -917,8 +918,11 @@ void bgp_notify_send_with_data(struct peer *peer, uint8_t code, |
| * If possible, store last packet for debugging purposes. This check is |
| * in place because we are sometimes called with a doppelganger peer, |
| * who tends to have a plethora of fields nulled out. |
| + * |
| + * Some callers should not attempt this - the io pthread for example |
| + * should not touch internals of the peer struct. |
| */ |
| - if (peer->curr) { |
| + if (use_curr && peer->curr) { |
| size_t packetsize = stream_get_endp(peer->curr); |
| assert(packetsize <= peer->max_packet_size); |
| memcpy(peer->last_reset_cause, peer->curr->data, packetsize); |
| @@ -1001,7 +1005,27 @@ void bgp_notify_send_with_data(struct peer *peer, uint8_t code, |
| */ |
| void bgp_notify_send(struct peer *peer, uint8_t code, uint8_t sub_code) |
| { |
| - bgp_notify_send_with_data(peer, code, sub_code, NULL, 0); |
| + bgp_notify_send_internal(peer, code, sub_code, NULL, 0, true); |
| +} |
| + |
| +/* |
| + * Enqueue notification; called from the main pthread, peer object access is ok. |
| + */ |
| +void bgp_notify_send_with_data(struct peer *peer, uint8_t code, |
| + uint8_t sub_code, uint8_t *data, size_t datalen) |
| +{ |
| + bgp_notify_send_internal(peer, code, sub_code, data, datalen, true); |
| +} |
| + |
| +/* |
| + * For use by the io pthread, queueing a notification but avoiding access to |
| + * the peer object. |
| + */ |
| +void bgp_notify_io_invalid(struct peer *peer, uint8_t code, uint8_t sub_code, |
| + uint8_t *data, size_t datalen) |
| +{ |
| + /* Avoid touching the peer object */ |
| + bgp_notify_send_internal(peer, code, sub_code, data, datalen, false); |
| } |
| |
| /* |
| diff --git a/bgpd/bgp_packet.h b/bgpd/bgp_packet.h |
| index a0eb579db..9f6d772bc 100644 |
| --- a/bgpd/bgp_packet.h |
| +++ b/bgpd/bgp_packet.h |
| @@ -62,6 +62,8 @@ extern void bgp_open_send(struct peer *); |
| extern void bgp_notify_send(struct peer *, uint8_t, uint8_t); |
| extern void bgp_notify_send_with_data(struct peer *, uint8_t, uint8_t, |
| uint8_t *, size_t); |
| +void bgp_notify_io_invalid(struct peer *peer, uint8_t code, uint8_t sub_code, |
| + uint8_t *data, size_t datalen); |
| extern void bgp_route_refresh_send(struct peer *peer, afi_t afi, safi_t safi, |
| uint8_t orf_type, uint8_t when_to_refresh, |
| int remove, uint8_t subtype); |
| -- |
| 2.25.1 |
| |