linux-kernel - Re: [PATCH 5.10 002/299] bus: mhi: core: Clear configuration from channel context during reset

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: <56b55b739177950f3c3fd3d3e74b8338@codeaurora.org>
Date:   Fri, 28 May 2021 09:59:37 -0700
From:   Bhaumik Bhatt <bbhatt@...eaurora.org>
To:     Pavel Machek <pavel@...x.de>
Cc:     Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-kernel@...r.kernel.org, stable@...r.kernel.org,
        Hemant Kumar <hemantk@...eaurora.org>, quic_jhugo@...cinc.com
Subject: Re: [PATCH 5.10 002/299] bus: mhi: core: Clear configuration from
 channel context during reset

On 2021-05-28 01:52 AM, Pavel Machek wrote:
> Hi!
> 
>> > > > > commit 47705c08465931923e2f2b506986ca0bdf80380d upstream.
>> > > > >
>> > > > > When clearing up the channel context after client drivers are
>> > > > > done using channels, the configuration is currently not being
>> > > > > reset entirely. Ensure this is done to appropriately handle
>> > > > > issues where clients unaware of the context state end up calling
>> > > > > functions which expect a context.
>> > > >
>> > > > > +++ b/drivers/bus/mhi/core/init.c
>> > > > > @@ -544,6 +544,7 @@ void mhi_deinit_chan_ctxt(struct mhi_con
>> > > > > +	u32 tmp;
>> > > > > @@ -554,7 +555,19 @@ void mhi_deinit_chan_ctxt(struct mhi_con
>> > > > ...
>> > > > > +	tmp = chan_ctxt->chcfg;
>> > > > > +	tmp &= ~CHAN_CTX_CHSTATE_MASK;
>> > > > > +	tmp |= (MHI_CH_STATE_DISABLED << CHAN_CTX_CHSTATE_SHIFT);
>> > > > > +	chan_ctxt->chcfg = tmp;
>> > > > > +
>> > > > > +	/* Update to all cores */
>> > > > > +	smp_wmb();
>> > > > >  }
>> > > >
>> > > > This is really interesting code; author was careful to make sure chcfg
>> > > > is updated atomically, but C compiler is free to undo that. Plus, this
>> > > > will make all kinds of checkers angry.
>> > > >
>> > > > Does the file need to use READ_ONCE and WRITE_ONCE?
>> > > >
>> > >
>> > > Thanks for looking into this.
>> > >
>> > > I agree that the order could be mangled between chcfg read & write and
>> > > using READ_ONCE & WRITE_ONCE seems to be a good option.
>> > >
>> > > Bhaumik, can you please submit a patch and tag stable?
> 
>> > Hemant and I went over this patch and we noticed this particular function is
>> > already being called with the channel mutex lock held. This would take care
>> > of
>> > the atomicity and we also probably don't need the smp_wmb() barrier as the
>> > mutex
>> > unlock will implicitly take care of it.
>> >
>> 
>> okay
>> 
>> > To the point of compiler re-ordering, we would need some help to understand
>> > the
>> > purpose of READ_ONCE()/WRITE_ONCE() for these dependent statements:
>> >
>> > > +	tmp = chan_ctxt->chcfg;
>> > > +	tmp &= ~CHAN_CTX_CHSTATE_MASK;
>> > > +	tmp |= (MHI_CH_STATE_DISABLED << CHAN_CTX_CHSTATE_SHIFT);
>> > > +	chan_ctxt->chcfg = tmp;
>> >
>> > Since RMW operation means that the chan_ctxt->chcfg is copied to a local
>> > variable (tmp) and the _same_ is being written back to chan_ctxt->chcfg, can
>> > compiler reorder these dependent statements and cause a different result?
>> >
>> 
>> Well, I agree that there is a minimal guarantee with modern day CPUs 
>> on
>> not breaking the order of dependent memory accesses (like here tmp
>> variable is the one which gets read and written) but we want to make
>> sure that this won't break on future CPUs as well. So IMO using
>> READ_ONCE and WRITE_ONCE adds extra level of safety.
> 
> Umm, if this is protected by locking, already, we really should not
> add READ_ONCE. Code should be clear, not having "extra safety levels".
> 
> I assumed it was running unlocked due to the way it was written.
> 
> Best regards,
>     	       	    	      	     	 	       	      Pavel
Thanks for the confirmation Pavel.

Thanks,
Bhaumik
---
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
Forum,
a Linux Foundation Collaborative Project