Re: [PATCH 2/5] fuzz: Add flow type guards for fuzzing stability

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 10806 bytes --]

On Wed, Aug 12, 2026 at 12:56:25PM +0530, Anshu Kumari wrote:
> Under FUZZING, AFL++ can inject arbitrary epoll event types
> from its shared memory buffer. When an event references a flow
> table entry whose type doesn't match the handler, the existing
> assert() crashes the process eventually masking the real bugs.

This makes sense to me up until the "eventually masking the real
bugs".  IIUC the way AFL works is that it will use the coverage
feedback to adjust its input - so even if many inputs result in a
early crash due to mismatching types, it should be able to discover
inputs that get past that into more interesting territory.

Is it just a question of trying to filter our all the uninteresting
crashes from flow type mismatches?  Or to put it another way,
distinguishing between bugs and correct abnormal exits due to
something going wrong?

> If there is no flow at the start of fuzzing then also we are
> just returning early instead of hitting crashes.

I don't really understand what you mean by this.

> Allowing the
> fuzzer to explore other code path.
> 
> Replace assert() with NULL returns in the flow-lookup functions
> when compiled with -DFUZZING:
> 
> - tcp.c: conn_at_sidx(), tcp_timer_handler(), tcp_sock_handler()
> - tcp_splice.c: conn_at_sidx(), tcp_splice_sock_handler()
> - udp.c: udp_sock_handler(), udp_sock_to_sock(),
>   udp_buf_sock_to_tap(), udp_sock_fwd() error path
> - udp_flow.c: udp_at_sidx()
> - icmp.c: ping_at_sidx(), icmp_sock_handler()

So, in looking at this, I'm bearing in mind the distinction between
what should be a die() and what should be an assert().  Generally if
something external did something wrong we can't cope with, it's a
die() - and that's not a bug.  If we did something wrong it's an
assert() and that is a bug.

The tricky case is where the "external" thing is the kernel.  We
obviously have to trust it to some extent, so if we get something we
never expect to get from a kernel interace is that a die() or an
assert().

The reasoning for these being assert()s is that if we get something
unexpected here, by far the most likely cause is that we did something
wrong when we set the epoll reference, rather than the kernel
returning garbage.

Fuzzing, at least with the current draft setup is breaking that
assumption because the simulated kernel *is* returning garbage a lot
of the time.

So, we either need to constrain generation of epoll events to
"plausible" ones, or safely ignore bad events.  AIUI this patch is
taking the second approach.

That basic approach makes sense to me, but I'm not loving the way it
works out in practice.  It's fairly ugly and invasive, for starters.
But, hat concerns me more though, is that at the points you're
removing the assert()s it's not very obvious what new other paths the
early returns will trigger.  Because those are paths we'll never reach
in real workloads, they're also not that interesting to fuzz.

I can think of two possible approaches that might be this nicer.

1) IIUC the way the coverage driven fuzzing works, in the assert()
   cases we don't really need to carry on looking for other bugs.  It
   should be ok to exit immediately as long as that is flagged as "we
   exited because something went wrong externally" not "we exited
   because we hit a bug".  The coverage driven engine should then be
   able to go back and generate new inputs that explore other paths.

   I don't really know the interfaces used to communicate with AFL++,
   but could we replace these assert()s with say fuzz_assert(), which
   still exits, but signals to the fuzzer that this exit is not a bug?
   Is changing the abort() to a die() #ifdef FUZZING enough to do that?

2) We could move epoll event validation to immediately after the
   epoll_wait().  We esentially want to filter generated epoll events
   to plausible ones - at least meaning that the returned reference is
   one of the ones we epoll_add()ed.

   In one way, that's awkward, because we have to have a single point
   with all the validation logic, which might vary across various
   different branches.  On the other hand, it makes the invasiveness
   much more localised, and it's clearer what a validation failure
   will do, whether that's exit in a non-bug way, or just ignore this
   event and go on to the next one.

