netdev - Re: [PATCH net v2] datagram: When peeking datagrams with offset

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: <CAF=yD-L5DVQ-pDuk+B_CWkmQsZ8+L04SppkmCO0QahaiNWAaWw@mail.gmail.com>
Date:   Fri, 18 Aug 2017 13:52:03 -0400
From:   Willem de Bruijn <willemdebruijn.kernel@...il.com>
To:     Paolo Abeni <pabeni@...hat.com>
Cc:     Matthew Dawson <matthew@...systems.ca>,
        Network Development <netdev@...r.kernel.org>,
        "Macieira, Thiago" <thiago.macieira@...el.com>
Subject: Re: [PATCH net v2] datagram: When peeking datagrams with offset < 0
 don't skip empty skbs

On Fri, Aug 18, 2017 at 1:42 PM, Paolo Abeni <pabeni@...hat.com> wrote:
> On Fri, 2017-08-18 at 12:39 -0400, Matthew Dawson wrote:
>> On Friday, August 18, 2017 10:05:18 AM EDT Paolo Abeni wrote:
>> > On Thu, 2017-08-17 at 22:11 -0400, Matthew Dawson wrote:
>> > > diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
>> > > index 7b52a380d710..be8982b4f8c0 100644
>> > > --- a/net/unix/af_unix.c
>> > > +++ b/net/unix/af_unix.c
>> > > @@ -2304,10 +2304,7 @@ static int unix_stream_read_generic(struct
>> > > unix_stream_read_state *state,>
>> > >    */
>> > >
>> > >   mutex_lock(&u->iolock);
>> > >
>> > > - if (flags & MSG_PEEK)
>> > > -         skip = sk_peek_offset(sk, flags);
>> > > - else
>> > > -         skip = 0;
>> > > + skip = max(sk_peek_offset(sk, flags), 0);
>> > >
>> > >   do {
>> > >
>> > >           int chunk;
>> >
>> > later we have:
>> >
>> >     chunk = min_t(unsigned int, unix_skb_len(skb) - skip, size);
>> >
>> > without any call to __skb_try_recv_from_queue(), so we will get
>> > bad/unexpected values from the above assignment when 'skip' is
>> > negative.
>>
>> The assignment to skip should ensure it is never less then zero, thanks to the
>> max(sk...(), 0).  Thus that shouldn't be an issue?
>
> Right, I missed the max() call. Thanks for pointing it out.
> I'm fine with the above.
>
>> >
>> > Overall I still think that adding/using an explicit MSG_PEEK_OFF bit
>> > would produce a simpler code, but is just a personal preference.
>>
>> I don't mind either way, that just seemed to be the preference I saw from the
>> discussion around the patch.  I think either way will work, so whatever the
>> list prefers I'm happy with.
>
> I'm ok either way. Probably it's worth continue this way.

I don't think anyone cares too much, as long as this is fixed.

I have a slight subjective preference for directly passing the sk_peek_off
variable and relying on the negative value as signal for whether the
SO_PEEK_OFF mode is enabled or not, simply because that signal
already exists and we avoid an intermediate conversion step.

That said, I have a follow-on bug fix for __sk_queue_drop_skb where
that signal is not available, but the flags argument is. I believe that
we will need to take the same action in both cases, so that this is moot.
Just mentioning it in case that would sway opinion the other way.

As said, I'm fine with both. Will Ack this.