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:   Sat, 8 Aug 2020 07:47:16 -0700
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Dhananjay Phadke <dphadke@...ux.microsoft.com>
Cc:     bcm-kernel-feedback-list@...adcom.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 8:55 PM, Dhananjay Phadke wrote:
> 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. 

Agreed.

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

The teardown sequence is sort of a critical section if we may say, so
ensuring that everything happens within it and that enable_irq() is the
last operation would seem more natural to me at least. Thanks
-- 
Florian

Powered by blists - more mailing lists