Message-ID: <20170531003310.GS20170@codeaurora.org>
Date:   Tue, 30 May 2017 17:33:10 -0700
From:   Stephen Boyd <sboyd@...eaurora.org>
To:     Kiran Gunda <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
Subject: Re: [PATCH V1 01/15] spmi: pmic_arb: block access of invalid read
 and writes

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.

> 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.

> 
> Signed-off-by: Abhijeet Dharmapurikar <adharmap@...eaurora.org>
> Signed-off-by: Kiran Gunda <kgunda@...eaurora.org>
> ---
>  drivers/spmi/spmi-pmic-arb.c | 84 +++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 83 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c
> index 5ec3a59..df463d4 100644
> --- a/drivers/spmi/spmi-pmic-arb.c
> +++ b/drivers/spmi/spmi-pmic-arb.c
> @@ -111,6 +111,7 @@ enum pmic_arb_cmd_op_code {
>   * @ee:			the current Execution Environment
>   * @min_apid:		minimum APID (used for bounding IRQ search)
>   * @max_apid:		maximum APID
> + * @max_periph:		maximum number of PMIC peripherals supported by HW.

Nitpick: Most of these lines don't end with a full-stop.

>   * @mapping_table:	in-memory copy of PPID -> APID mapping table.
>   * @domain:		irq domain object for PMIC IRQ domain
>   * @spmic:		SPMI controller object
> @@ -132,6 +133,7 @@ struct spmi_pmic_arb_dev {
>  	u8			ee;
>  	u16			min_apid;
>  	u16			max_apid;
> +	u16			max_periph;
>  	u32			*mapping_table;
>  	DECLARE_BITMAP(mapping_table_valid, PMIC_ARB_MAX_PERIPHS);
>  	struct irq_domain	*domain;
> @@ -140,11 +142,13 @@ struct spmi_pmic_arb_dev {
>  	const struct pmic_arb_ver_ops *ver_ops;
>  	u16			*ppid_to_chan;
>  	u16			last_channel;
> +	u8			*chan_to_owner;

And we didn't document this one.

>  };
>  
>  /**
>   * pmic_arb_ver: version dependent functionality.
>   *
> + * @mode:	access rights to specified pmic peripheral.
>   * @non_data_cmd:	on v1 issues an spmi non-data command.
>   *			on v2 no HW support, returns -EOPNOTSUPP.
>   * @offset:		on v1 offset of per-ee channel.
> @@ -160,6 +164,8 @@ struct spmi_pmic_arb_dev {
>   *			on v2 offset of SPMI_PIC_IRQ_CLEARn.
>   */
>  struct pmic_arb_ver_ops {
> +	int (*mode)(struct spmi_pmic_arb_dev *dev, u8 sid, u16 addr,
> +			mode_t *mode);
>  	/* spmi commands (read_cmd, write_cmd, cmd) functionality */
>  	int (*offset)(struct spmi_pmic_arb_dev *dev, u8 sid, u16 addr,
>  		      u32 *offset);
> @@ -313,11 +319,23 @@ static int pmic_arb_read_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
>  	u32 cmd;
>  	int rc;
>  	u32 offset;
> +	mode_t mode;
>  
>  	rc = pmic_arb->ver_ops->offset(pmic_arb, sid, addr, &offset);
>  	if (rc)
>  		return rc;
>  
> +	rc = pmic_arb->ver_ops->mode(pmic_arb, sid, addr, &mode);
> +	if (rc)
> +		return rc;
> +
> +	if (!(mode & S_IRUSR)) {

Using mode_t for hardware access is odd. Perhaps just come up
with some sort of READ/WRITE enum instead (if this sort of
checking is even needed)?

> +		dev_err(&pmic_arb->spmic->dev,

The dev_err() just after uses ctrl->dev? Why not here?

> +			"error: impermissible read from peripheral sid:%d addr:0x%x\n",
> +			sid, addr);
> +		return -EPERM;
> +	}
> +
>  	if (bc >= PMIC_ARB_MAX_TRANS_BYTES) {
>  		dev_err(&ctrl->dev,
>  			"pmic-arb supports 1..%d bytes per trans, but:%zu requested",
> @@ -364,11 +382,23 @@ static int pmic_arb_write_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
>  	u32 cmd;
>  	int rc;
>  	u32 offset;
> +	mode_t mode;
>  
>  	rc = pmic_arb->ver_ops->offset(pmic_arb, sid, addr, &offset);
>  	if (rc)
>  		return rc;
>  
> +	rc = pmic_arb->ver_ops->mode(pmic_arb, sid, addr, &mode);
> +	if (rc)
> +		return rc;
> +
> +	if (!(mode & S_IWUSR)) {
> +		dev_err(&pmic_arb->spmic->dev,

The dev_err() just after uses ctrl->dev? Why not here?

> +			"error: impermissible write to peripheral sid:%d addr:0x%x\n",
> +			sid, addr);
> +		return -EPERM;
> +	}
> +
>  	if (bc >= PMIC_ARB_MAX_TRANS_BYTES) {
>  		dev_err(&ctrl->dev,
>  			"pmic-arb supports 1..%d bytes per trans, but:%zu requested",
> @@ -727,6 +757,13 @@ static int qpnpint_irq_domain_map(struct irq_domain *d,
>  	return 0;
>  }
>  
> +static int
> +pmic_arb_mode_v1(struct spmi_pmic_arb_dev *pa, u8 sid, u16 addr, mode_t *mode)
> +{
> +	*mode = S_IRUSR | S_IWUSR;
> +	return 0;

If mode was only positive then errors could be negative and no
access could be 0. Then we could just return the mode from the
function instead of passing a pointer around.

> +}
> +
>  /* v1 offset per ee */
>  static int
>  pmic_arb_offset_v1(struct spmi_pmic_arb_dev *pa, u8 sid, u16 addr, u32 *offset)
> @@ -745,7 +782,11 @@ static u16 pmic_arb_find_chan(struct spmi_pmic_arb_dev *pa, u16 ppid)
>  	 * PMIC_ARB_REG_CHNL is a table in HW mapping channel to ppid.
>  	 * ppid_to_chan is an in-memory invert of that table.
>  	 */
> -	for (chan = pa->last_channel; ; chan++) {
> +	for (chan = pa->last_channel; chan < pa->max_periph; chan++) {
> +		regval = readl_relaxed(pa->cnfg +
> +				      SPMI_OWNERSHIP_TABLE_REG(chan));
> +		pa->chan_to_owner[chan] = SPMI_OWNERSHIP_PERIPH2OWNER(regval);
> +
>  		offset = PMIC_ARB_REG_CHNL(chan);
>  		if (offset >= pa->core_size)
>  			break;

Seems like an unrelated change to the mapping logic?

> @@ -767,6 +808,27 @@ static u16 pmic_arb_find_chan(struct spmi_pmic_arb_dev *pa, u16 ppid)
>  }
>  
>  
> +static int
> +pmic_arb_mode_v2(struct spmi_pmic_arb_dev *pa, u8 sid, u16 addr, mode_t *mode)

Probably spmi_pmic_arb_dev should be const here.

> +{
> +	u16 ppid = (sid << 8) | (addr >> 8);
> +	u16 chan;
> +	u8 owner;
> +
> +	chan = pa->ppid_to_chan[ppid];
> +	if (!(chan & PMIC_ARB_CHAN_VALID))
> +		return -ENODEV;
> +
> +	*mode = 0;
> +	*mode |= S_IRUSR;
> +
> +	chan &= ~PMIC_ARB_CHAN_VALID;
> +	owner = pa->chan_to_owner[chan];
> +	if (owner == pa->ee)
> +		*mode |= S_IWUSR;
> +	return 0;
> +}
> +
>  /* v2 offset per ppid (chan) and per ee */
>  static int
>  pmic_arb_offset_v2(struct spmi_pmic_arb_dev *pa, u8 sid, u16 addr, u32 *offset)
> @@ -879,6 +943,12 @@ static int spmi_pmic_arb_probe(struct platform_device *pdev)
>  
>  	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "core");
>  	pa->core_size = resource_size(res);
> +	if (pa->core_size <= 0x800) {
> +		dev_err(&pdev->dev, "core_size is smaller than 0x800. Failing Probe\n");

Not sure why probe is capitalized.

> +		err = -EINVAL;
> +		goto err_put_ctrl;
> +	}
> +

We don't need these sorts of DT validation checks. Please remove.

>  	core = devm_ioremap_resource(&ctrl->dev, res);
>  	if (IS_ERR(core)) {
>  		err = PTR_ERR(core);
> @@ -899,6 +969,9 @@ static int spmi_pmic_arb_probe(struct platform_device *pdev)
>  		pa->core = core;
>  		pa->ver_ops = &pmic_arb_v2;
>  
> +		/* the apid to ppid table starts at PMIC_ARB_REG_CHNL(0) */
> +		pa->max_periph =  (pa->core_size - PMIC_ARB_REG_CHNL(0)) / 4;
> +
>  		res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
>  						   "obsrvr");
>  		pa->rd_base = devm_ioremap_resource(&ctrl->dev, res);
>  
>  	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "intr");
> -- 
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
> --

P.S. Please put a newline in your signature so it doesn't exceed
80 columns.

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

Powered by blists - more mailing lists