netdev - Re: [PATCH net v2 3/3] net: ipv6: fix consecutive input and output transformation in lwtunnels

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]

Message-ID: <e5d30bbd-4dfc-4a97-9eaf-39e0dfe51826@uliege.be>
Date: Thu, 13 Feb 2025 23:57:57 +0100
From: Justin Iurman <justin.iurman@...ege.be>
To: Paolo Abeni <pabeni@...hat.com>, netdev@...r.kernel.org
Cc: davem@...emloft.net, dsahern@...nel.org, edumazet@...gle.com,
 kuba@...nel.org, horms@...nel.org
Subject: Re: [PATCH net v2 3/3] net: ipv6: fix consecutive input and output
 transformation in lwtunnels



On 2/13/25 15:33, Paolo Abeni wrote:
> On 2/11/25 11:16 PM, Justin Iurman wrote:
>> Some lwtunnel users implement both lwt input and output handlers. If the
>> post-transformation destination on input is the same, the output handler
>> is also called and the same transformation is applied (again). Here are
>> the users: ila, bpf, rpl, seg6. The first one (ila) does not need this
>> fix, since it already implements a check to avoid such a duplicate. The
>> second (bpf) may need this fix, but I'm not familiar with that code path
>> and will keep it out of this patch. The two others (rpl and seg6) do
>> need this patch.
>>
>> Due to the ila implementation (as an example), we cannot fix the issue
>> in lwtunnel_input() and lwtunnel_output() directly. Instead, we need to
>> do it on a case-by-case basis. This patch fixes both rpl_iptunnel and
>> seg6_iptunnel users. The fix re-uses skb->redirected in input handlers
>> to notify corresponding output handlers that the transformation was
>> already applied and to skip it. The "redirected" field seems safe to be
>> used here.
>>
>> Fixes: a7a29f9c361f ("net: ipv6: add rpl sr tunnel")
>> Fixes: 6c8702c60b88 ("ipv6: sr: add support for SRH encapsulation and injection with lwtunnels")
>> Signed-off-by: Justin Iurman <justin.iurman@...ege.be>
>> ---
>>   net/ipv6/rpl_iptunnel.c  | 14 ++++++++++++--
>>   net/ipv6/seg6_iptunnel.c | 16 +++++++++++++---
>>   2 files changed, 25 insertions(+), 5 deletions(-)
>>
>> diff --git a/net/ipv6/rpl_iptunnel.c b/net/ipv6/rpl_iptunnel.c
>> index dc004e9aa649..2dc1f2297e39 100644
>> --- a/net/ipv6/rpl_iptunnel.c
>> +++ b/net/ipv6/rpl_iptunnel.c
>> @@ -208,6 +208,12 @@ static int rpl_output(struct net *net, struct sock *sk, struct sk_buff *skb)
>>   	struct rpl_lwt *rlwt;
>>   	int err;
>>   
>> +	/* Don't re-apply the transformation when rpl_input() already did it */
>> +	if (skb_is_redirected(skb)) {
> 
> This check looks false-positive prone, i.e. if packet lands on an LWT
> tunnel due to an tc redirect from another non lwt device.

True, it was indeed a trade-off solution :-/

> On the flip side I don't see any good method to propagate the relevant
> information. A skb ext would work, but I would not call that a good method.

Agree :-( Did not check but maybe we could also look at 
skb->tc_at_ingress in that case? Not sure it'd help though. Or... any 
chance we could find a hole in sk_buff for a new :1 field?