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

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

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

On 06/12, kgunda@...eaurora.org wrote:
> On 2017-05-31 06:03, Stephen Boyd wrote:
> >On 05/30, Kiran Gunda wrote:
> >>From: Abhijeet Dharmapurikar <adharmap@...eaurora.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