netdev - Re: [PATCH net-next v4 2/5] ipv6: ioam: Data plane support for Pre-allocated Trace

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 31 May 2021 13:43:37 +0200 (CEST)
From:   Justin Iurman <justin.iurman@...ege.be>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     netdev@...r.kernel.org, davem@...emloft.net, tom@...bertland.com
Subject: Re: [PATCH net-next v4 2/5] ipv6: ioam: Data plane support for
 Pre-allocated Trace

>> >> A per-interface sysctl ioam6_enabled is provided to process/ignore IOAM
>> >> headers. Default is ignore (= disabled). Another per-interface sysctl
>> >> ioam6_id is provided to define the IOAM (unique) identifier of the
>> >> interface. Default is 0. A per-namespace sysctl ioam6_id is provided to
>> >> define the IOAM (unique) identifier of the node. Default is 0.
>> > 
>> > Last two sentences are repeated.
>> 
>> One describes net.ipv6.conf.XXX.ioam6_id (per interface) and the
>> other describes net.ipv6.ioam6_id (per namespace). It allows for
>> defining an IOAM id to an interface and, also, the node in general.
> 
> I see it now, please rephrase.

Will do.

> 
>> >> @@ -929,6 +932,50 @@ static bool ipv6_hop_ra(struct sk_buff *skb, int optoff)
>> >>  	return false;
>> >>  }
>> >>  
>> >> +/* IOAM */
>> >> +
>> >> +static bool ipv6_hop_ioam(struct sk_buff *skb, int optoff)
>> >> +{
>> >> +	struct ioam6_trace_hdr *trace;
>> >> +	struct ioam6_namespace *ns;
>> >> +	struct ioam6_hdr *hdr;
>> >> +
>> >> +	/* Must be 4n-aligned */
>> >> +	if (optoff & 3)
>> >> +		goto drop;
>> >> +
>> >> +	/* Ignore if IOAM is not enabled on ingress */
>> >> +	if (!__in6_dev_get(skb->dev)->cnf.ioam6_enabled)
>> >> +		goto ignore;
>> >> +
>> >> +	hdr = (struct ioam6_hdr *)(skb_network_header(skb) + optoff);
>> >> +
>> >> +	switch (hdr->type) {
>> >> +	case IOAM6_TYPE_PREALLOC:
>> >> +		trace = (struct ioam6_trace_hdr *)((u8 *)hdr + sizeof(*hdr));
>> >> +		ns = ioam6_namespace(ipv6_skb_net(skb), trace->namespace_id);
>> > 
>> > Shouldn't there be validation that the header is not truncated or
>> > malformed before we start poking into the fields?
>> 
>> ioam6_fill_trace_data is responsible (right after that) for checking
>> the header and making sure the whole thing makes sense before
>> inserting data. But, first, we need to parse the IOAM-Namespace ID to
>> check if it is a known (defined) one or not, and therefore either
>> going deeper or ignoring the option. Anyway, maybe I could add a
>> check on hdr->opt_len and make sure it has at least the length of the
>> required header (what comes after is data).
> 
> Right, don't we also need to check hdr->opt_len vs trace->remlen?

Indeed, I'll add a check for both.

> 
> BTW the ASCII art in patch 1 looks like node data is filled in in order

I agree, this one could be quite confusing without the related paragraph in the draft that explains it. Two possibilities here: (a) add the paragraph in the patch description to remove ambiguity; or (b) revert indexes in the ASCII art (from n to 0). Thoughts?

> but:
> 
> +	data = trace->data + trace->remlen * 4 - trace->nodelen * 4 - sclen * 4;
> 
> Looks like we'd start from the last node data?

Correct, it works as a stack from bottom (end of the pre-allocated space) to top (start of the pre-allocated space).