netdev - Re: [PATCH bpf v2] af_unix: Disable MSG_OOB handling for sockets in sockmap/sockhash

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] [day] [month] [year] [list]

Message-ID: <87msmqn9ws.fsf@cloudflare.com>
Date: Tue, 09 Jul 2024 12:21:07 +0200
From: Jakub Sitnicki <jakub@...udflare.com>
To: Michal Luczaj <mhal@...x.co>
Cc: netdev@...r.kernel.org,  bpf@...r.kernel.org,  davem@...emloft.net,
  edumazet@...gle.com,  kuba@...nel.org,  pabeni@...hat.com,
  john.fastabend@...il.com,  kuniyu@...zon.com,  Rao.Shoaib@...cle.com,
  Cong Wang <cong.wang@...edance.com>
Subject: Re: [PATCH bpf v2] af_unix: Disable MSG_OOB handling for sockets in
 sockmap/sockhash

On Mon, Jul 08, 2024 at 01:10 AM +02, Michal Luczaj wrote:
> On 6/27/24 09:40, Jakub Sitnicki wrote:
>> On Wed, Jun 26, 2024 at 12:19 PM +02, Michal Luczaj wrote:
>>> On 6/24/24 16:15, Jakub Sitnicki wrote:
>>>> On Sun, Jun 23, 2024 at 12:25 AM +02, Michal Luczaj wrote:
>>>>> AF_UNIX socket tracks the most recent OOB packet (in its receive queue)
>>>>> with an `oob_skb` pointer. BPF redirecting does not account for that: when
>>>>> an OOB packet is moved between sockets, `oob_skb` is left outdated. This
>>>>> results in a single skb that may be accessed from two different sockets.
>>>>>
>>>>> Take the easy way out: silently drop MSG_OOB data targeting any socket that
>>>>> is in a sockmap or a sockhash. Note that such silent drop is akin to the
>>>>> fate of redirected skb's scm_fp_list (SCM_RIGHTS, SCM_CREDENTIALS).
>>>>>
>>>>> For symmetry, forbid MSG_OOB in unix_bpf_recvmsg().
>>>>>
>>>>> Suggested-by: Kuniyuki Iwashima <kuniyu@...zon.com>
>>>>> Fixes: 314001f0bf92 ("af_unix: Add OOB support")
>>>>> Signed-off-by: Michal Luczaj <mhal@...x.co>
>>>>> ---

[...]

>>> And regarding Rao's comment, I took a look and I think sockmap'ed TCP OOB
>>> does indeed act the same way. I'll try to add that into selftest as well.n
>> 
>> Right, it does sound like we're not clearing the offset kept in
>> tcp_sock::urg_data when skb is redirected.
>
> Yeah, so I also wanted to extend the TCP's redir_to_connected(), but is
> that code correct? It seems to be testing REDIR_INGRESS, yet
> prog_stream_verdict() doesn't run bpf_sk_redirect_map() with the
> BPF_F_INGRESS flag.

Right, we don't have the ingress-to-local [1] setup covered there.

This test case has outgrown its initial coverage purpose - using sockmap
with listening / unconnected sockets - and needs to be split up, IMO.

There are definitely gaps in the coverage of redirect cases, and I'd
like to extend it to cover all combinations (supported and unsupported
ones) [2].

[1] It's a term I coined to make it easier to communiate
    https://github.com/jsitnicki/kubecon-2024-sockmap/blob/main/cheatsheet-sockmap-redirect.png
[2] https://gist.github.com/jsitnicki/578fdd614d181bed2b02922b17972b4e