On Wed, Dec 11, 2024 at 01:46:49AM +0100, Stefano Brivio wrote:
> With glibc commit 25a5eb4010df ("string: strerror, strsignal cannot
> use buffer after dlmopen (bug 32026)"), strerror() now needs, at least
> on x86, the getrandom() and brk() system calls, in order to fill in
> the locale-translated error message. But getrandom() and brk() are not
> allowed by our seccomp profiles.
> 
> This became visible on Fedora Rawhide with the "podman login and
> logout" Podman tests, defined at test/e2e/login_logout_test.go in the
> Podman source tree, where pasta would terminate upon printing error
> descriptions (at least the ones related to the SO_ERROR queue for
> spliced connections).
> 
> Avoid dynamic memory allocation by calling strerrordesc_np() instead,
> which is a GNU function returning a static, untranslated version of
> the error description. If it's not available, keep calling strerror(),
> which at that point should be simple enough as to be usable (at least,
> that's currently the case for musl).
> 
> Reported-by: Paul Holzinger <pholzing@redhat.com>
> Link: https://github.com/containers/podman/issues/24804
> Analysed-by: Paul Holzinger <pholzing@redhat.com>
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
> v2: Name the wrapper strerror_() instead of __strerror()
> 
>  conf.c       | 10 +++++-----
>  icmp.c       |  4 ++--
>  log.c        |  2 +-
>  netlink.c    |  2 +-
>  pasta.c      | 22 +++++++++++-----------
>  tcp.c        | 22 +++++++++++-----------
>  tcp_splice.c | 16 ++++++++--------
>  udp.c        |  4 ++--
>  udp_flow.c   |  8 ++++----
>  util.c       |  6 +++---
>  util.h       | 32 ++++++++++++++++++++++++++++++++
>  11 files changed, 80 insertions(+), 48 deletions(-)
> 
> diff --git a/conf.c b/conf.c
> index eaa7d99..43daeca 100644
> --- a/conf.c
> +++ b/conf.c
> @@ -365,7 +365,7 @@ mode_conflict:
>  	die("Port forwarding mode '%s' conflicts with previous mode", optarg);
>  bind_fail:
>  	die("Failed to bind port %u (%s) for option '-%c %s', exiting",
> -	    i, strerror(-ret), optname, optarg);
> +	    i, strerror_(-ret), optname, optarg);
>  bind_all_fail:
>  	die("Failed to bind any port for '-%c %s', exiting", optname, optarg);
>  }
> @@ -655,7 +655,7 @@ static unsigned int conf_ip4(unsigned int ifi, struct ip4_ctx *ip4)
>  					  &ip4->guest_gw);
>  		if (rc < 0) {
>  			debug("Couldn't discover IPv4 gateway address: %s",
> -			      strerror(-rc));
> +			      strerror_(-rc));
>  			return 0;
>  		}
>  	}
> @@ -665,7 +665,7 @@ static unsigned int conf_ip4(unsigned int ifi, struct ip4_ctx *ip4)
>  				     &ip4->addr, &ip4->prefix_len, NULL);
>  		if (rc < 0) {
>  			debug("Couldn't discover IPv4 address: %s",
> -			      strerror(-rc));
> +			      strerror_(-rc));
>  			return 0;
>  		}
>  	}
> @@ -729,7 +729,7 @@ static unsigned int conf_ip6(unsigned int ifi, struct ip6_ctx *ip6)
>  		rc = nl_route_get_def(nl_sock, ifi, AF_INET6, &ip6->guest_gw);
>  		if (rc < 0) {
>  			debug("Couldn't discover IPv6 gateway address: %s",
> -			      strerror(-rc));
> +			      strerror_(-rc));
>  			return 0;
>  		}
>  	}
> @@ -738,7 +738,7 @@ static unsigned int conf_ip6(unsigned int ifi, struct ip6_ctx *ip6)
>  			 IN6_IS_ADDR_UNSPECIFIED(&ip6->addr) ? &ip6->addr : NULL,
>  			 &prefix_len, &ip6->our_tap_ll);
>  	if (rc < 0) {
> -		debug("Couldn't discover IPv6 address: %s", strerror(-rc));
> +		debug("Couldn't discover IPv6 address: %s", strerror_(-rc));
>  		return 0;
>  	}
>  
> diff --git a/icmp.c b/icmp.c
> index f514dbc..143e93b 100644
> --- a/icmp.c
> +++ b/icmp.c
> @@ -85,7 +85,7 @@ void icmp_sock_handler(const struct ctx *c, union epoll_ref ref)
>  
>  	n = recvfrom(ref.fd, buf, sizeof(buf), 0, &sr.sa, &sl);
>  	if (n < 0) {
> -		flow_err(pingf, "recvfrom() error: %s", strerror(errno));
> +		flow_err(pingf, "recvfrom() error: %s", strerror_(errno));
>  		return;
>  	}
>  
> @@ -301,7 +301,7 @@ int icmp_tap_handler(const struct ctx *c, uint8_t pif, sa_family_t af,
>  	pif_sockaddr(c, &sa, &sl, PIF_HOST, &tgt->eaddr, 0);
>  	if (sendto(pingf->sock, pkt, l4len, MSG_NOSIGNAL, &sa.sa, sl) < 0) {
>  		flow_dbg(pingf, "failed to relay request to socket: %s",
> -			 strerror(errno));
> +			 strerror_(errno));
>  	} else {
>  		flow_dbg(pingf,
>  			 "echo request to socket, ID: %"PRIu16", seq: %"PRIu16,
> diff --git a/log.c b/log.c
> index 239c8ce..95e4576 100644
> --- a/log.c
> +++ b/log.c
> @@ -322,7 +322,7 @@ void logmsg_perror(int pri, const char *format, ...)
>  	vlogmsg(false, false, pri, format, ap);
>  	va_end(ap);
>  
> -	logmsg(true, true, pri, ": %s", strerror(errno_copy));
> +	logmsg(true, true, pri, ": %s", strerror_(errno_copy));
>  }
>  
>  /**
> diff --git a/netlink.c b/netlink.c
> index 4aba2a3..0407692 100644
> --- a/netlink.c
> +++ b/netlink.c
> @@ -320,7 +320,7 @@ unsigned int nl_get_ext_if(int s, sa_family_t af)
>  	}
>  
>  	if (status < 0)
> -		warn("netlink: RTM_GETROUTE failed: %s", strerror(-status));
> +		warn("netlink: RTM_GETROUTE failed: %s", strerror_(-status));
>  
>  	if (defifi) {
>  		if (ndef > 1) {
> diff --git a/pasta.c b/pasta.c
> index 96dacc3..ff41c95 100644
> --- a/pasta.c
> +++ b/pasta.c
> @@ -296,7 +296,7 @@ void pasta_ns_conf(struct ctx *c)
>  	rc = nl_link_set_flags(nl_sock_ns, 1 /* lo */, IFF_UP, IFF_UP);
>  	if (rc < 0)
>  		die("Couldn't bring up loopback interface in namespace: %s",
> -		    strerror(-rc));
> +		    strerror_(-rc));
>  
>  	/* Get or set MAC in target namespace */
>  	if (MAC_IS_ZERO(c->guest_mac))
> @@ -305,7 +305,7 @@ void pasta_ns_conf(struct ctx *c)
>  		rc = nl_link_set_mac(nl_sock_ns, c->pasta_ifi, c->guest_mac);
>  	if (rc < 0)
>  		die("Couldn't set MAC address in namespace: %s",
> -		    strerror(-rc));
> +		    strerror_(-rc));
>  
>  	if (c->pasta_conf_ns) {
>  		unsigned int flags = IFF_UP;
> @@ -332,7 +332,7 @@ void pasta_ns_conf(struct ctx *c)
>  
>  			if (rc < 0) {
>  				die("Couldn't set IPv4 address(es) in namespace: %s",
> -				    strerror(-rc));
> +				    strerror_(-rc));
>  			}
>  
>  			if (c->ip4.no_copy_routes) {
> @@ -346,7 +346,7 @@ void pasta_ns_conf(struct ctx *c)
>  
>  			if (rc < 0) {
>  				die("Couldn't set IPv4 route(s) in guest: %s",
> -				    strerror(-rc));
> +				    strerror_(-rc));
>  			}
>  		}
>  
> @@ -355,13 +355,13 @@ void pasta_ns_conf(struct ctx *c)
>  					    &c->ip6.addr_ll_seen);
>  			if (rc < 0) {
>  				warn("Can't get LL address from namespace: %s",
> -				    strerror(-rc));
> +				    strerror_(-rc));
>  			}
>  
>  			rc = nl_addr_set_ll_nodad(nl_sock_ns, c->pasta_ifi);
>  			if (rc < 0) {
>  				warn("Can't set nodad for LL in namespace: %s",
> -				    strerror(-rc));
> +				    strerror_(-rc));
>  			}
>  
>  			/* We dodged DAD: re-enable neighbour solicitations */
> @@ -382,7 +382,7 @@ void pasta_ns_conf(struct ctx *c)
>  
>  			if (rc < 0) {
>  				die("Couldn't set IPv6 address(es) in namespace: %s",
> -				    strerror(-rc));
> +				    strerror_(-rc));
>  			}
>  
>  			if (c->ip6.no_copy_routes) {
> @@ -397,7 +397,7 @@ void pasta_ns_conf(struct ctx *c)
>  
>  			if (rc < 0) {
>  				die("Couldn't set IPv6 route(s) in guest: %s",
> -				    strerror(-rc));
> +				    strerror_(-rc));
>  			}
>  		}
>  	}
> @@ -446,18 +446,18 @@ void pasta_netns_quit_init(const struct ctx *c)
>  		return;
>  
>  	if ((dir_fd = open(c->netns_dir, O_CLOEXEC | O_RDONLY)) < 0)
> -		die("netns dir open: %s, exiting", strerror(errno));
> +		die("netns dir open: %s, exiting", strerror_(errno));
>  
>  	if (fstatfs(dir_fd, &s)          || s.f_type == DEVPTS_SUPER_MAGIC ||
>  	    s.f_type == PROC_SUPER_MAGIC || s.f_type == SYSFS_MAGIC)
>  		try_inotify = false;
>  
>  	if (try_inotify && (fd = inotify_init1(flags)) < 0)
> -		warn("inotify_init1(): %s, use a timer", strerror(errno));
> +		warn("inotify_init1(): %s, use a timer", strerror_(errno));
>  
>  	if (fd >= 0 && inotify_add_watch(fd, c->netns_dir, IN_DELETE) < 0) {
>  		warn("inotify_add_watch(): %s, use a timer",
> -		     strerror(errno));
> +		     strerror_(errno));
>  		close(fd);
>  		fd = -1;
>  	}
> diff --git a/tcp.c b/tcp.c
> index 1872ccb..ec433f7 100644
> --- a/tcp.c
> +++ b/tcp.c
> @@ -516,7 +516,7 @@ static void tcp_timer_ctl(const struct ctx *c, struct tcp_tap_conn *conn)
>  		fd = timerfd_create(CLOCK_MONOTONIC, 0);
>  		if (fd == -1 || fd > FD_REF_MAX) {
>  			flow_dbg(conn, "failed to get timer: %s",
> -				 strerror(errno));
> +				 strerror_(errno));
>  			if (fd > -1)
>  				close(fd);
>  			conn->timer = -1;
> @@ -526,7 +526,7 @@ static void tcp_timer_ctl(const struct ctx *c, struct tcp_tap_conn *conn)
>  
>  		if (epoll_ctl(c->epollfd, EPOLL_CTL_ADD, conn->timer, &ev)) {
>  			flow_dbg(conn, "failed to add timer: %s",
> -				 strerror(errno));
> +				 strerror_(errno));
>  			close(conn->timer);
>  			conn->timer = -1;
>  			return;
> @@ -551,7 +551,7 @@ static void tcp_timer_ctl(const struct ctx *c, struct tcp_tap_conn *conn)
>  		 (unsigned long long)it.it_value.tv_nsec / 1000 / 1000);
>  
>  	if (timerfd_settime(conn->timer, 0, &it, NULL))
> -		flow_err(conn, "failed to set timer: %s", strerror(errno));
> +		flow_err(conn, "failed to set timer: %s", strerror_(errno));
>  }
>  
>  /**
> @@ -1307,7 +1307,7 @@ int tcp_conn_sock(const struct ctx *c, sa_family_t af)
>  		return s;
>  
>  	err("TCP: Unable to open socket for new connection: %s",
> -	    strerror(-s));
> +	    strerror_(-s));
>  	return -1;
>  }
>  
> @@ -1360,7 +1360,7 @@ static void tcp_bind_outbound(const struct ctx *c,
>  			flow_dbg(conn,
>  				 "Can't bind TCP outbound socket to %s:%hu: %s",
>  				 inany_ntop(&tgt->oaddr, sstr, sizeof(sstr)),
> -				 tgt->oport, strerror(errno));
> +				 tgt->oport, strerror_(errno));
>  		}
>  	}
>  
> @@ -1371,7 +1371,7 @@ static void tcp_bind_outbound(const struct ctx *c,
>  				       strlen(c->ip4.ifname_out))) {
>  				flow_dbg(conn, "Can't bind IPv4 TCP socket to"
>  					 " interface %s: %s", c->ip4.ifname_out,
> -					 strerror(errno));
> +					 strerror_(errno));
>  			}
>  		}
>  	} else if (bind_sa.sa_family == AF_INET6) {
> @@ -1381,7 +1381,7 @@ static void tcp_bind_outbound(const struct ctx *c,
>  				       strlen(c->ip6.ifname_out))) {
>  				flow_dbg(conn, "Can't bind IPv6 TCP socket to"
>  					 " interface %s: %s", c->ip6.ifname_out,
> -					 strerror(errno));
> +					 strerror_(errno));
>  			}
>  		}
>  	}
> @@ -2113,7 +2113,7 @@ void tcp_timer_handler(const struct ctx *c, union epoll_ref ref)
>  	 * and we just set the timer to a new point in the future: discard it.
>  	 */
>  	if (timerfd_gettime(conn->timer, &check_armed))
> -		flow_err(conn, "failed to read timer: %s", strerror(errno));
> +		flow_err(conn, "failed to read timer: %s", strerror_(errno));
>  
>  	if (check_armed.it_value.tv_sec || check_armed.it_value.tv_nsec)
>  		return;
> @@ -2154,7 +2154,7 @@ void tcp_timer_handler(const struct ctx *c, union epoll_ref ref)
>  		 */
>  		if (timerfd_settime(conn->timer, 0, &new, &old))
>  			flow_err(conn, "failed to set timer: %s",
> -				 strerror(errno));
> +				 strerror_(errno));
>  
>  		if (old.it_value.tv_sec == ACT_TIMEOUT) {
>  			flow_dbg(conn, "activity timeout");
> @@ -2422,13 +2422,13 @@ static void tcp_sock_refill_init(const struct ctx *c)
>  		int rc = tcp_sock_refill_pool(c, init_sock_pool4, AF_INET);
>  		if (rc < 0)
>  			warn("TCP: Error refilling IPv4 host socket pool: %s",
> -			     strerror(-rc));
> +			     strerror_(-rc));
>  	}
>  	if (c->ifi6) {
>  		int rc = tcp_sock_refill_pool(c, init_sock_pool6, AF_INET6);
>  		if (rc < 0)
>  			warn("TCP: Error refilling IPv6 host socket pool: %s",
> -			     strerror(-rc));
> +			     strerror_(-rc));
>  	}
>  }
>  
> diff --git a/tcp_splice.c b/tcp_splice.c
> index 93f8bce..3a0f868 100644
> --- a/tcp_splice.c
> +++ b/tcp_splice.c
> @@ -160,7 +160,7 @@ static int tcp_splice_epoll_ctl(const struct ctx *c,
>  	if (epoll_ctl(c->epollfd, m, conn->s[0], &ev[0]) ||
>  	    epoll_ctl(c->epollfd, m, conn->s[1], &ev[1])) {
>  		int ret = -errno;
> -		flow_err(conn, "ERROR on epoll_ctl(): %s", strerror(errno));
> +		flow_err(conn, "ERROR on epoll_ctl(): %s", strerror_(errno));
>  		return ret;
>  	}
>  
> @@ -314,7 +314,7 @@ static int tcp_splice_connect_finish(const struct ctx *c,
>  		if (conn->pipe[sidei][0] < 0) {
>  			if (pipe2(conn->pipe[sidei], O_NONBLOCK | O_CLOEXEC)) {
>  				flow_err(conn, "cannot create %d->%d pipe: %s",
> -					 sidei, !sidei, strerror(errno));
> +					 sidei, !sidei, strerror_(errno));
>  				conn_flag(c, conn, CLOSING);
>  				return -EIO;
>  			}
> @@ -370,7 +370,7 @@ static int tcp_splice_connect(const struct ctx *c, struct tcp_splice_conn *conn)
>  	if (connect(conn->s[1], &sa.sa, sl)) {
>  		if (errno != EINPROGRESS) {
>  			flow_trace(conn, "Couldn't connect socket for splice: %s",
> -				   strerror(errno));
> +				   strerror_(errno));
>  			return -errno;
>  		}
>  
> @@ -469,10 +469,10 @@ void tcp_splice_sock_handler(struct ctx *c, union epoll_ref ref,
>  		rc = getsockopt(ref.fd, SOL_SOCKET, SO_ERROR, &err, &sl);
>  		if (rc)
>  			flow_err(conn, "Error retrieving SO_ERROR: %s",
> -				 strerror(errno));
> +				 strerror_(errno));
>  		else
>  			flow_trace(conn, "Error event on socket: %s",
> -				   strerror(err));
> +				   strerror_(err));
>  
>  		goto close;
>  	}
> @@ -551,7 +551,7 @@ eintr:
>  					       &lowat, sizeof(lowat))) {
>  					flow_trace(conn,
>  						   "Setting SO_RCVLOWAT %i: %s",
> -						   lowat, strerror(errno));
> +						   lowat, strerror_(errno));
>  				} else {
>  					conn_flag(c, conn, lowat_set_flag);
>  					conn_flag(c, conn, lowat_act_flag);
> @@ -696,13 +696,13 @@ static int tcp_sock_refill_ns(void *arg)
>  		int rc = tcp_sock_refill_pool(c, ns_sock_pool4, AF_INET);
>  		if (rc < 0)
>  			warn("TCP: Error refilling IPv4 ns socket pool: %s",
> -			     strerror(-rc));
> +			     strerror_(-rc));
>  	}
>  	if (c->ifi6) {
>  		int rc = tcp_sock_refill_pool(c, ns_sock_pool6, AF_INET6);
>  		if (rc < 0)
>  			warn("TCP: Error refilling IPv6 ns socket pool: %s",
> -			     strerror(-rc));
> +			     strerror_(-rc));
>  	}
>  
>  	return 0;
> diff --git a/udp.c b/udp.c
> index c89f031..923cc38 100644
> --- a/udp.c
> +++ b/udp.c
> @@ -453,7 +453,7 @@ static int udp_sock_recverr(int s)
>  
>  	/* TODO: When possible propagate and otherwise handle errors */
>  	debug("%s error on UDP socket %i: %s",
> -	      str_ee_origin(ee), s, strerror(ee->ee_errno));
> +	      str_ee_origin(ee), s, strerror_(ee->ee_errno));
>  
>  	return 1;
>  }
> @@ -492,7 +492,7 @@ int udp_sock_errs(const struct ctx *c, int s, uint32_t events)
>  	}
>  
>  	if (err) {
> -		debug("Unqueued error on UDP socket %i: %s", s, strerror(err));
> +		debug("Unqueued error on UDP socket %i: %s", s, strerror_(err));
>  		n_err++;
>  	}
>  
> diff --git a/udp_flow.c b/udp_flow.c
> index c8fdb5f..343caae 100644
> --- a/udp_flow.c
> +++ b/udp_flow.c
> @@ -95,7 +95,7 @@ static flow_sidx_t udp_flow_new(const struct ctx *c, union flow *flow,
>  		if (uflow->s[INISIDE] < 0) {
>  			flow_err(uflow,
>  				 "Couldn't duplicate listening socket: %s",
> -				 strerror(errno));
> +				 strerror_(errno));
>  			goto cancel;
>  		}
>  	}
> @@ -115,14 +115,14 @@ static flow_sidx_t udp_flow_new(const struct ctx *c, union flow *flow,
>  		if (uflow->s[TGTSIDE] < 0) {
>  			flow_dbg(uflow,
>  				 "Couldn't open socket for spliced flow: %s",
> -				 strerror(errno));
> +				 strerror_(errno));
>  			goto cancel;
>  		}
>  
>  		if (flowside_connect(c, uflow->s[TGTSIDE], tgtpif, tgt) < 0) {
>  			flow_dbg(uflow,
>  				 "Couldn't connect flow socket: %s",
> -				 strerror(errno));
> +				 strerror_(errno));
>  			goto cancel;
>  		}
>  
> @@ -144,7 +144,7 @@ static flow_sidx_t udp_flow_new(const struct ctx *c, union flow *flow,
>  		} else if (errno != EAGAIN) {
>  			flow_err(uflow,
>  				 "Unexpected error discarding datagrams: %s",
> -				 strerror(errno));
> +				 strerror_(errno));
>  		}
>  	}
>  
> diff --git a/util.c b/util.c
> index 55cae3f..11973c4 100644
> --- a/util.c
> +++ b/util.c
> @@ -90,7 +90,7 @@ int sock_l4_sa(const struct ctx *c, enum epoll_type type,
>  
>  	ret = -errno;
>  	if (fd < 0) {
> -		warn("L4 socket: %s", strerror(-ret));
> +		warn("L4 socket: %s", strerror_(-ret));
>  		return ret;
>  	}
>  
> @@ -162,7 +162,7 @@ int sock_l4_sa(const struct ctx *c, enum epoll_type type,
>  
>  	if (type == EPOLL_TYPE_TCP_LISTEN && listen(fd, 128) < 0) {
>  		ret = -errno;
> -		warn("TCP socket listen: %s", strerror(-ret));
> +		warn("TCP socket listen: %s", strerror_(-ret));
>  		close(fd);
>  		return ret;
>  	}
> @@ -171,7 +171,7 @@ int sock_l4_sa(const struct ctx *c, enum epoll_type type,
>  	ev.data.u64 = ref.u64;
>  	if (epoll_ctl(c->epollfd, EPOLL_CTL_ADD, fd, &ev) == -1) {
>  		ret = -errno;
> -		warn("L4 epoll_ctl: %s", strerror(-ret));
> +		warn("L4 epoll_ctl: %s", strerror_(-ret));
>  		return ret;
>  	}
>  
> diff --git a/util.h b/util.h
> index 41bbd60..3fa1d12 100644
> --- a/util.h
> +++ b/util.h
> @@ -274,6 +274,38 @@ static inline bool mod_between(unsigned x, unsigned i, unsigned j, unsigned m)
>  
>  void raw_random(void *buf, size_t buflen);
>  
> +/*
> + * Starting from glibc 2.40.9000 and commit 25a5eb4010df ("string: strerror,
> + * strsignal cannot use buffer after dlmopen (bug 32026)"), strerror() needs
> + * getrandom(2) and brk(2) as it allocates memory for the locale-translated
> + * error description, but our seccomp profiles forbid both.
> + *
> + * Use the strerror_() wrapper instead, calling into strerrordesc_np() to get
> + * a static untranslated string. It's a GNU implementation, but also defined by
> + * bionic.
> + *
> + * If strerrordesc_np() is not defined (e.g. musl), call strerror(). C libraries
> + * not defining strerrordesc_np() are expected to provide strerror()
> + * implementations that are simple enough for us to call.
> + */
> +__attribute__ ((weak)) const char *strerrordesc_np(int errnum);
> +
> +/**
> + * strerror_() - strerror() wrapper calling strerrordesc_np() if available
> + * @errnum:	Error code
> + *
> + * Return: error description string
> + */
> +static inline const char *strerror_(int errnum)
> +{
> +	if (strerrordesc_np)
> +		return strerrordesc_np(errnum);
> +
> +	return strerror(errnum);
> +}
> +
> +#define strerror(x) @ "Don't call strerror() directly, use strerror_() instead"
> +
>  /*
>   * Workarounds for https://github.com/llvm/llvm-project/issues/58992
>   *

-- 
David Gibson (he or they)	| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you, not the other way
				| around.
http://www.ozlabs.org/~dgibson