Re: [PATCH net] ipv6: ioam: fix heap buffer overflow in __ioam6_fill_trace_data()

[Justin Iurman <justin.iurman@gmail.com>]

On 2/10/26 04:25, Qanux wrote:
> The IPv6 IOAM Pre-allocated Trace receive path (ipv6_hop_ioam in
> net/ipv6/exthdrs.c) does not validate that the nodelen field in the
> trace header is consistent with the type bitmap.
> 
> When a crafted packet arrives with nodelen=0 but with type bits set,
> __ioam6_fill_trace_data() computes the write pointer at the end of
> the trace data buffer and then writes past it, overflowing into
> adjacent memory (skb_shared_info). This corrupts critical SKB metadata,
> leading to a kernel panic when the SKB is freed.
> 
> The send path properly validates the trace header via
> ioam6_validate_trace_hdr() which recomputes nodelen from the type
> bitmap. However, calling that function from the receive path would
> break forward compatibility, as it rejects packets with undefined
> type bits (bit 12-21) that the receive path intentionally accepts.
> 
> Fix by recomputing nodelen directly from the type bitmap inside
> ioam6_fill_trace_data() before using it, preserving the receive
> path's forward compatibility while preventing the overflow.

This is correct in theory. In practice, IOAM (just like SRv6, for 
example) is to be deployed in so-called limited domains, where 
ingress/egress filters MUST be configured. Therefore, an attacker from 
outside your domain is not supposed to be able to send crafted IOAM 
header/data.

That being said, I guess we could add a check in the data plane, as you 
suggested. The reason I did not add it in the first place was to avoid 
spending "useless" cycles for that. Note that you could theoretically be 
in a situation where the attacker is inside your domain (e.g., a 
compromised node). In that case, you would indeed have the 
aforementioned problem. However, if the attacker is already inside, then 
you're in much more trouble. If you're interested, we wrote this 
(hopefully, soon-to-be-RFC) document [1] to provide integrity protection 
of IOAM, for cases like that.

Some comments on the patch below.

  [1] https://datatracker.ietf.org/doc/draft-ietf-ippm-ioam-data-integrity/

