linux-kernel - Re: [PATCH v2] i2c: iproc: fix race between client unreg and isr

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:   Fri,  7 Aug 2020 20:55:25 -0700
From:   Dhananjay Phadke <dphadke@...ux.microsoft.com>
To:     f.fainelli@...il.com
Cc:     bcm-kernel-feedback-list@...adcom.com, dphadke@...ux.microsoft.com,
        linux-i2c@...r.kernel.org, linux-kernel@...r.kernel.org,
        rayagonda.kokatanur@...adcom.com, rjui@...adcom.com, wsa@...nel.org
Subject: Re: [PATCH v2] i2c: iproc: fix race between client unreg and isr

On 8/7/2020, Florian Fainelli wrote:
> > When i2c client unregisters, synchronize irq before setting
> > iproc_i2c->slave to NULL.
> > 
> > (1) disable_irq()
> > (2) Mask event enable bits in control reg
> > (3) Erase slave address (avoid further writes to rx fifo)
> > (4) Flush tx and rx FIFOs
> > (5) Clear pending event (interrupt) bits in status reg
> > (6) enable_irq()
> > (7) Set client pointer to NULL
> > 
> 
> > @@ -1091,6 +1091,17 @@ static int bcm_iproc_i2c_unreg_slave(struct i2c_client *slave)
> >  	tmp &= ~BIT(S_CFG_EN_NIC_SMB_ADDR3_SHIFT);
> >  	iproc_i2c_wr_reg(iproc_i2c, S_CFG_SMBUS_ADDR_OFFSET, tmp);
> >  
> > +	/* flush TX/RX FIFOs */
> > +	tmp = (BIT(S_FIFO_RX_FLUSH_SHIFT) | BIT(S_FIFO_TX_FLUSH_SHIFT));
> > +	iproc_i2c_wr_reg(iproc_i2c, S_FIFO_CTRL_OFFSET, tmp);
> > +
> > +	/* clear all pending slave interrupts */
> > +	iproc_i2c_wr_reg(iproc_i2c, IS_OFFSET, ISR_MASK_SLAVE);
> > +
> > +	enable_irq(iproc_i2c->irq);
> > +
> > +	iproc_i2c->slave = NULL;
> 
> There is nothing that checks on iproc_i2c->slave being valid within the
> interrupt handler, we assume that the pointer is valid which is fin,
> however non functional it may be, it may feel more natural to move the
> assignment before the enable_irq()?

As far as the teardown sequence ensures no more interrupts arrive after
enable_irq() and they are enabled only after setting pointer during
client register(); checking for NULL in ISR isn't necessary. 

If The teardown sequence doesn't guarantee quiescing of interrupts,
setting NULL before or after enable_irq() is equally vulnerable.

Dhananjay