From 7ee27418a7d38542577c48abc43df57c0d3e69a5 Mon Sep 17 00:00:00 2001 From: Maria Matejka Date: Thu, 14 Nov 2024 23:34:28 +0100 Subject: [PATCH 1/6] Printf: impossible buffer overflow fix When printing near the end of the buffer, there was an overflow in two cases: (1) %c and size is zero (2) %1N, %1I, %1I4, %1I6 (auto-fill field_width for Net or IP), size is more than actual length of the net/ip but less than the auto-filled field width. Manual code examination showed that nothing could have ever triggered this behavior. All older versions of BIRD, including BIRD 3 development versions, are totally safe. This exact overflow has been found while implementing a new feature in later commits. --- lib/printf.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/printf.c b/lib/printf.c index 318e683c..0d2f95e8 100644 --- a/lib/printf.c +++ b/lib/printf.c @@ -169,9 +169,9 @@ int bvsnprintf(char *buf, int size, const char *fmt, va_list args) int qualifier; /* 'h' or 'l' for integer fields */ for (start=str=buf ; *fmt ; ++fmt, size-=(str-start), start=str) { + if (!size) + return -1; if (*fmt != '%') { - if (!size) - return -1; *str++ = *fmt; continue; } @@ -272,7 +272,7 @@ int bvsnprintf(char *buf, int size, const char *fmt, va_list args) len = strlen(s); if (precision >= 0 && len > precision) len = precision; - if (len > size) + if ((len > size) || (field_width > size)) return -1; if (!(flags & LEFT)) From 946386f2dd69982105d0b02a90f57391b265d2ef Mon Sep 17 00:00:00 2001 From: Maria Matejka Date: Thu, 14 Nov 2024 22:15:56 +0100 Subject: [PATCH 2/6] MRT: instead of crashing, ignore non-BGP attributes --- proto/bgp/attrs.c | 6 +++++- proto/bgp/bgp.h | 1 + proto/mrt/mrt.c | 5 ++++- 3 files changed, 10 insertions(+), 2 deletions(-) diff --git a/proto/bgp/attrs.c b/proto/bgp/attrs.c index 5bd05fb2..0872ae23 100644 --- a/proto/bgp/attrs.c +++ b/proto/bgp/attrs.c @@ -1247,7 +1247,11 @@ bgp_export_attrs(struct bgp_export_state *s, ea_list *attrs) static inline int bgp_encode_attr(struct bgp_write_state *s, eattr *a, byte *buf, uint size) { - ASSERT(EA_PROTO(a->id) == PROTOCOL_BGP); + if (EA_PROTO(a->id) != PROTOCOL_BGP) + if (s->ignore_non_bgp_attrs) + return 0; + else + bug("Tried to encode a non-BGP attribute"); uint code = EA_ID(a->id); diff --git a/proto/bgp/bgp.h b/proto/bgp/bgp.h index 4cef8caf..19b44372 100644 --- a/proto/bgp/bgp.h +++ b/proto/bgp/bgp.h @@ -453,6 +453,7 @@ struct bgp_write_state { int add_path; int mpls; int sham; + int ignore_non_bgp_attrs; eattr *mp_next_hop; const adata *mpls_labels; diff --git a/proto/mrt/mrt.c b/proto/mrt/mrt.c index 3378bace..1336837a 100644 --- a/proto/mrt/mrt.c +++ b/proto/mrt/mrt.c @@ -594,7 +594,10 @@ mrt_table_dump_free(struct mrt_table_dump_state *s) static int mrt_table_dump_step(struct mrt_table_dump_state *s) { - struct bgp_write_state bws = { .as4_session = 1 }; + struct bgp_write_state bws = { + .as4_session = 1, + .ignore_non_bgp_attrs = 1, + }; s->max = 2048; s->bws = &bws; From 145830bdc871dc97d7d170ec52932cbcf63fe803 Mon Sep 17 00:00:00 2001 From: Maria Matejka Date: Thu, 14 Nov 2024 20:46:45 +0100 Subject: [PATCH 3/6] CLI: adding cli_vprintf() --- nest/cli.c | 5 +---- nest/cli.h | 10 +++++++++- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/nest/cli.c b/nest/cli.c index 9573f770..42f1d6cd 100644 --- a/nest/cli.c +++ b/nest/cli.c @@ -117,9 +117,8 @@ cli_alloc_out(cli *c, int size) * macro instead. */ void -cli_printf(cli *c, int code, char *msg, ...) +cli_vprintf(cli *c, int code, const char *msg, va_list args) { - va_list args; byte buf[CLI_LINE_SIZE]; int cd = code; int errcode; @@ -147,9 +146,7 @@ cli_printf(cli *c, int code, char *msg, ...) } c->last_reply = cd; - va_start(args, msg); cnt = bvsnprintf(buf+size, sizeof(buf)-size-1, msg, args); - va_end(args); if (cnt < 0) { cli_printf(c, errcode, ""); diff --git a/nest/cli.h b/nest/cli.h index e48216c2..423b2180 100644 --- a/nest/cli.h +++ b/nest/cli.h @@ -75,7 +75,15 @@ extern struct cli *this_cli; /* Used during parsing */ /* Functions to be called by command handlers */ -void cli_printf(cli *, int, char *, ...); +void cli_vprintf(cli *, int, const char *, va_list); +static inline void cli_printf(cli *cli, int code, const char *fmt, ...) +{ + va_list args; + va_start(args, fmt); + cli_vprintf(cli, code, fmt, args); + va_end(args); +} + #define cli_msg(x...) cli_printf(this_cli, x) void cli_set_log_echo(cli *, uint mask, uint size); void cli_set_timeformat(cli *c, const struct timeformat tf); From 4dd5b3d90e962d5fd1794f97a8f82077f8f1eb36 Mon Sep 17 00:00:00 2001 From: Maria Matejka Date: Thu, 14 Nov 2024 21:20:49 +0100 Subject: [PATCH 4/6] Logging: exposing vlog() to log va_lists --- client/util.c | 6 +++--- lib/birdlib.h | 2 ++ sysdep/unix/log.c | 2 +- 3 files changed, 6 insertions(+), 4 deletions(-) diff --git a/client/util.c b/client/util.c index 1d83e518..3a78a7c4 100644 --- a/client/util.c +++ b/client/util.c @@ -17,7 +17,7 @@ /* Client versions of logging functions */ static void -vlog(const char *msg, va_list args) +vlog_cli(const char *msg, va_list args) { char buf[1024]; @@ -38,7 +38,7 @@ bug(const char *msg, ...) va_start(args, msg); cleanup(); fputs("Internal error: ", stderr); - vlog(msg, args); + vlog_cli(msg, args); vfprintf(stderr, msg, args); va_end(args); exit(1); @@ -51,7 +51,7 @@ die(const char *msg, ...) va_start(args, msg); cleanup(); - vlog(msg, args); + vlog_cli(msg, args); va_end(args); exit(1); } diff --git a/lib/birdlib.h b/lib/birdlib.h index 8852b609..4fea3f9f 100644 --- a/lib/birdlib.h +++ b/lib/birdlib.h @@ -10,6 +10,7 @@ #define _BIRD_BIRDLIB_H_ #include "lib/alloca.h" +#include /* Ugly structure offset handling macros */ @@ -159,6 +160,7 @@ void log_msg(const char *msg, ...); void log_rl(struct tbf *rl, const char *msg, ...); void die(const char *msg, ...) NORET; void bug(const char *msg, ...) NORET; +void vlog(int class, const char *msg, va_list args); #define L_DEBUG "\001" /* Debugging messages */ #define L_TRACE "\002" /* Protocol tracing */ diff --git a/sysdep/unix/log.c b/sysdep/unix/log.c index 613a6aa5..6c7cac54 100644 --- a/sysdep/unix/log.c +++ b/sysdep/unix/log.c @@ -274,7 +274,7 @@ log_commit(int class, buffer *buf) int buffer_vprint(buffer *buf, const char *fmt, va_list args); -static void +void vlog(int class, const char *msg, va_list args) { buffer buf; From 73ace6acad0b434849e0a9cd51603cd49c136cb3 Mon Sep 17 00:00:00 2001 From: Maria Matejka Date: Wed, 11 Dec 2024 10:05:05 +0100 Subject: [PATCH 5/6] Proto show: show creation and last autorestart time --- nest/proto.c | 41 +++++++++++++++++++++++++++++++---------- nest/protocol.h | 1 + 2 files changed, 32 insertions(+), 10 deletions(-) diff --git a/nest/proto.c b/nest/proto.c index 5a46514b..1029957a 100644 --- a/nest/proto.c +++ b/nest/proto.c @@ -1334,6 +1334,7 @@ proto_new(struct proto_config *cf) p->proto = cf->protocol; p->proto_state = PS_DOWN_XX; p->last_state_change = current_time(); + p->last_reconfiguration = current_time(); p->net_type = cf->net_type; p->disabled = cf->disabled; @@ -2183,14 +2184,17 @@ proto_restart_event_hook(void *_p) proto_rethink_goal(p); if (proto_restart) - if (current_time_now() - p->last_restart < p->restart_limit) - log(L_ERR "%s: too frequent restarts, disabling", p->name); - else - p->disabled = 0; + { + /* Store the last restart time */ + p->last_restart = current_time(); + + /* Allow the protocol back again */ + p->disabled = 0; /* No need to call proto_rethink_goal() here again as the proto_cleanup() routine will * call it after the protocol stops ... and both these routines are fixed to main_birdloop. */ + } } static inline void @@ -2306,8 +2310,16 @@ channel_limit_down(struct limit *l, void *data) channel_activate_limit(c, l, dir); + bool restart = (c->limit_actions[dir] == PLA_RESTART); + + if (restart && (current_time_now() - p->last_restart < p->restart_limit)) + { + log(L_ERR "%s: too frequent restarts, disabling", p->name); + restart = false; + } + if (p->proto_state == PS_UP) - proto_schedule_down(p, c->limit_actions[dir] == PLA_RESTART, chl_dir_down[dir]); + proto_schedule_down(p, restart, chl_dir_down[dir]); return 1; } @@ -2619,8 +2631,10 @@ proto_cmd_show(struct proto *p, uintptr_t verbose, int cnt) p->proto->get_status(p, buf); rcu_read_lock(); - tm_format_time(tbuf, this_cli->tf ?: &atomic_load_explicit(&global_runtime, memory_order_acquire)->tf_proto, p->last_state_change); + struct timeformat *tf = this_cli->tf ?: &atomic_load_explicit(&global_runtime, memory_order_acquire)->tf_proto; rcu_read_unlock(); + + tm_format_time(tbuf, tf, p->last_state_change); cli_msg(-1002, "%-10s %-10s %-10s %-6s %-12s %s", p->name, p->proto->name, @@ -2632,13 +2646,20 @@ proto_cmd_show(struct proto *p, uintptr_t verbose, int cnt) if (verbose) { if (p->cf->dsc) - cli_msg(-1006, " Description: %s", p->cf->dsc); + cli_msg(-1006, " Description: %s", p->cf->dsc); if (p->message) - cli_msg(-1006, " Message: %s", p->message); + cli_msg(-1006, " Message: %s", p->message); + tm_format_time(tbuf, tf, p->last_reconfiguration); + cli_msg(-1006, " Created: %s", tbuf); + if (p->last_restart > p->last_reconfiguration) + { + tm_format_time(tbuf, tf, p->last_restart); + cli_msg(-1006, " Last autorestart: %s", tbuf); + } if (p->cf->router_id) - cli_msg(-1006, " Router ID: %R", p->cf->router_id); + cli_msg(-1006, " Router ID: %R", p->cf->router_id); if (p->vrf) - cli_msg(-1006, " VRF: %s", p->vrf->name); + cli_msg(-1006, " VRF: %s", p->vrf->name); if (p->proto->show_proto_info) p->proto->show_proto_info(p); diff --git a/nest/protocol.h b/nest/protocol.h index 171a0a5b..5e1098c2 100644 --- a/nest/protocol.h +++ b/nest/protocol.h @@ -170,6 +170,7 @@ struct proto { u32 hash_key; /* Random key used for hashing of neighbors */ btime last_state_change; /* Time of last state transition */ btime last_restart; /* Time of last restart */ + btime last_reconfiguration; /* Time of last hard reconfiguration */ btime restart_limit; /* Minimum allowed time between limit restarts */ char *last_state_name_announced; /* Last state name we've announced to the user */ char *message; /* State-change message, allocated from proto_pool */ From 28b0adc15f82996a737fd832ef4a934dbbf29d93 Mon Sep 17 00:00:00 2001 From: Maria Matejka Date: Wed, 11 Dec 2024 17:51:46 +0100 Subject: [PATCH 6/6] Fix alignment requirements to include atomic u64. Not having this led to bus errors on unaligned atomic u64 access on architectures with 4B pointers. --- lib/mempool.c | 1 + lib/resource.c | 5 ++++- lib/slab.c | 2 ++ 3 files changed, 7 insertions(+), 1 deletion(-) diff --git a/lib/mempool.c b/lib/mempool.c index 5940e571..6de44b6b 100644 --- a/lib/mempool.c +++ b/lib/mempool.c @@ -29,6 +29,7 @@ struct lp_chunk { struct lp_chunk *next; struct linpool *lp; uintptr_t data_align[0]; + _Atomic u64 data_align_atomic[0]; byte data[0]; }; diff --git a/lib/resource.c b/lib/resource.c index b89f772c..17af79d7 100644 --- a/lib/resource.c +++ b/lib/resource.c @@ -373,7 +373,10 @@ tmp_flush(void) struct mblock { resource r; unsigned size; - uintptr_t data_align[0]; + union { + uintptr_t bigint; + _Atomic u64 atom; + } _align[0]; byte data[0]; }; diff --git a/lib/slab.c b/lib/slab.c index ab913f0c..276ea8d0 100644 --- a/lib/slab.c +++ b/lib/slab.c @@ -171,6 +171,8 @@ struct sl_head { struct sl_alignment { /* Magic structure for testing of alignment */ byte data; int x[0]; + _Atomic u64 y[0]; + void *z[0]; }; #define TLIST_PREFIX sl_head