netdev - Re: [PATCH rfc v0 2/9] net: dsa: qca8k: Move completion into DSA core

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 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:   Wed, 21 Sep 2022 00:22:54 +0000
From:   Vladimir Oltean <vladimir.oltean@....com>
To:     Andrew Lunn <andrew@...n.ch>
CC:     "mattias.forsblad@...il.com" <mattias.forsblad@...il.com>,
        netdev <netdev@...r.kernel.org>,
        Florian Fainelli <f.fainelli@...il.com>,
        Christian Marangi <ansuelsmth@...il.com>
Subject: Re: [PATCH rfc v0 2/9] net: dsa: qca8k: Move completion into DSA core

On Wed, Sep 21, 2022 at 02:19:54AM +0200, Andrew Lunn wrote:
> I've been thinking about this a bit. And i think the bug is in the old
> code.
> 
> As soon as we call reinit_completion(), a late arriving packet can
> trigger the completion. Note that the sequence number has not been
> incremented yet. So that late arriving packet can pass the sequence
> number test, and the results will be copied and complete() called.
> 
> qca8k_read_eth() can continue, increment the sequence number, call
> wait_for_completion_timeout() and immediately exit, returning the
> contents of the late arriving reply.
> 
> To make this safe:
> 
> 1) The sequence number needs to be incremented before
>    reinit_completion(). That closes one race
> 
> 2) If the sequence numbers don't match, silently drop the packet, it
>    is either later, or bogus. Hopefully the correct reply packet will
>    come along soon and trigger the completion.
> 
> I've also got some of this wrong in my code.

Wouldn't the programming be more obvious if we didn't try to reuse the
same dsa_inband structure for every request/response, but simply
allocate/free on demand and use only once?