Re: [PATCH V1 01/15] spmi: pmic_arb: block access of invalid read and writes

From: Stephen Boyd <sboyd@codeaurora.org>
To: kgunda@codeaurora.org
Cc: Abhijeet Dharmapurikar <adharmap@codeaurora.org>,
	Christophe JAILLET <christophe.jaillet@wanadoo.fr>,
	Subbaraman Narayanamurthy <subbaram@codeaurora.org>,
	David Collins <collinsd@codeaurora.org>,
	linux-kernel@vger.kernel.org, linux-arm-msm@vger.kernel.org,
	adharmap@quicinc.com, aghayal@qti.qualcomm.com,
	linux-arm-msm-owner@vger.kernel.org
Subject: Re: [PATCH V1 01/15] spmi: pmic_arb: block access of invalid read and writes
Date: Mon, 12 Jun 2017 19:09:43 -0700	[thread overview]
Message-ID: <20170613020943.GR20170@codeaurora.org> (raw)
In-Reply-To: <a3f516a2ab42c5ed1d7b7996846e6d67@codeaurora.org>

On 06/12, kgunda@codeaurora.org wrote:
> On 2017-05-31 06:03, Stephen Boyd wrote:
> >On 05/30, Kiran Gunda wrote:
> >>From: Abhijeet Dharmapurikar <adharmap@codeaurora.org>
> >>
> >>The system crashes due to bad access when reading from an non
> >>configured
> >>peripheral and when writing to peripheral which is not owned by
> >>current
> >>ee. This patch verifies ownership to avoid crashing on
> >>write.
> >
> >What systems? As far as I know we don't have any bad accesses
> >happening right now. If they are happening, we should fix the
> >code that's accessing hardware that isn't owned by them.
> >
> This change greatly improves the debugging effort for developers by
> printing
> a very simple and clear error message when an invalid SPMI access occurs
> (due to bad DT configuration, bad bootloader SPMI permission
> configurations,
> or other issues).  Without this change, such accesses will cause XPU
> violations
> that crash the system and require extensive effort to decode.

Right, but they're easily detectable because we would know almost
immediately that something isn't working when we integrate a
change. If you update the DT and it stops working, the DT is bad.
If you update the bootloader and it stops working, the bootloader
is bad, etc.

> 
> >>For reads, since the forward mapping table, data_channel->ppid, is
> >>towards the end of the block, we use the core size to figure the
> >>max number of ppids supported. The table starts at an offset of 0x800
> >>within the block, so size - 0x800 will give us the area used by the
> >>table. Since each table is 4 bytes long (core_size - 0x800) / 4 will
> >>gives us the number of data_channel supported.
> >>This new protection is functional on hw v2.
> >
> >Which brings us to the next question which is why do we need this
> >patch at all? We aren't probing hardware to see what we have
> >access to and then populating device structures based on that.
> >Instead, we're just populating DT nodes that we've hardcoded in
> >the dts files, so I'm a little lost on why we would have a node
> >in there that we couldn't access. Please add such details to the
> >commit text.
> >
> invalid SPMI access occurs due to bad DT configuration, bad
> bootloader SPMI
> permission configurations, or other issues. This change reduces the
> debugging
> effort for developers by printing clear error message when an
> invalid SPMI
> access occurs.

Well we also take an overhead on every read/write. Sure things
are slow so the overhead is negligible, but the permissions are
on a peripheral id basis, so really we should look into _not_
populating devices that aren't accessible in the first place.
Then we move the checks out of the read/write path and to a more
logical place whereby we prevent a driver from attempting to even
attach to read or write a register that is protected.

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