netdev - RE: [PATCH 1/2] ravb: Fix descriptor counters' conditions

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:   Tue, 27 Jul 2021 09:52:31 +0000
From:   Yoshihiro Shimoda <yoshihiro.shimoda.uh@...esas.com>
To:     Ulrich Hecht <uli@...nd.eu>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-renesas-soc@...r.kernel.org" 
        <linux-renesas-soc@...r.kernel.org>,
        "sergei.shtylyov@...il.com" <sergei.shtylyov@...il.com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "kuba@...nel.org" <kuba@...nel.org>
Subject: RE: [PATCH 1/2] ravb: Fix descriptor counters' conditions

Hi Ulrich-san,

> From: Ulrich Hecht, Sent: Tuesday, July 27, 2021 6:14 PM
> 
> > On 07/27/2021 10:55 AM Ulrich Hecht <uli@...nd.eu> wrote:
> >
> >
> > > On 07/27/2021 10:21 AM Yoshihiro Shimoda <yoshihiro.shimoda.uh@...esas.com> wrote:
> > >
> > >
> > > The descriptor counters ({cur,dirty}_[rt]x) acts as free counters
> > > so that conditions are possible to be incorrect when a left value
> > > was overflowed.
> > >
> > > So, for example, ravb_tx_free() could not free any descriptors
> > > because the following condition was checked as a signed value,
> > > and then "NETDEV WATCHDOG" happened:
> > >
> > >     for (; priv->cur_tx[q] - priv->dirty_tx[q] > 0; priv->dirty_tx[q]++) {
> > >
> > > To fix the issue, add get_num_desc() to calculate numbers of
> > > remaining descriptors.
> > >
> > > Fixes: c156633f1353 ("Renesas Ethernet AVB driver proper")
> > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@...esas.com>
> > > ---
> > >  drivers/net/ethernet/renesas/ravb_main.c | 22 +++++++++++++++-------
> > >  1 file changed, 15 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> > > index 805397088850..70fbac572036 100644
> > > --- a/drivers/net/ethernet/renesas/ravb_main.c
> > > +++ b/drivers/net/ethernet/renesas/ravb_main.c
> > > @@ -172,6 +172,14 @@ static const struct mdiobb_ops bb_ops = {
> > >  	.get_mdio_data = ravb_get_mdio_data,
> > >  };
> > >
> > > +static u32 get_num_desc(u32 from, u32 subtract)
> > > +{
> > > +	if (from >= subtract)
> > > +		return from - subtract;
> > > +
> > > +	return U32_MAX - subtract + 1 + from;
> > > +}
> >
> > This is a very roundabout way to implement an unsigned subtraction. :)

I agree :) However...

> > I think it would make more sense to simply return 0 if "subtract" is larger than "from".
> > (Likewise for sh_eth).
> 
> ...and the tests for "> 0" should be rewritten as "!= 0". Sorry, not fully awake yet.

such a change could not fix the issue, IIUC.

cur_tx   = 0x00000000
dirty_tx = 0xffffffff 

In that case, numbers of remaining descriptors is 1. So, the patch can return 1.
However, if the function return 0, this could not fix the issue because
the code could not run into the for statement.
---
+	for (; get_num_desc(priv->cur_tx[q], priv->dirty_tx[q]) != 0; priv->dirty_tx[q]++) {
 		bool txed;
 
 		entry = priv->dirty_tx[q] % (priv->num_tx_ring[q] *
---

I guess returning 1 instead is possible to be simple. But, the following condition requires
actual numbers of descriptors so that the current patch is better, I believe...
---
+	if (get_num_desc(priv->cur_tx[q], priv->dirty_tx[q]) >
+	    (priv->num_tx_ring[q] - 1) * num_tx_desc) {
 		netif_err(priv, tx_queued, ndev,
 			  "still transmitting with the full ring!\n");
 		netif_stop_subqueue(ndev, q);
---

Best regards,
Yoshihiro Shimoda