netdev - Re: [PATCH v2 net] inet: inet_defrag: prevent sk release while still in use

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: <CANn89iJEhQvo0tz2jfGDKijHSrxKHPPr622CU3QsnqeF6aFoiQ@mail.gmail.com>
Date: Wed, 27 Mar 2024 15:30:44 +0100
From: Eric Dumazet <edumazet@...gle.com>
To: Florian Westphal <fw@...len.de>
Cc: netdev@...r.kernel.org, dsahern@...nel.org, kuba@...nel.org, 
	pabeni@...hat.com, netfilter-devel@...r.kernel.org, 
	xingwei lee <xrivendell7@...il.com>, yue sun <samsun1006219@...il.com>, 
	syzbot+e5167d7144a62715044c@...kaller.appspotmail.com
Subject: Re: [PATCH v2 net] inet: inet_defrag: prevent sk release while still
 in use

On Tue, Mar 26, 2024 at 11:19 AM Florian Westphal <fw@...len.de> wrote:
>
> ip_local_out() and other functions can pass skb->sk as function argument.
>
> If the skb is a fragment and reassembly happens before such function call
> returns, the sk must not be released.
>
> This affects skb fragments reassembled via netfilter or similar
> modules, e.g. openvswitch or ct_act.c, when run as part of tx pipeline.
>
> Eric Dumazet made an initial analysis of this bug.  Quoting Eric:
>   Calling ip_defrag() in output path is also implying skb_orphan(),
>   which is buggy because output path relies on sk not disappearing.
>
>   A relevant old patch about the issue was :
>   8282f27449bf ("inet: frag: Always orphan skbs inside ip_defrag()")
>
>   [..]
>
>   net/ipv4/ip_output.c depends on skb->sk being set, and probably to an
>   inet socket, not an arbitrary one.
>
>   If we orphan the packet in ipvlan, then downstream things like FQ
>   packet scheduler will not work properly.
>
>   We need to change ip_defrag() to only use skb_orphan() when really
>   needed, ie whenever frag_list is going to be used.
>
> Eric suggested to stash sk in fragment queue and made an initial patch.
> However there is a problem with this:
>
> If skb is refragmented again right after, ip_do_fragment() will copy
> head->sk to the new fragments, and sets up destructor to sock_wfree.
> IOW, we have no choice but to fix up sk_wmem accouting to reflect the
> fully reassembled skb, else wmem will underflow.
>
> This change moves the orphan down into the core, to last possible moment.
> As ip_defrag_offset is aliased with sk_buff->sk member, we must move the
> offset into the FRAG_CB, else skb->sk gets clobbered.
>
> This allows to delay the orphaning long enough to learn if the skb has
> to be queued or if the skb is completing the reasm queue.
>
> In the former case, things work as before, skb is orphaned.  This is
> safe because skb gets queued/stolen and won't continue past reasm engine.
>
> In the latter case, we will steal the skb->sk reference, reattach it to
> the head skb, and fix up wmem accouting when inet_frag inflates truesize.
>
> Fixes: 7026b1ddb6b8 ("netfilter: Pass socket pointer down through okfn().")
> Diagnosed-by: Eric Dumazet <edumazet@...gle.com>
> Reported-by: xingwei lee <xrivendell7@...il.com>
> Reported-by: yue sun <samsun1006219@...il.com>
> Reported-by: syzbot+e5167d7144a62715044c@...kaller.appspotmail.com
> Signed-off-by: Florian Westphal <fw@...len.de>

Thanks a lot Florian !

Reviewed-by: Eric Dumazet <edumazet@...gle.com>