Skip to content

Commit 4fbe60f

Browse files
aknoblochCQ Bot
authored andcommitted
[starnix] Skip invalid nexthop configuration packets
See the bug for more details, but the TL;DR is that Linux will silently discard invalid nexthop configurations, whilst continuing to process the valid configurations it may have received. This differs from the rust-netlink library's handling of invalid configurations: rust-netlink/netlink-packet-route#153 I've modified our fork to behave closer to Linux, but the above change was pulled down to stay as close as possible with the upstream source. Fixed: 387579405 Test: Local validation with repro script in bug Change-Id: Id158c9ab5a5c288c6cb9f3485c567bde6dd89144 Reviewed-on: https://fuchsia-review.googlesource.com/c/fuchsia/+/1235150 Commit-Queue: Aaron Knobloch <[email protected]> Reviewed-by: Jeff Martin <[email protected]>
1 parent 6206d5e commit 4fbe60f

File tree

3 files changed

+43
-13
lines changed

3 files changed

+43
-13
lines changed

src/starnix/lib/third_party/rust_netlink/netlink_packet_route/src/route/attribute.rs

Lines changed: 24 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -293,19 +293,30 @@ impl<'a, T: AsRef<[u8]> + ?Sized>
293293
let mut next_hops = vec![];
294294
let mut buf = payload;
295295
loop {
296-
let nh_buf = RouteNextHopBuffer::new_checked(&buf).map_err(|error| {
297-
RouteError::InvalidValue { kind: "RTA_MULTIPATH", error }
298-
})?;
299-
let len = nh_buf.length() as usize;
300-
let nh = RouteNextHop::parse_with_param(
301-
&nh_buf,
302-
(address_family, route_type, encap_type),
303-
)?;
304-
next_hops.push(nh);
305-
if buf.len() == len {
306-
break;
307-
}
308-
buf = &buf[len..];
296+
match RouteNextHopBuffer::new_checked(&buf) {
297+
Ok(nh_buf) => {
298+
let len = nh_buf.length() as usize;
299+
let nh = RouteNextHop::parse_with_param(
300+
&nh_buf,
301+
(address_family, route_type, encap_type),
302+
)?;
303+
next_hops.push(nh);
304+
if buf.len() == len {
305+
break;
306+
}
307+
buf = &buf[len..];
308+
}
309+
Err(e) => {
310+
// This is a Fuchsia-only change to the Netlink logic. In the
311+
// upstream rust-netlink library, they treat an invalid nexthop
312+
// configuration as an error, and promptly fail. Starnix instead
313+
// would prefer to silently discard these, to more closely match
314+
// Linux behavior. See b/387579405 for more context, and the upstream
315+
// commit: https://github.com/rust-netlink/netlink-packet-route/pull/153
316+
log::error!("Invalid nexthop configuration, {}", e);
317+
break;
318+
}
319+
};
309320
}
310321
Self::MultiPath(next_hops)
311322
}

src/starnix/lib/third_party/rust_netlink/netlink_packet_route/src/route/next_hops.rs

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,13 @@ impl<T: AsRef<[u8]>> RouteNextHopBuffer<T> {
6262
buffer_len: self.length() as usize,
6363
});
6464
}
65+
if (self.length() as usize) < PAYLOAD_OFFSET {
66+
return Err(DecodeError::InvalidBufferLength {
67+
name: "RouteNextHopBuffer",
68+
len: self.length() as usize,
69+
buffer_len: PAYLOAD_OFFSET,
70+
});
71+
}
6572
Ok(())
6673
}
6774
}

src/starnix/lib/third_party/rust_netlink/netlink_packet_route/src/route/tests/route_flags.rs

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -60,3 +60,15 @@ fn test_next_hop_max_buffer_len() {
6060
let buffer = [0xff, 0xff, 0, 0, 0, 0, 0, 0];
6161
assert!(RouteNextHopBuffer::new_checked(buffer).is_err());
6262
}
63+
64+
#[test]
65+
fn test_invalid_next_hop() {
66+
let buffer = [
67+
0, 0, // length
68+
0, // flags,
69+
0, // hops
70+
0, 0, 0, 0, // interface index
71+
0, 0, 0, 0, 0, 0, 0, 0, // payload
72+
];
73+
assert!(RouteNextHopBuffer::new_checked(buffer).is_err());
74+
}

0 commit comments

Comments
 (0)