> Fixes: 9ee11f0fff20 ("ipv6: ioam: Data plane support for Pre-allocated Trace")
> Cc: stable@vger.kernel.org
> Signed-off-by: Junxi Qian <qjx1298677004@gmail.com>
> ---
>   include/net/ioam6.h       |  4 ++++
>   net/ipv6/ioam6.c          | 10 +++++++++-
>   net/ipv6/ioam6_iptunnel.c |  3 ---
>   3 files changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/include/net/ioam6.h b/include/net/ioam6.h
> index abfa518cd917..a3f2a1e4fd19 100644
> --- a/include/net/ioam6.h
> +++ b/include/net/ioam6.h
> @@ -14,6 +14,10 @@
>   #include <linux/ioam6_genl.h>
>   #include <linux/rhashtable-types.h>
>   
> +#define IOAM6_MASK_SHORT_FIELDS 0xff100000
> +#define IOAM6_MASK_WIDE_FIELDS  0x00e00000
> +#define IOAM6_MASK_UNDEF_FIELDS 0x000ffc00
> +
>   struct ioam6_namespace {
>   	struct rhash_head head;
>   	struct rcu_head rcu;
> diff --git a/net/ipv6/ioam6.c b/net/ipv6/ioam6.c
> index 02aboriginal1..02bfixedcode 100644
> --- a/net/ipv6/ioam6.c
> +++ b/net/ipv6/ioam6.c
> @@ -929,12 +929,20 @@ void ioam6_fill_trace_data(struct sk_buff *skb,
>   			    bool is_input)
>   {
>   	struct ioam6_schema *sc;
> +	u32 fields;
>   	u8 sclen = 0;
>   
>   	/* Skip if Overflow flag is set
>   	 */
>   	if (trace->overflow)
>   		return;
>   
> +	trace->nodelen = 0;
> +	fields = be32_to_cpu(trace->type_be32);
> +	trace->nodelen += hweight32(fields & IOAM6_MASK_SHORT_FIELDS)
> +				* (sizeof(__be32) / 4);
> +	trace->nodelen += hweight32(fields & IOAM6_MASK_WIDE_FIELDS)
> +				* (sizeof(__be64) / 4);
> +	trace->nodelen += hweight32(fields & IOAM6_MASK_UNDEF_FIELDS)
> +				* (sizeof(__be32) / 4);
>   

Don't modify trace->nodelen. If trace->nodelen is incorrect, then we 
should just drop the packet. Also, please don't perform the checks here 
in the data plane, as it would unnecessarily penalize the encapsulating 
node (where checks were already performed once in the control plane). 
Move that inside ipv6_hop_ioam() instead.

While at it, let's pack the nodelen computation inside a new function, 
and call it from both situations. No need to add IOAM6_MASK_UNDEF_FIELDS 
for bits 12-21, they should be added to IOAM6_MASK_SHORT_FIELDS directly 
(from RFC9197: "Every future node data field corresponding to one of 
these bits MUST be 4 octets long"). For example (not tested):

In include/net/ioam6.h:

u8 ioam6_trace_compute_nodelen(u32 trace_type);

In net/ipv6/ioam6.c:

#define IOAM6_MASK_SHORT_FIELDS 0xff1ffc00
#define IOAM6_MASK_WIDE_FIELDS 0xe00000

u8 ioam6_trace_compute_nodelen(u32 trace_type)
{
	u8 nodelen = hweight32(trace_type & IOAM6_MASK_SHORT_FIELDS)
			* (sizeof(__be32) / 4);

	nodelen += hweight32(trace_type & IOAM6_MASK_WIDE_FIELDS)
			* (sizeof(__be64) / 4);

	return nodelen;
}

In net/ipv6/ioam6_iptunnel.c:

static bool ioam6_validate_trace_hdr(struct ioam6_trace_hdr *trace)
{
	u32 fields;

	if (!trace->type_be32 || !trace->remlen ||
	    trace->remlen > IOAM6_TRACE_DATA_SIZE_MAX / 4 ||
	    trace->type.bit12 | trace->type.bit13 | trace->type.bit14 |
	    trace->type.bit15 | trace->type.bit16 | trace->type.bit17 |
	    trace->type.bit18 | trace->type.bit19 | trace->type.bit20 |
	    trace->type.bit21 | trace->type.bit23)
		return false;

	fields = be32_to_cpu(trace->type_be32);
	trace->nodelen = ioam6_trace_compute_nodelen(fields);

	return true;
}

In net/ipv6/exthdrs.c:

static bool ipv6_hop_ioam(struct sk_buff *skb, int optoff)
{
	u32 trace_type;
[...]
	/* after the check for "Malformed Pre-allocated Trace header" */

	trace_type = be32_to_cpu(trace->type_be32);
	if (trace->nodelen != ioam6_trace_compute_nodelen(trace_type))
		goto drop;
[...]
}

Thanks,
Justin

>   	/* NodeLen does not include Opaque State Snapshot length. We need to
>   	 * take it into account if the corresponding bit is set (bit 22) and
> diff --git a/net/ipv6/ioam6_iptunnel.c b/net/ipv6/ioam6_iptunnel.c
> index 7abcdef12345..7abcfixedcode 100644
> --- a/net/ipv6/ioam6_iptunnel.c
> +++ b/net/ipv6/ioam6_iptunnel.c
> @@ -23,9 +23,6 @@
>   #include <net/ip6_route.h>
>   #include <net/addrconf.h>
>   
> -#define IOAM6_MASK_SHORT_FIELDS 0xff100000
> -#define IOAM6_MASK_WIDE_FIELDS  0x00e00000
> -
>   struct ioam6_lwt_encap {
>   	struct ipv6_hopopt_hdr eh;
>   	u8 pad[2];			/* 2-octet padding for 4n-alignment */