> Signed-off-by: Anshu Kumari <anskuma@redhat.com>
> ---
>  icmp.c       | 14 +++++++++++++-
>  tcp.c        | 19 ++++++++++++++++++-
>  tcp_splice.c | 10 ++++++++++
>  udp.c        | 29 +++++++++++++++++++++++++++--
>  udp_flow.c   |  5 +++++
>  5 files changed, 73 insertions(+), 4 deletions(-)
> 
> diff --git a/icmp.c b/icmp.c
> index 0fe2366..cdfa253 100644
> --- a/icmp.c
> +++ b/icmp.c
> @@ -39,6 +39,7 @@
>  #include "icmp.h"
>  #include "flow_table.h"
>  #include "epoll_ctl.h"
> +#include "fuzz.h"
>  
>  #define ICMP_ECHO_TIMEOUT	60 /* s, timeout for ICMP socket activity */
>  #define ICMP_NUM_IDS		(1U << 16)
> @@ -58,7 +59,12 @@ static struct icmp_ping_flow *ping_at_sidx(flow_sidx_t sidx)
>  	if (!flow)
>  		return NULL;
>  
> +#ifdef FUZZING
> +	if (flow->f.type != FLOW_PING4 && flow->f.type != FLOW_PING6)
> +		return NULL;
> +#else
>  	assert(flow->f.type == FLOW_PING4 || flow->f.type == FLOW_PING6);
> +#endif
>  	return &flow->ping;
>  }
>  
> @@ -72,7 +78,13 @@ void icmp_sock_handler(const struct ctx *c, union epoll_ref ref,
>  		       const struct timespec *now)
>  {
>  	struct icmp_ping_flow *pingf = ping_at_sidx(ref.flowside);
> -	const struct flowside *ini = &pingf->f.side[INISIDE];
> +	const struct flowside *ini;
> +
> +#ifdef FUZZING
> +	if (!pingf)
> +		return;
> +#endif
> +	ini = &pingf->f.side[INISIDE];
>  	union sockaddr_inany sr;
>  	socklen_t sl = sizeof(sr);
>  	char buf[USHRT_MAX];
> diff --git a/tcp.c b/tcp.c
> index 3b78d2e..612c884 100644
> --- a/tcp.c
> +++ b/tcp.c
> @@ -316,6 +316,7 @@
>  #include "tcp_buf.h"
>  #include "tcp_vu.h"
>  #include "epoll_ctl.h"
> +#include "fuzz.h"
>  
>  /*
>   * The size of TCP header (including options) is given by doff (Data Offset)
> @@ -456,7 +457,12 @@ static struct tcp_tap_conn *conn_at_sidx(flow_sidx_t sidx)
>  	if (!flow)
>  		return NULL;
>  
> +#ifdef FUZZING
> +	if (flow->f.type != FLOW_TCP)
> +		return NULL;
> +#else
>  	assert(flow->f.type == FLOW_TCP);
> +#endif
>  	return &flow->tcp;
>  }
>  
> @@ -2681,7 +2687,14 @@ void tcp_timer_handler(const struct ctx *c, union epoll_ref ref,
>  		       const struct timespec *now)
>  {
>  	struct itimerspec check_armed = { { 0 }, { 0 } };
> -	struct tcp_tap_conn *conn = &FLOW(ref.flow)->tcp;
> +	struct tcp_tap_conn *conn;
> +
> +#ifdef FUZZING
> +	if (ref.flow >= FLOW_MAX ||
> +	    FLOW(ref.flow)->f.type != FLOW_TCP)
> +		return;
> +#endif
> +	conn = &FLOW(ref.flow)->tcp;
>  
>  	assert(!c->no_tcp);
>  	assert(conn->f.type == FLOW_TCP);
> @@ -2752,6 +2765,10 @@ void tcp_sock_handler(const struct ctx *c, union epoll_ref ref,
>  {
>  	struct tcp_tap_conn *conn = conn_at_sidx(ref.flowside);
>  
> +#ifdef FUZZING
> +	if (!conn)
> +		return;
> +#endif
>  	assert(!c->no_tcp);
>  	assert(pif_at_sidx(ref.flowside) != PIF_TAP);
>  
> diff --git a/tcp_splice.c b/tcp_splice.c
> index 4b01f1a..005ecd1 100644
> --- a/tcp_splice.c
> +++ b/tcp_splice.c
> @@ -105,7 +105,12 @@ static struct tcp_splice_conn *conn_at_sidx(flow_sidx_t sidx)
>  	if (!flow)
>  		return NULL;
>  
> +#ifdef FUZZING
> +	if (flow->f.type != FLOW_TCP_SPLICE)
> +		return NULL;
> +#else
>  	assert(flow->f.type == FLOW_TCP_SPLICE);
> +#endif
>  	return &flow->tcp_splice;
>  }
>  
> @@ -594,6 +599,11 @@ void tcp_splice_sock_handler(struct ctx *c, union epoll_ref ref,
>  	struct tcp_splice_conn *conn = conn_at_sidx(ref.flowside);
>  	unsigned evsidei = ref.flowside.sidei;
>  
> +#ifdef FUZZING
> +	if (!conn)
> +		return;
> +#endif
> +
>  	assert(conn->f.type == FLOW_TCP_SPLICE);
>  
>  	if (conn->events == SPLICE_CLOSED)
> diff --git a/udp.c b/udp.c
> index 505e554..9431353 100644
> --- a/udp.c
> +++ b/udp.c
> @@ -118,6 +118,7 @@
>  #include "udp_internal.h"
>  #include "udp_vu.h"
>  #include "epoll_ctl.h"
> +#include "fuzz.h"
>  
>  #define UDP_MAX_FRAMES		32  /* max # of frames to receive at once */
>  
> @@ -807,9 +808,15 @@ static void udp_sock_to_sock(const struct ctx *c, int from_s, int n,
>  	const struct flowside *toside = flowside_at_sidx(tosidx);
>  	const struct udp_flow *uflow = udp_at_sidx(tosidx);
>  	uint8_t topif = pif_at_sidx(tosidx);
> -	int to_s = uflow->s[tosidx.sidei];
> +	int to_s;
>  	int i;
>  
> +#ifdef FUZZING
> +	if (!uflow)
> +		return;
> +#endif
> +	to_s = uflow->s[tosidx.sidei];
> +
>  	if ((n = udp_sock_recv(c, from_s, udp_mh_recv, n)) <= 0)
>  		return;
>  
> @@ -836,9 +843,15 @@ static void udp_buf_sock_to_tap(const struct ctx *c, int s, int n,
>  {
>  	const struct flowside *toside = flowside_at_sidx(tosidx);
>  	struct udp_flow *uflow = udp_at_sidx(tosidx);
> -	uint8_t *omac = uflow->f.tap_omac;
> +	uint8_t *omac;
>  	int i;
>  
> +#ifdef FUZZING
> +	if (!uflow)
> +		return;
> +#endif
> +	omac = uflow->f.tap_omac;
> +
>  	if ((n = udp_sock_recv(c, s, udp_mh_recv, n)) <= 0)
>  		return;
>  
> @@ -901,10 +914,18 @@ void udp_sock_fwd(const struct ctx *c, int s, int rule_hint,
>  		} else if (flow_sidx_valid(tosidx)) {
>  			struct udp_flow *uflow = udp_at_sidx(tosidx);
>  
> +#ifdef FUZZING
> +			if (!uflow) {
> +				discard = true;
> +				continue;
> +			}
> +#endif
> +
>  			flow_err_ratelimit(
>  				uflow, now,
>  				"No support for forwarding UDP from %s to %s",
>  				pif_name(frompif), pif_name(topif));
> +
>  			discard = true;
>  		} else {
>  			warn_ratelimit(now, "Discarding datagram without flow");
> @@ -949,6 +970,10 @@ void udp_sock_handler(const struct ctx *c, union epoll_ref ref,
>  {
>  	struct udp_flow *uflow = udp_at_sidx(ref.flowside);
>  
> +#ifdef FUZZING
> +	if (!uflow)
> +		return;
> +#endif
>  	assert(!c->no_udp && uflow);
>  
>  	if (events & EPOLLERR) {
> diff --git a/udp_flow.c b/udp_flow.c
> index f59649f..6c5b010 100644
> --- a/udp_flow.c
> +++ b/udp_flow.c
> @@ -31,7 +31,12 @@ struct udp_flow *udp_at_sidx(flow_sidx_t sidx)
>  	if (!flow)
>  		return NULL;
>  
> +#ifdef FUZZING
> +	if (flow->f.type != FLOW_UDP)
> +		return NULL;
> +#else
>  	assert(flow->f.type == FLOW_UDP);
> +#endif
>  	return &flow->udp;
>  }
>  
> -- 
> 2.55.0
> 

-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]