linux-kernel - Re: Query regarding "firmware: arm_scmi: Free mailbox channels if probe fails"

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 30 Sep 2022 18:29:02 +0530
From:   Shivnandan Kumar <quic_kshivnan@...cinc.com>
To:     Cristian Marussi <cristian.marussi@....com>
CC:     <sudeep.holla@....com>, <linux-arm-kernel@...ts.infradead.org>,
        <linux-kernel@...r.kernel.org>, <quic_rgottimu@...cinc.com>,
        <quic_avajid@...cinc.com>
Subject: Re: Query regarding "firmware: arm_scmi: Free mailbox channels if
 probe fails"

hi Cristian,

Thanks for your support in providing the patch to try.

I found one race condition in our downstream mbox controller driver 
while accessing con_priv, when I serialized access to this, issue is not 
seen on 3 days of testing.

As you rightly mentioned that your provided patch will impact all the 
other users.

Also if  we take your provided patch, same race still exists while 
accessing con_priv in our downstream mbox controller so this issue will 
still be there.

So, we are planning to merge the patch( serialized access to con_priv) 
in our downstream mbox controller now.


Thanks,

Shivnandan


On 9/22/2022 8:58 PM, Cristian Marussi wrote:
> On Thu, Sep 22, 2022 at 03:54:31PM +0100, Cristian Marussi wrote:
>> On Thu, Sep 22, 2022 at 10:31:47AM +0530, Shivnandan Kumar wrote:
>>> Hi Cristian,
>>>
> Hi Shivnandan,
>   
> [snip]
>
>> Looking at the transport layer that you use, mailbox, I see that while
>> setup/free helpers are synchronized on an internal chan->lock, the RX
>> path inside the mailbox core is not, so I tried this:
>>
>>
>> diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
>> index 4229b9b5da98..bb6173c0ad54 100644
>> --- a/drivers/mailbox/mailbox.c
>> +++ b/drivers/mailbox/mailbox.c
>> @@ -157,9 +157,13 @@ static enum hrtimer_restart txdone_hrtimer(struct hrtimer *hrtimer)
>>    */
>>   void mbox_chan_received_data(struct mbox_chan *chan, void *mssg)
>>   {
>> +       unsigned long flags;
>> +
>> +       spin_lock_irqsave(&chan->lock, flags);
>>          /* No buffering the received data */
>>          if (chan->cl->rx_callback)
>>                  chan->cl->rx_callback(chan->cl, mssg);
>> +       spin_unlock_irqrestore(&chan->lock, flags);
>>   }
>>   EXPORT_SYMBOL_GPL(mbox_chan_received_data);
>>   
> ...sorry... a small change on the tentative above fix...
>
> ----8<------
>
> diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
> index 4229b9b5da98..6fbe183acdae 100644
> --- a/drivers/mailbox/mailbox.c
> +++ b/drivers/mailbox/mailbox.c
> @@ -157,9 +157,13 @@ static enum hrtimer_restart txdone_hrtimer(struct hrtimer *hrtimer)
>    */
>   void mbox_chan_received_data(struct mbox_chan *chan, void *mssg)
>   {
> +       unsigned long flags;
> +
> +       spin_lock_irqsave(&chan->lock, flags);
>          /* No buffering the received data */
> -       if (chan->cl->rx_callback)
> +       if (chan->cl && chan->cl->rx_callback)
>                  chan->cl->rx_callback(chan->cl, mssg);
> +       spin_unlock_irqrestore(&chan->lock, flags);
>   }
>   EXPORT_SYMBOL_GPL(mbox_chan_received_data);
>
> ------8<-----
>
> Thanks,
> Cristian
>