diff mbox

[3/3] pinctrl: qcom: Don't allow protected pins to be requested

Message ID 20180110015848.11480-4-sboyd@codeaurora.org (mailing list archive)
State New, archived
Headers show

Commit Message

Stephen Boyd Jan. 10, 2018, 1:58 a.m. UTC
Some qcom platforms make some GPIOs or pins unavailable for use
by non-secure operating systems, and thus reading or writing the
registers for those pins will cause access control issues and
reset the device. With a DT/ACPI property to describe the set of
pins that are available for use, parse the available pins and set
the irq valid bits for gpiolib to know what to consider 'valid'.
This should avoid any issues with gpiolib. Furthermore, implement
the pinmux_ops::request function so that pinmux can also make
sure to not use pins that are unavailable.

Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
---
 drivers/pinctrl/qcom/pinctrl-msm.c | 98 ++++++++++++++++++++++++++++++++++++--
 1 file changed, 94 insertions(+), 4 deletions(-)

Comments

Bjorn Andersson Jan. 10, 2018, 6:11 a.m. UTC | #1
On Tue 09 Jan 17:58 PST 2018, Stephen Boyd wrote:

I like it, a few comment below though.

> +static int msm_gpio_init_irq_valid_mask(struct gpio_chip *chip,
> +					struct msm_pinctrl *pctrl)
> +{
> +	int ret;
> +	unsigned int len, i;
> +	unsigned int max_gpios = pctrl->soc->ngpios;
> +	struct device_node *np = pctrl->dev->of_node;
> +
> +	/* The number of GPIOs in the ACPI tables */
> +	ret = device_property_read_u16_array(pctrl->dev, "gpios", NULL, 0);
> +	if (ret > 0 && ret < max_gpios) {
> +		u16 *tmp;
> +
> +		len = ret;
> +		tmp = kmalloc_array(len, sizeof(tmp[0]), GFP_KERNEL);
> +		if (!tmp)
> +			return -ENOMEM;
> +
> +		ret = device_property_read_u16_array(pctrl->dev, "gpios", tmp,
> +						     len);
> +		if (ret < 0) {
> +			dev_err(pctrl->dev, "could not read list of GPIOs\n");
> +			kfree(tmp);
> +			return ret;
> +		}
> +
> +		bitmap_zero(chip->irq_valid_mask, max_gpios);

The valid_mask is moving into the gpio_irq_chip, so this should be
chip->irq.valid_mask.

See dc7b0387ee89 ("gpio: Move irq_valid_mask into struct gpio_irq_chip")

> +		for (i = 0; i < len; i++)
> +			set_bit(tmp[i], chip->irq_valid_mask);
> +

You're leaking tmp here.

> +		return 0;
> +	}
> +
> +	/* If there's a DT ngpios-ranges property then add those ranges */
> +	ret = of_property_count_u32_elems(np,  "ngpios-ranges");
> +	if (ret > 0 && ret % 2 == 0 && ret / 2 < max_gpios) {
> +		u32 start;
> +		u32 count;
> +
> +		len = ret / 2;
> +		bitmap_zero(chip->irq_valid_mask, max_gpios);
> +
> +		for (i = 0; i < len; i++) {

If you take steps of 2, when looping from 0 to ret, your loop index will
have the value that you're passing as index into the read - without
duplicating it.

> +			of_property_read_u32_index(np, "ngpios-ranges",
> +						   i * 2, &start);
> +			of_property_read_u32_index(np, "ngpios-ranges",
> +						   i * 2 + 1, &count);
> +			bitmap_set(chip->irq_valid_mask, start, count);
> +		}
> +	}
> +
> +	return 0;
> +}
[..]
> @@ -824,6 +907,7 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl)
>  	chip->parent = pctrl->dev;
>  	chip->owner = THIS_MODULE;
>  	chip->of_node = pctrl->dev->of_node;
> +	chip->irq_need_valid_mask = msm_gpio_needs_irq_valid_mask(pctrl);

