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

Message-Id: <18cf439a-8fde-02b0-31b6-9ac42f7e972c@broadcom.com>
Date:   Mon, 10 Aug 2020 15:22:49 -0700
From:   Dhananjay Phadke <dphadke@...ux.microsoft.com>
To:     ray.jui@...adcom.com
Cc:     bcm-kernel-feedback-list@...adcom.com, dphadke@...ux.microsoft.com,
        f.fainelli@...il.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/10/2020 02:17 PM, Ray Jui wrote:
>> 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.
>> 
>
>Okay I think we all agree that this teardown sequence will guarantee
>that no further "slave" interrupts will be fired after it.
>
>>>
>>> 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
>> 
>
>I tend to agree with Florian here.
>
>1. Enable/Disable IRQ is done on the interrupt line for both master and
>slave (or even other peripherals that share the same interrupt line,
>although that is not the case here since this interrupt is dedicated to
>I2C in all iProc based SoCs).
>
>2. The tear down sequence here wrapped by disable/enable_irq is slave
>specific
>
>The effect of 1. is temporary, and the purpose of it is to ensure slave
>interrupts are quiesced properly at the end of the sequence.
>
>If we consider both 1. and 2., I agree with Florian that while the end
>result is the same, it is indeed more natural to wrap the entire slave
>tear down sequence within disable/enable irq.

Ok, will send v3 with this change.

Thanks,
Dhananjay