netdev - Re: [PATCH v1 net] net: dsa: microchip: fix wrong register write when masking interrupt

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] [day] [month] [year] [list]

Date: Thu, 20 Jun 2024 12:40:50 +0300
From: Vladimir Oltean <olteanv@...il.com>
To: Tristram.Ha@...rochip.com
Cc: Arun.Ramadoss@...rochip.com, Woojung.Huh@...rochip.com, andrew@...n.ch,
	vivien.didelot@...il.com, f.fainelli@...il.com, davem@...emloft.net,
	edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com,
	UNGLinuxDriver@...rochip.com, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 net] net: dsa: microchip: fix wrong register write
 when masking interrupt

On Tue, Jun 18, 2024 at 11:55:22PM +0000, Tristram.Ha@...rochip.com wrote:
> > Subject: Re: [PATCH v1 net] net: dsa: microchip: fix wrong register write when
> > masking interrupt
> > 
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content
> > is safe
> > 
> > On Thu, May 30, 2024 at 06:39:13PM -0700, Tristram.Ha@...rochip.com wrote:
> > > From: Tristram Ha <tristram.ha@...rochip.com>
> > >
> > > Initially the macro REG_SW_PORT_INT_MASK__4 is defined as 0x001C in
> > > ksz9477_reg.h and REG_PORT_INT_MASK is defined as 0x#01F.  Because the
> > > global and port interrupt handling is about the same the new
> > > REG_SW_PORT_INT_MASK__1 is defined as 0x1F in ksz_common.h.  This works
> > > as only the least significant bits have effect.  As a result the 32-bit
> > > write needs to be changed to 8-bit.
> > >
> > > Fixes: e1add7dd6183 ("net: dsa: microchip: use common irq routines for girq and pirq")
> > > Signed-off-by: Tristram Ha <tristram.ha@...rochip.com>
> > > ---
> > > v1
> > >  - clarify the reason to change the code
> > 
> > After v1 comes v2.
> 
> I thought the initial version starts at index 0?

Nope - you can cross-check with other posts on the list. Both RFCs and
normal patch submissions on the same topic start from v1 (either
explicitly specified or not) and increment for each submitted version,
regardless of whether RFC or not (the first proper submission after 3
RFC iterations is a v4).

> > >
> > >  drivers/net/dsa/microchip/ksz_common.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/net/dsa/microchip/ksz_common.c
> > b/drivers/net/dsa/microchip/ksz_common.c
> > > index 1e0085cd9a9a..3ad0879b00cd 100644
> > > --- a/drivers/net/dsa/microchip/ksz_common.c
> > > +++ b/drivers/net/dsa/microchip/ksz_common.c
> > > @@ -2185,7 +2185,7 @@ static void ksz_irq_bus_sync_unlock(struct irq_data *d)
> > >       struct ksz_device *dev = kirq->dev;
> > >       int ret;
> > >
> > > -     ret = ksz_write32(dev, kirq->reg_mask, kirq->masked);
> > > +     ret = ksz_write8(dev, kirq->reg_mask, kirq->masked);
> > >       if (ret)
> > >               dev_err(dev->dev, "failed to change IRQ mask\n");
> > >
> > > --
> > > 2.34.1
> > >
> > 
> > What is the user-visible functional impact of the 32-bit access? Justify
> > why this is a bug worth sending to stable kernels please.
> > 
> > FWIW, struct ksz_irq operates on 16-bit registers.
> 
> As explained before the initial code uses register 0x1C but now it is
> changed to 0x1F.
> 
> See the real operating code if not modified:
> 
> ret = ksz_write32(dev, 0x1F, 0x0000007F);
> 
> The original code looks like this:
> 
> ret = ksz_write32(dev, 0x1C, 0x0000007F);
> 
> BTW, all other KSZ switches except KSZ9897/KSZ9893 and LAN937X families
> use only 8-bit access.
> 

So you're saying that the original code made a 32-bit access to address
0x1c (aka byte-wise accesses to 0x1c, 0x1d, 0x1e, 0x1f) and then the
blamed commit changed the address of the 32-bit access to 0x1f (aka
misaligned byte-wise accesses to 0x1f, 0x20, 0x21, 0x22), leading to a
failure to mask the correct IRQ? And that only the byte-wise access to
0x1f is actually required, so instead of restoring the 32-bit access to
address 0x1c as would be expected from a trivial fix, we can just
perform an 8-bit write to 0x1f directly?

Sorry, but if this is the case, it isn't clear at all from the commit
message and later explanations, at least it isn't to me.