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: Windows password security audit tool. GUI, reports in PDF.

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

Date: Thu, 27 Jun 2024 12:04:32 +0200
From: Paolo Abeni <pabeni@...hat.com>
To: Kuniyuki Iwashima <kuniyu@...zon.com>
Cc: Rao.Shoaib@...cle.com, davem@...emloft.net, edumazet@...gle.com, 
	kuba@...nel.org, kuni1840@...il.com, netdev@...r.kernel.org
Subject: Re: [PATCH v1 net 03/11] af_unix: Stop recv(MSG_PEEK) at consumed
 OOB skb.

On Wed, 2024-06-26 at 14:47 -0700, Kuniyuki Iwashima wrote:
> 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 

... excluding the other ~1200 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().

The last commit mentioned above actually sparked a bit of post merge
discussion, which is IMHO sub-optimal.

On the flip side, I think all the changes in this series make sense,
and the self-tests refactor and extension is largish, but very nice.

TL;DR: I'm going to apply this now.

Thanks,

Paolo