netdev - Re: [PATCH v1 net 03/11] af_unix: Stop recv(MSG

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 for Android: free password hash cracker in your pocket

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

Date: Wed, 26 Jun 2024 14:47:00 -0700
From: Kuniyuki Iwashima <kuniyu@...zon.com>
To: <pabeni@...hat.com>
CC: <Rao.Shoaib@...cle.com>, <davem@...emloft.net>, <edumazet@...gle.com>,
	<kuba@...nel.org>, <kuni1840@...il.com>, <kuniyu@...zon.com>,
	<netdev@...r.kernel.org>
Subject: Re: [PATCH v1 net 03/11] af_unix: Stop recv(MSG_PEEK) at consumed OOB skb.

From: Paolo Abeni <pabeni@...hat.com>
Date: Wed, 26 Jun 2024 23:10:40 +0200
> On Wed, 2024-06-26 at 18:56 +0200, Paolo Abeni wrote:
> > On Mon, 2024-06-24 at 18:36 -0700, Kuniyuki Iwashima wrote:
> > > After consuming OOB data, recv() reading the preceding data must break at
> > > the OOB skb regardless of MSG_PEEK.
> > > 
> > > Currently, MSG_PEEK does not stop recv() for AF_UNIX, and the behaviour is
> > > not compliant with TCP.
> > 
> > I'm unsure we can change the MSG_PEEK behavior at this point: existing
> > application(s?) could relay on that, regardless of how inconsistent
> > such behavior is.
> > 
> > I think we need at least an explicit ack from Rao, as the main known
> > user.
> 
> I see Rao stated that the unix OoB implementation was designed to mimic
> the tcp one:
> 
> https://lore.kernel.org/netdev/c5f6abbe-de43-48b8-856a-36ded227e94f@oracle.com/
> 
> so the series should be ok.
> 
> Still given the size of the changes and the behavior change I'm
> wondering if the series should target net-next instead.
> That would offer some time cushion to deal with eventual regression.
> WDYT?

The actual change is 37 LoC and we recently have this kind of changes
(30 LoC in total) in net.git.  The last two were merged in April and
we have no user report so far.

  a6736a0addd6 af_unix: Read with MSG_PEEK loops if the first unread byte is OOB
  22dd70eb2c3d af_unix: Don't peek OOB data without MSG_OOB.
  283454c8a123 af_unix: Call manage_oob() for every skb in unix_stream_read_generic().

Most of the changes are due to selftest.  The original test repeated the
same set of code but covered few cases.  OTOH, the new test spends most
of lines to add as many test cases as possible, which IMHO nicely covers
regression if we want to mimic TCP.

  On net.git:

  # FAILED: 20 / 38 tests passed.
  # Totals: pass:20 fail:18 xfail:0 xpass:0 skip:0 error:0

Also, now the original test is broken in stable after the commits above,
so I think it would be nice to have this series in stable.

Thanks!