Patrick Williams | 8dd6848 | 2022-10-04 07:57:18 -0500 | [diff] [blame] | 1 | From 71ca5b09bc71e8cbe38177cf41e83fe164e52eee Mon Sep 17 00:00:00 2001 |
| 2 | From: Mark Stapp <mstapp@nvidia.com> |
| 3 | Date: Thu, 8 Sep 2022 16:14:36 -0400 |
| 4 | Subject: [PATCH] bgpd: avoid notify race between io and main pthreads |
| 5 | |
| 6 | The "bgp_notify_" apis in bgp_packet.c generate a notification |
| 7 | to a peer, usually during error handling. The io pthread wants |
| 8 | to send notifications in a couple of cases during early |
| 9 | received-packet validation - but the existing api interacts |
| 10 | with the peer struct itself, and that's not safe. |
| 11 | |
| 12 | Add a new api for use by the io pthread, and adjust the main |
| 13 | notify api so that it can avoid touching the peer struct. |
| 14 | |
| 15 | Signed-off-by: Mark Stapp <mstapp@nvidia.com> |
| 16 | |
| 17 | CVE: CVE-2022-37035 |
| 18 | |
| 19 | Upstream-Status: Backport |
| 20 | [https://github.com/FRRouting/frr/commit/71ca5b09bc71e8cbe38177cf41e83fe164e52eee] |
| 21 | |
| 22 | Signed-off-by: Yi Zhao <yi.zhao@windriver.com> |
| 23 | --- |
| 24 | bgpd/bgp_io.c | 17 ++++++++--------- |
| 25 | bgpd/bgp_packet.c | 32 ++++++++++++++++++++++++++++---- |
| 26 | bgpd/bgp_packet.h | 2 ++ |
| 27 | 3 files changed, 38 insertions(+), 13 deletions(-) |
| 28 | |
| 29 | diff --git a/bgpd/bgp_io.c b/bgpd/bgp_io.c |
| 30 | index 7af1fae28..f9bb8d518 100644 |
| 31 | --- a/bgpd/bgp_io.c |
| 32 | +++ b/bgpd/bgp_io.c |
| 33 | @@ -37,7 +37,7 @@ |
| 34 | #include "bgpd/bgp_debug.h" // for bgp_debug_neighbor_events, bgp_type_str |
| 35 | #include "bgpd/bgp_errors.h" // for expanded error reference information |
| 36 | #include "bgpd/bgp_fsm.h" // for BGP_EVENT_ADD, bgp_event |
| 37 | -#include "bgpd/bgp_packet.h" // for bgp_notify_send_with_data, bgp_notify... |
| 38 | +#include "bgpd/bgp_packet.h" // for bgp_notify_io_invalid... |
| 39 | #include "bgpd/bgp_trace.h" // for frrtraces |
| 40 | #include "bgpd/bgpd.h" // for peer, BGP_MARKER_SIZE, bgp_master, bm |
| 41 | /* clang-format on */ |
| 42 | @@ -526,8 +526,8 @@ static bool validate_header(struct peer *peer) |
| 43 | return false; |
| 44 | |
| 45 | if (memcmp(m_correct, m_rx, BGP_MARKER_SIZE) != 0) { |
| 46 | - bgp_notify_send(peer, BGP_NOTIFY_HEADER_ERR, |
| 47 | - BGP_NOTIFY_HEADER_NOT_SYNC); |
| 48 | + bgp_notify_io_invalid(peer, BGP_NOTIFY_HEADER_ERR, |
| 49 | + BGP_NOTIFY_HEADER_NOT_SYNC, NULL, 0); |
| 50 | return false; |
| 51 | } |
| 52 | |
| 53 | @@ -547,9 +547,8 @@ static bool validate_header(struct peer *peer) |
| 54 | zlog_debug("%s unknown message type 0x%02x", peer->host, |
| 55 | type); |
| 56 | |
| 57 | - bgp_notify_send_with_data(peer, BGP_NOTIFY_HEADER_ERR, |
| 58 | - BGP_NOTIFY_HEADER_BAD_MESTYPE, &type, |
| 59 | - 1); |
| 60 | + bgp_notify_io_invalid(peer, BGP_NOTIFY_HEADER_ERR, |
| 61 | + BGP_NOTIFY_HEADER_BAD_MESTYPE, &type, 1); |
| 62 | return false; |
| 63 | } |
| 64 | |
| 65 | @@ -574,9 +573,9 @@ static bool validate_header(struct peer *peer) |
| 66 | |
| 67 | uint16_t nsize = htons(size); |
| 68 | |
| 69 | - bgp_notify_send_with_data(peer, BGP_NOTIFY_HEADER_ERR, |
| 70 | - BGP_NOTIFY_HEADER_BAD_MESLEN, |
| 71 | - (unsigned char *)&nsize, 2); |
| 72 | + bgp_notify_io_invalid(peer, BGP_NOTIFY_HEADER_ERR, |
| 73 | + BGP_NOTIFY_HEADER_BAD_MESLEN, |
| 74 | + (unsigned char *)&nsize, 2); |
| 75 | return false; |
| 76 | } |
| 77 | |
| 78 | diff --git a/bgpd/bgp_packet.c b/bgpd/bgp_packet.c |
| 79 | index 7daac4494..90695219a 100644 |
| 80 | --- a/bgpd/bgp_packet.c |
| 81 | +++ b/bgpd/bgp_packet.c |
| 82 | @@ -871,8 +871,9 @@ bool bgp_notify_received_hard_reset(struct peer *peer, uint8_t code, |
| 83 | * @param data Data portion |
| 84 | * @param datalen length of data portion |
| 85 | */ |
| 86 | -void bgp_notify_send_with_data(struct peer *peer, uint8_t code, |
| 87 | - uint8_t sub_code, uint8_t *data, size_t datalen) |
| 88 | +static void bgp_notify_send_internal(struct peer *peer, uint8_t code, |
| 89 | + uint8_t sub_code, uint8_t *data, |
| 90 | + size_t datalen, bool use_curr) |
| 91 | { |
| 92 | struct stream *s; |
| 93 | bool hard_reset = bgp_notify_send_hard_reset(peer, code, sub_code); |
| 94 | @@ -917,8 +918,11 @@ void bgp_notify_send_with_data(struct peer *peer, uint8_t code, |
| 95 | * If possible, store last packet for debugging purposes. This check is |
| 96 | * in place because we are sometimes called with a doppelganger peer, |
| 97 | * who tends to have a plethora of fields nulled out. |
| 98 | + * |
| 99 | + * Some callers should not attempt this - the io pthread for example |
| 100 | + * should not touch internals of the peer struct. |
| 101 | */ |
| 102 | - if (peer->curr) { |
| 103 | + if (use_curr && peer->curr) { |
| 104 | size_t packetsize = stream_get_endp(peer->curr); |
| 105 | assert(packetsize <= peer->max_packet_size); |
| 106 | memcpy(peer->last_reset_cause, peer->curr->data, packetsize); |
| 107 | @@ -1001,7 +1005,27 @@ void bgp_notify_send_with_data(struct peer *peer, uint8_t code, |
| 108 | */ |
| 109 | void bgp_notify_send(struct peer *peer, uint8_t code, uint8_t sub_code) |
| 110 | { |
| 111 | - bgp_notify_send_with_data(peer, code, sub_code, NULL, 0); |
| 112 | + bgp_notify_send_internal(peer, code, sub_code, NULL, 0, true); |
| 113 | +} |
| 114 | + |
| 115 | +/* |
| 116 | + * Enqueue notification; called from the main pthread, peer object access is ok. |
| 117 | + */ |
| 118 | +void bgp_notify_send_with_data(struct peer *peer, uint8_t code, |
| 119 | + uint8_t sub_code, uint8_t *data, size_t datalen) |
| 120 | +{ |
| 121 | + bgp_notify_send_internal(peer, code, sub_code, data, datalen, true); |
| 122 | +} |
| 123 | + |
| 124 | +/* |
| 125 | + * For use by the io pthread, queueing a notification but avoiding access to |
| 126 | + * the peer object. |
| 127 | + */ |
| 128 | +void bgp_notify_io_invalid(struct peer *peer, uint8_t code, uint8_t sub_code, |
| 129 | + uint8_t *data, size_t datalen) |
| 130 | +{ |
| 131 | + /* Avoid touching the peer object */ |
| 132 | + bgp_notify_send_internal(peer, code, sub_code, data, datalen, false); |
| 133 | } |
| 134 | |
| 135 | /* |
| 136 | diff --git a/bgpd/bgp_packet.h b/bgpd/bgp_packet.h |
| 137 | index a0eb579db..9f6d772bc 100644 |
| 138 | --- a/bgpd/bgp_packet.h |
| 139 | +++ b/bgpd/bgp_packet.h |
| 140 | @@ -62,6 +62,8 @@ extern void bgp_open_send(struct peer *); |
| 141 | extern void bgp_notify_send(struct peer *, uint8_t, uint8_t); |
| 142 | extern void bgp_notify_send_with_data(struct peer *, uint8_t, uint8_t, |
| 143 | uint8_t *, size_t); |
| 144 | +void bgp_notify_io_invalid(struct peer *peer, uint8_t code, uint8_t sub_code, |
| 145 | + uint8_t *data, size_t datalen); |
| 146 | extern void bgp_route_refresh_send(struct peer *peer, afi_t afi, safi_t safi, |
| 147 | uint8_t orf_type, uint8_t when_to_refresh, |
| 148 | int remove, uint8_t subtype); |
| 149 | -- |
| 150 | 2.25.1 |
| 151 | |