chip->irq.need_valid_mask

>  
>  	ret = gpiochip_add_data(&pctrl->chip, pctrl);
>  	if (ret) {
> @@ -831,6 +915,12 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl)
>  		return ret;
>  	}
>  
> +	ret = msm_gpio_init_irq_valid_mask(chip, pctrl);
> +	if (ret) {
> +		dev_err(pctrl->dev, "Failed to setup irq valid bits\n");

gpiochip_remove() here, to release resources allocated by
gpiochip_add_data.

> +		return ret;
> +	}
> +
>  	ret = gpiochip_add_pin_range(&pctrl->chip, dev_name(pctrl->dev), 0, 0, chip->ngpio);
>  	if (ret) {
>  		dev_err(pctrl->dev, "Failed to add pin range\n");

Regards,
Bjorn
Timur Tabi Jan. 22, 2018, 1:55 p.m. UTC | #2
On 1/9/18 7:58 PM, Stephen Boyd wrote:
> +		ret = device_property_read_u16_array(pctrl->dev, "gpios", tmp,
> +						     len);
> +		if (ret < 0) {
> +			dev_err(pctrl->dev, "could not read list of GPIOs\n");
> +			kfree(tmp);
> +			return ret;
> +		}

Just FYI, I'm still going to have to parse "gpios" in my 
pinctrl-qdf2xxx.c driver, even though you're also parsing it here. 
That's because I need to make sure that the msm_pingroup array only 
contains "approve" addresses in its ctl_reg fields.

+	for (i = 0; i < avail_gpios; i++) {
+		unsigned int gpio = gpios[i];
+
+		groups[gpio].npins = 1;
+		snprintf(names[i], NAME_SIZE, "gpio%u", gpio);
+		pins[gpio].name = names[i];
+		groups[gpio].name = names[i];
+
+		groups[gpio].ctl_reg = 0x10000 * gpio;
  		       ^^^^

I do this because I need to make sure that "unapproved" physical 
addresses are never store anywhere in groups[].  That way, it's 
impossible for the driver to cause an XPU violation -- the worst that 
can happen is a null pointer dereference.
Timur Tabi Jan. 22, 2018, 8:03 p.m. UTC | #3
On 01/22/2018 07:55 AM, Timur Tabi wrote:
> 
> Just FYI, I'm still going to have to parse "gpios" in my 
> pinctrl-qdf2xxx.c driver, even though you're also parsing it here. 
> That's because I need to make sure that the msm_pingroup array only 
> contains "approve" addresses in its ctl_reg fields.

Also, my patch

[PATCH 3/3] [v7] pinctrl: qcom: qdf2xxx: add support for new ACPI HID 
QCOM8002

Applies on top of your patches as-is.

Also, what about

[PATCH 1/3] [v2] Revert "gpio: set up initial state from .get_direction()"

I think you still need this patch.
Timur Tabi Jan. 25, 2018, 8:48 p.m. UTC | #4
On 01/09/2018 07:58 PM, Stephen Boyd wrote:
> +static int msm_pinmux_request(struct pinctrl_dev *pctldev, unsigned offset)

"unsigned int", instead of just "unsigned"?
Stephen Boyd Jan. 25, 2018, 9:51 p.m. UTC | #5
On 01/22, Timur Tabi wrote:
> On 1/9/18 7:58 PM, Stephen Boyd wrote:
> >+		ret = device_property_read_u16_array(pctrl->dev, "gpios", tmp,
> >+						     len);
> >+		if (ret < 0) {
> >+			dev_err(pctrl->dev, "could not read list of GPIOs\n");
> >+			kfree(tmp);
> >+			return ret;
> >+		}
> 
> Just FYI, I'm still going to have to parse "gpios" in my
> pinctrl-qdf2xxx.c driver, even though you're also parsing it here.
> That's because I need to make sure that the msm_pingroup array only
> contains "approve" addresses in its ctl_reg fields.
> 
> +	for (i = 0; i < avail_gpios; i++) {
> +		unsigned int gpio = gpios[i];
> +
> +		groups[gpio].npins = 1;
> +		snprintf(names[i], NAME_SIZE, "gpio%u", gpio);
> +		pins[gpio].name = names[i];
> +		groups[gpio].name = names[i];
> +
> +		groups[gpio].ctl_reg = 0x10000 * gpio;
>  		       ^^^^
> 
> I do this because I need to make sure that "unapproved" physical
> addresses are never store anywhere in groups[].  That way, it's
> impossible for the driver to cause an XPU violation -- the worst
> that can happen is a null pointer dereference.
> 

Sorry I don't get it. Is that some sort of hardening requirement?
If the framework doesn't cause those pins to be touched I fail to
see how it could hurt to have the other addresses listed. I'm
sure with some effort protected addresses could be crafted in
other ways to cause an XPU violation to the same place.
Timur Tabi Jan. 25, 2018, 9:53 p.m. UTC | #6
On 01/25/2018 03:51 PM, Stephen Boyd wrote:
> Sorry I don't get it. Is that some sort of hardening requirement?
> If the framework doesn't cause those pins to be touched I fail to
> see how it could hurt to have the other addresses listed. I'm
> sure with some effort protected addresses could be crafted in
> other ways to cause an XPU violation to the same place.

It's for my own sanity.  By ensuring that those physical addresses are 
not ever present in the driver or any data structure, I can fend off, 
"Hey Timur, your gpio driver is causing XPU violations again, heh heh".
diff mbox

Patch

diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
index 7a960590ecaa..4a251268ebc4 100644
--- a/drivers/pinctrl/qcom/pinctrl-msm.c
+++ b/drivers/pinctrl/qcom/pinctrl-msm.c
@@ -105,6 +105,17 @@  static const struct pinctrl_ops msm_pinctrl_ops = {
 	.dt_free_map		= pinctrl_utils_free_map,
 };
 
+static int msm_pinmux_request(struct pinctrl_dev *pctldev, unsigned offset)
+{
+	struct msm_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
+	struct gpio_chip *chip = &pctrl->chip;
+
+	if (gpiochip_irqchip_irq_valid(chip, offset))
+		return 0;
+
+	return -EINVAL;
+}
+
 static int msm_get_functions_count(struct pinctrl_dev *pctldev)
 {
 	struct msm_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
@@ -166,6 +177,7 @@  static int msm_pinmux_set_mux(struct pinctrl_dev *pctldev,
 }
 
 static const struct pinmux_ops msm_pinmux_ops = {
+	.request		= msm_pinmux_request,
 	.get_functions_count	= msm_get_functions_count,
 	.get_function_name	= msm_get_function_name,
 	.get_function_groups	= msm_get_function_groups,
@@ -506,6 +518,9 @@  static void msm_gpio_dbg_show_one(struct seq_file *s,
 		"pull up"
 	};
 
+	if (!gpiochip_irqchip_irq_valid(chip, offset))
+		return;
+
 	g = &pctrl->soc->groups[offset];
 	ctl_reg = readl(pctrl->regs + g->ctl_reg);
 
@@ -516,7 +531,7 @@  static void msm_gpio_dbg_show_one(struct seq_file *s,
 
 	seq_printf(s, " %-8s: %-3s %d", g->name, is_out ? "out" : "in", func);
 	seq_printf(s, " %dmA", msm_regval_to_drive(drive));
-	seq_printf(s, " %s", pulls[pull]);
+	seq_printf(s, " %s\n", pulls[pull]);
 }
 
 static void msm_gpio_dbg_show(struct seq_file *s, struct gpio_chip *chip)
@@ -524,10 +539,8 @@  static void msm_gpio_dbg_show(struct seq_file *s, struct gpio_chip *chip)
 	unsigned gpio = chip->base;
 	unsigned i;
 
-	for (i = 0; i < chip->ngpio; i++, gpio++) {
+	for (i = 0; i < chip->ngpio; i++, gpio++)
 		msm_gpio_dbg_show_one(s, NULL, chip, i, gpio);
-		seq_puts(s, "\n");
-	}
 }
 
 #else
@@ -808,6 +821,76 @@  static void msm_gpio_irq_handler(struct irq_desc *desc)
 	chained_irq_exit(chip, desc);
 }
 
+static int msm_gpio_init_irq_valid_mask(struct gpio_chip *chip,
+					struct msm_pinctrl *pctrl)
+{
+	int ret;
+	unsigned int len, i;
+	unsigned int max_gpios = pctrl->soc->ngpios;
+	struct device_node *np = pctrl->dev->of_node;
+
+	/* The number of GPIOs in the ACPI tables */
+	ret = device_property_read_u16_array(pctrl->dev, "gpios", NULL, 0);
+	if (ret > 0 && ret < max_gpios) {
+		u16 *tmp;
+
+		len = ret;
+		tmp = kmalloc_array(len, sizeof(tmp[0]), GFP_KERNEL);
+		if (!tmp)
+			return -ENOMEM;
+
+		ret = device_property_read_u16_array(pctrl->dev, "gpios", tmp,
+						     len);
+		if (ret < 0) {
+			dev_err(pctrl->dev, "could not read list of GPIOs\n");
+			kfree(tmp);
+			return ret;
+		}
+
+		bitmap_zero(chip->irq_valid_mask, max_gpios);
+		for (i = 0; i < len; i++)
+			set_bit(tmp[i], chip->irq_valid_mask);
+
+		return 0;
+	}
+
+	/* If there's a DT ngpios-ranges property then add those ranges */
+	ret = of_property_count_u32_elems(np,  "ngpios-ranges");
+	if (ret > 0 && ret % 2 == 0 && ret / 2 < max_gpios) {
+		u32 start;
+		u32 count;
+
+		len = ret / 2;
+		bitmap_zero(chip->irq_valid_mask, max_gpios);
+
+		for (i = 0; i < len; i++) {
+			of_property_read_u32_index(np, "ngpios-ranges",
+						   i * 2, &start);
+			of_property_read_u32_index(np, "ngpios-ranges",
+						   i * 2 + 1, &count);
+			bitmap_set(chip->irq_valid_mask, start, count);
+		}
+	}
+
+	return 0;
+}
+
+static bool msm_gpio_needs_irq_valid_mask(struct msm_pinctrl *pctrl)
+{
+	int ret;
+	struct device_node *np = pctrl->dev->of_node;
+
+	ret = device_property_read_u16_array(pctrl->dev, "gpios", NULL, 0);
+	if (ret > 0)
+		return true;
+
+	ret = of_property_count_u32_elems(np,  "ngpios-ranges");
+	if (ret > 0 && ret % 2 == 0)
+		return true;
+
+	return false;
+}
+
 static int msm_gpio_init(struct msm_pinctrl *pctrl)
 {
 	struct gpio_chip *chip;
@@ -824,6 +907,7 @@  static int msm_gpio_init(struct msm_pinctrl *pctrl)
 	chip->parent = pctrl->dev;
 	chip->owner = THIS_MODULE;
 	chip->of_node = pctrl->dev->of_node;
+	chip->irq_need_valid_mask = msm_gpio_needs_irq_valid_mask(pctrl);
 
 	ret = gpiochip_add_data(&pctrl->chip, pctrl);
 	if (ret) {
@@ -831,6 +915,12 @@  static int msm_gpio_init(struct msm_pinctrl *pctrl)
 		return ret;
 	}
 
+	ret = msm_gpio_init_irq_valid_mask(chip, pctrl);
+	if (ret) {
+		dev_err(pctrl->dev, "Failed to setup irq valid bits\n");
+		return ret;
+	}
+
 	ret = gpiochip_add_pin_range(&pctrl->chip, dev_name(pctrl->dev), 0, 0, chip->ngpio);
 	if (ret) {
 		dev_err(pctrl->dev, "Failed to add pin range\n");