1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
|
From db24300d56ad5831d9f6e4545ff2999b99e71bac 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 9b5a31f28..c736d02db 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 7c92a8d9e..a5ce5a527 100644
--- a/bgpd/bgp_packet.c
+++ b/bgpd/bgp_packet.c
@@ -736,8 +736,9 @@ static void bgp_write_notify(struct peer *peer)
* @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;
@@ -769,8 +770,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);
@@ -853,7 +857,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 280d3ec17..898f88ff5 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
|
