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 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: <YypYqqwgllmcPJZj@lunn.ch>
Date:   Wed, 21 Sep 2022 02:19:54 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     Vladimir Oltean <vladimir.oltean@....com>
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 Tue, Sep 20, 2022 at 02:43:04PM +0000, Vladimir Oltean wrote:
> On Tue, Sep 20, 2022 at 12:18:46AM +0200, Andrew Lunn wrote:
> > @@ -248,8 +248,6 @@ static int qca8k_read_eth(struct qca8k_priv *priv, u32 reg, u32 *val, int len)
> >  
> >  	skb->dev = priv->mgmt_master;
> >  
> > -	reinit_completion(&mgmt_eth_data->rw_done);
> > -
> >  	/* Increment seq_num and set it in the mdio pkt */
> >  	mgmt_eth_data->seq++;
> >  	qca8k_mdio_header_fill_seq_num(skb, mgmt_eth_data->seq);
> > @@ -257,8 +255,8 @@ static int qca8k_read_eth(struct qca8k_priv *priv, u32 reg, u32 *val, int len)
> >  
> >  	dev_queue_xmit(skb);
> >  
> > -	ret = wait_for_completion_timeout(&mgmt_eth_data->rw_done,
> > -					  QCA8K_ETHERNET_TIMEOUT);
> > +	ret = dsa_inband_wait_for_completion(&mgmt_eth_data->inband,
> > +					     QCA8K_ETHERNET_TIMEOUT);
> >  
> >  	*val = mgmt_eth_data->data[0];
> >  	if (len > QCA_HDR_MGMT_DATA1_LEN)
> 
> Replacing the pattern above with this pattern:
> 
> int dsa_inband_wait_for_completion(struct dsa_inband *inband, int timeout_ms)
> {
> 	unsigned long jiffies = msecs_to_jiffies(timeout_ms);
> 
> 	reinit_completion(&inband->completion);
> 
> 	return wait_for_completion_timeout(&inband->completion, jiffies);
> }
> 
> is buggy because we reinitialize the completion later than the original
> code used to. We now call reinit_completion() from a code path that
> races with the handler that is supposed to call dsa_inband_complete().

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.

     Andrew