[PATCH 0/9] serial: Fix out-of-bounds accesses through DT aliases

[PATCH 0/9] serial: Fix out-of-bounds accesses through DT aliases

[Geert Uytterhoeven]

	Hi all,

Serial drivers used on DT platforms use the "serialN" alias in DT to
obtain the serial port index for a specific port.  Drivers typically use
a fixed-size array for keeping track of all available serial ports.
However, several drivers do not perform any validation on the index
obtained from DT, which may lead to out-of-bounds accesses of these
fixed-size arrays.

While the DTB passed to the kernel might be considered trusted, some of
these out-of-bounds accesses can be triggered by a legitimate DTB:
  - In some drivers the size of the array is defined by a Kconfig
    symbol, so a user who doesn't need all serial ports may lower this
    value rightfully,
  - Tomorrow's new SoC may have more serial ports than the fixed-size
    array in today's driver can accommodate, which the user may forget
    to enlarge.

Hence this series fixes that by adding checks for out-of-range aliases,
logging an error message when triggered.

Tested on r8a7791/koelsch (sh-sci), all other drivers were
compile-tested only.

Thanks for your comments!

Geert Uytterhoeven (9):
  serial: arc_uart: Fix out-of-bounds access through DT alias
  serial: fsl_lpuart: Fix out-of-bounds access through DT alias
  serial: imx: Fix out-of-bounds access through DT alias
  serial: mxs-auart: Fix out-of-bounds access through DT alias
  serial: pxa: Fix out-of-bounds access through DT alias
  serial: samsung: Fix out-of-bounds access through DT alias
  serial: sh-sci: Fix out-of-bounds access through DT alias
  serial: sirf: Fix out-of-bounds access through DT alias
  serial: xuartps: Fix out-of-bounds access through DT alias

 drivers/tty/serial/arc_uart.c      | 5 +++++
 drivers/tty/serial/fsl_lpuart.c    | 4 ++++
 drivers/tty/serial/imx.c           | 5 +++++
 drivers/tty/serial/mxs-auart.c     | 4 ++++
 drivers/tty/serial/pxa.c           | 4 ++++
 drivers/tty/serial/samsung.c       | 4 ++++
 drivers/tty/serial/sh-sci.c        | 4 ++++
 drivers/tty/serial/sirfsoc_uart.c  | 5 +++++
 drivers/tty/serial/xilinx_uartps.c | 2 +-
 9 files changed, 36 insertions(+), 1 deletion(-)

-- 
2.7.4

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

[PATCH 1/9] serial: arc_uart: Fix out-of-bounds access through DT alias

[Geert Uytterhoeven]

The arc_uart_ports[] array is indexed using a value derived from the
"serialN" alias in DT, which may lead to an out-of-bounds access.

Fix this by adding a range check.

Note that the array size is defined by a Kconfig symbol
(CONFIG_SERIAL_ARC_NR_PORTS), so this can even be triggered using a
legitimate DTB.

Fixes: 10640deb04b7949a ("serial: arc_uart: Fix out-of-bounds access through DT alias")
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 drivers/tty/serial/arc_uart.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/tty/serial/arc_uart.c b/drivers/tty/serial/arc_uart.c
index 2599f9ecccfe7769..1cb827a6b836d0dd 100644
--- a/drivers/tty/serial/arc_uart.c
+++ b/drivers/tty/serial/arc_uart.c
@@ -593,6 +593,11 @@ static int arc_serial_probe(struct platform_device *pdev)
 	if (dev_id < 0)
 		dev_id = 0;
 
+	if (dev_id >= CONFIG_SERIAL_ARC_NR_PORTS) {
+		dev_err(&pdev->dev, "serial%d out of range\n", dev_id);
+		return -EINVAL;
+	}
+
 	uart = &arc_uart_ports[dev_id];
 	port = &uart->port;
 
-- 
2.7.4

Re: [PATCH 1/9] serial: arc_uart: Fix out-of-bounds access through DT alias

[Geert Uytterhoeven]

On Tue, Feb 20, 2018 at 10:40 AM, Geert Uytterhoeven
<geert+renesas@glider.be> wrote:
> The arc_uart_ports[] array is indexed using a value derived from the
> "serialN" alias in DT, which may lead to an out-of-bounds access.
>
> Fix this by adding a range check.
>
> Note that the array size is defined by a Kconfig symbol
> (CONFIG_SERIAL_ARC_NR_PORTS), so this can even be triggered using a
> legitimate DTB.
>
> Fixes: 10640deb04b7949a ("serial: arc_uart: Fix out-of-bounds access through DT alias")

Fixes: ea28fd56fcde69af ("serial/arc-uart: switch to devicetree based probing")

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[PATCH 2/9] serial: fsl_lpuart: Fix out-of-bounds access through DT alias

[Geert Uytterhoeven]

The lpuart_ports[] array is indexed using a value derived from the
"serialN" alias in DT, which may lead to an out-of-bounds access.

Fix this by adding a range check.

Fixes: 970416c691dc68b5 ("serial: fsl_lpuart: Fix out-of-bounds access through DT alias")
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 drivers/tty/serial/fsl_lpuart.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/tty/serial/fsl_lpuart.c b/drivers/tty/serial/fsl_lpuart.c
index 8cf112f2efc30707..5218e2ed1ba90a5b 100644
--- a/drivers/tty/serial/fsl_lpuart.c
+++ b/drivers/tty/serial/fsl_lpuart.c
@@ -2145,6 +2145,10 @@ static int lpuart_probe(struct platform_device *pdev)
 		dev_err(&pdev->dev, "failed to get alias id, errno %d\n", ret);
 		return ret;
 	}
+	if (ret >= UART_NR) {
+		dev_err(&pdev->dev, "serial%d out of range\n", ret);
+		return -EINVAL;
+	}
 	sport->port.line = ret;
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	sport->port.membase = devm_ioremap_resource(&pdev->dev, res);
-- 
2.7.4

Re: [PATCH 2/9] serial: fsl_lpuart: Fix out-of-bounds access through DT alias

[Geert Uytterhoeven]

On Tue, Feb 20, 2018 at 10:40 AM, Geert Uytterhoeven
<geert+renesas@glider.be> wrote:
> The lpuart_ports[] array is indexed using a value derived from the
> "serialN" alias in DT, which may lead to an out-of-bounds access.
>
> Fix this by adding a range check.
>
> Fixes: 970416c691dc68b5 ("serial: fsl_lpuart: Fix out-of-bounds access through DT alias")
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

Fixes: c9e2e946fb0ba5d2 ("tty: serial: add Freescale lpuart driver support")

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[PATCH 3/9] serial: imx: Fix out-of-bounds access through DT alias

[Geert Uytterhoeven]

The imx_ports[] array is indexed using a value derived from the
"serialN" alias in DT, which may lead to an out-of-bounds access.

Fix this by adding a range check.

Fixes: 9206ab8a0350c3da ("serial: imx: Fix out-of-bounds access through DT alias")
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 drivers/tty/serial/imx.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 1d7ca382bc12b238..e89e90ad87d8245c 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -2041,6 +2041,11 @@ static int serial_imx_probe(struct platform_device *pdev)
 		serial_imx_probe_pdata(sport, pdev);
 	else if (ret < 0)
 		return ret;
+	if (sport->port.line >= UART_NR) {
+		dev_err(&pdev->dev, "serial%d out of range\n",
+			sport->port.line);
+		return -EINVAL;
+	}
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	base = devm_ioremap_resource(&pdev->dev, res);
-- 
2.7.4

Re: [PATCH 3/9] serial: imx: Fix out-of-bounds access through DT alias

[Uwe Kleine-König]

Hello Geert,

On Tue, Feb 20, 2018 at 10:40:18AM +0100, Geert Uytterhoeven wrote:
> The imx_ports[] array is indexed using a value derived from the
> "serialN" alias in DT, which may lead to an out-of-bounds access.
> 
> Fix this by adding a range check.
> 
> Fixes: 9206ab8a0350c3da ("serial: imx: Fix out-of-bounds access through DT alias")

huh, this patch fixes itself?

> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
>  drivers/tty/serial/imx.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> index 1d7ca382bc12b238..e89e90ad87d8245c 100644
> --- a/drivers/tty/serial/imx.c
> +++ b/drivers/tty/serial/imx.c
> @@ -2041,6 +2041,11 @@ static int serial_imx_probe(struct platform_device *pdev)
>  		serial_imx_probe_pdata(sport, pdev);
>  	else if (ret < 0)
>  		return ret;

I'd prefer an empty line here.

> +	if (sport->port.line >= UART_NR) {

I would have used:

	if (sport->port.line >= ARRAY_SIZE(imx_ports))

which IMHO is better understandable
> +		dev_err(&pdev->dev, "serial%d out of range\n",
> +			sport->port.line);

Note that the same overflow can happen when a device is probed using
platform data (and your commit fixes that, too). Maybe worth to point
out in the commit log?

Other than that: Good catch, thanks for your patch.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH 3/9] serial: imx: Fix out-of-bounds access through DT alias

[Geert Uytterhoeven]

Hi Uwe,

On Tue, Feb 20, 2018 at 11:31 AM, Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
> On Tue, Feb 20, 2018 at 10:40:18AM +0100, Geert Uytterhoeven wrote:
>> The imx_ports[] array is indexed using a value derived from the
>> "serialN" alias in DT, which may lead to an out-of-bounds access.
>>
>> Fix this by adding a range check.
>>
>> Fixes: 9206ab8a0350c3da ("serial: imx: Fix out-of-bounds access through DT alias")
>
> huh, this patch fixes itself?

Oops

    Fixes: ff05967a07225ab6 ("serial/imx: add of_alias_get_id() reference back")

>
>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
>> ---
>>  drivers/tty/serial/imx.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
>> index 1d7ca382bc12b238..e89e90ad87d8245c 100644
>> --- a/drivers/tty/serial/imx.c
>> +++ b/drivers/tty/serial/imx.c
>> @@ -2041,6 +2041,11 @@ static int serial_imx_probe(struct platform_device *pdev)
>>               serial_imx_probe_pdata(sport, pdev);
>>       else if (ret < 0)
>>               return ret;
>
> I'd prefer an empty line here.

OK

>> +     if (sport->port.line >= UART_NR) {
>
> I would have used:
>
>         if (sport->port.line >= ARRAY_SIZE(imx_ports))
>
> which IMHO is better understandable

OK.

>> +             dev_err(&pdev->dev, "serial%d out of range\n",
>> +                     sport->port.line);
>
> Note that the same overflow can happen when a device is probed using
> platform data (and your commit fixes that, too). Maybe worth to point
> out in the commit log?

That's correct. But board code is tied more intimate to the kernel.
Will update.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

_______________________________________________
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc

[PATCH 4/9] serial: mxs-auart: Fix out-of-bounds access through DT alias

[Geert Uytterhoeven]

The auart_port[] array is indexed using a value derived from the
"serialN" alias in DT, which may lead to an out-of-bounds access.

Fix this by adding a range check.

Fixes: cabf23e7aa00b145 ("serial: mxs-auart: Fix out-of-bounds access through DT alias")
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 drivers/tty/serial/mxs-auart.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/tty/serial/mxs-auart.c b/drivers/tty/serial/mxs-auart.c
index 079dc47aa142d8e1..7ec1298512facfc8 100644
--- a/drivers/tty/serial/mxs-auart.c
+++ b/drivers/tty/serial/mxs-auart.c
@@ -1663,6 +1663,10 @@ static int mxs_auart_probe(struct platform_device *pdev)
 		s->port.line = pdev->id < 0 ? 0 : pdev->id;
 	else if (ret < 0)
 		return ret;
+	if (s->port.line >= MXS_AUART_PORTS) {
+		dev_err(&pdev->dev, "serial%d out of range\n", s->port.line);
+		return -EINVAL;
+	}
 
 	if (of_id) {
 		pdev->id_entry = of_id->data;
-- 
2.7.4

Re: [PATCH 4/9] serial: mxs-auart: Fix out-of-bounds access through DT alias

[Geert Uytterhoeven]

On Tue, Feb 20, 2018 at 10:40 AM, Geert Uytterhoeven
<geert+renesas@glider.be> wrote:
> The auart_port[] array is indexed using a value derived from the
> "serialN" alias in DT, which may lead to an out-of-bounds access.
>
> Fix this by adding a range check.
>
> Fixes: cabf23e7aa00b145 ("serial: mxs-auart: Fix out-of-bounds access through DT alias")

Fixes: 1ea6607d4cdc9179 ("serial: mxs-auart: Allow device tree probing")

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[PATCH 5/9] serial: pxa: Fix out-of-bounds access through DT alias

[Geert Uytterhoeven]

The serial_pxa_ports[] array is indexed using a value derived from the
"serialN" alias in DT, which may lead to an out-of-bounds access.

Fix this by adding a range check.

Fixes: c8dcdc77298dde67 ("serial: pxa: Fix out-of-bounds access through DT alias")
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 drivers/tty/serial/pxa.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/tty/serial/pxa.c b/drivers/tty/serial/pxa.c
index baf552944d5686e8..ac25faa95baaad0e 100644
--- a/drivers/tty/serial/pxa.c
+++ b/drivers/tty/serial/pxa.c
@@ -885,6 +885,10 @@ static int serial_pxa_probe(struct platform_device *dev)
 		sport->port.line = dev->id;
 	else if (ret < 0)
 		goto err_clk;
+	if (sport->port.line > ARRAY_SIZE(serial_pxa_ports)) {
+		dev_err(&dev->dev, "serial%d out of range\n", sport->port.line);
+		return -EINVAL;
+	}
 	snprintf(sport->name, PXA_NAME_LEN - 1, "UART%d", sport->port.line + 1);
 
 	sport->port.membase = ioremap(mmres->start, resource_size(mmres));
-- 
2.7.4

Re: [PATCH 5/9] serial: pxa: Fix out-of-bounds access through DT alias

[Geert Uytterhoeven]

On Tue, Feb 20, 2018 at 10:40 AM, Geert Uytterhoeven
<geert+renesas@glider.be> wrote:
> The serial_pxa_ports[] array is indexed using a value derived from the
> "serialN" alias in DT, which may lead to an out-of-bounds access.
>
> Fix this by adding a range check.
>
> Fixes: c8dcdc77298dde67 ("serial: pxa: Fix out-of-bounds access through DT alias")

Fixes: 699c20f3e6310aa2 ("serial: pxa: add OF support")

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[PATCH 6/9] serial: samsung: Fix out-of-bounds access through DT alias

[Geert Uytterhoeven]

The s3c24xx_serial_ports[] array is indexed using a value derived from
the "serialN" alias in DT, which may lead to an out-of-bounds access.

Fix this by adding a range check.

Note that the array size is defined by a Kconfig symbol
(CONFIG_SERIAL_SAMSUNG_UARTS), so this can even be triggered using a
legitimate DTB.

Fixes: 3ac337e76a1c637b ("serial: samsung: Fix out-of-bounds access through DT alias")
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 drivers/tty/serial/samsung.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/tty/serial/samsung.c b/drivers/tty/serial/samsung.c
index f9fecc5ed0cee826..9ea42197ceefabba 100644
--- a/drivers/tty/serial/samsung.c
+++ b/drivers/tty/serial/samsung.c
@@ -1818,6 +1818,10 @@ static int s3c24xx_serial_probe(struct platform_device *pdev)
 
 	dbg("s3c24xx_serial_probe(%p) %d\n", pdev, index);
 
+	if (index >= CONFIG_SERIAL_SAMSUNG_UARTS) {
+		dev_err(&pdev->dev, "serial%d out of range\n", index);
+		return -EINVAL;
+	}
 	ourport = &s3c24xx_serial_ports[index];
 
 	ourport->drv_data = s3c24xx_get_driver_data(pdev);
-- 
2.7.4

Re: [PATCH 6/9] serial: samsung: Fix out-of-bounds access through DT alias

[Geert Uytterhoeven]

On Tue, Feb 20, 2018 at 10:40 AM, Geert Uytterhoeven
<geert+renesas@glider.be> wrote:
> The s3c24xx_serial_ports[] array is indexed using a value derived from
> the "serialN" alias in DT, which may lead to an out-of-bounds access.
>
> Fix this by adding a range check.
>
> Note that the array size is defined by a Kconfig symbol
> (CONFIG_SERIAL_SAMSUNG_UARTS), so this can even be triggered using a
> legitimate DTB.
>
> Fixes: 3ac337e76a1c637b ("serial: samsung: Fix out-of-bounds access through DT alias")

Fixes: 13a9f6c64fdc55eb ("serial: samsung: Consider DT alias when probing po
rts")

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[PATCH 7/9] serial: sh-sci: Fix out-of-bounds access through DT alias

[Geert Uytterhoeven]

The sci_ports[] array is indexed using a value derived from the
"serialN" alias in DT, which may lead to an out-of-bounds access.

Fix this by adding a range check.

Note that the array size is defined by a Kconfig symbol
(CONFIG_SERIAL_SH_SCI_NR_UARTS), so this can even be triggered using a
legitimate DTB.

Fixes: f650cdf1c115498e ("serial: sh-sci: Fix out-of-bounds access through DT alias")
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 drivers/tty/serial/sh-sci.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index 4d14f321cbec95e0..0fb4784860da6188 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -3620,6 +3620,10 @@ static struct plat_sci_port *sci_parse_dt(struct platform_device *pdev,
 		dev_err(&pdev->dev, "failed to get alias id (%d)\n", id);
 		return NULL;
 	}
+	if (id >= SCI_NPORTS) {
+		dev_err(&pdev->dev, "serial%d out of range\n", id);
+		return NULL;
+	}
 
 	sp = &sci_ports[id];
 	*dev_id = id;
-- 
2.7.4

Re: [PATCH 7/9] serial: sh-sci: Fix out-of-bounds access through DT alias

[Geert Uytterhoeven]

On Tue, Feb 20, 2018 at 10:40 AM, Geert Uytterhoeven
<geert+renesas@glider.be> wrote:
> The sci_ports[] array is indexed using a value derived from the
> "serialN" alias in DT, which may lead to an out-of-bounds access.
>
> Fix this by adding a range check.
>
> Note that the array size is defined by a Kconfig symbol
> (CONFIG_SERIAL_SH_SCI_NR_UARTS), so this can even be triggered using a
> legitimate DTB.
>
> Fixes: f650cdf1c115498e ("serial: sh-sci: Fix out-of-bounds access through DT alias")

Fixes: 97ed9790c514066b ("serial: sh-sci: Remove unused platform data
capabilities field")

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[PATCH 8/9] serial: sirf: Fix out-of-bounds access through DT alias

[Geert Uytterhoeven]

The sirf_ports[] array is indexed using a value derived from the
"serialN" alias in DT, which may lead to an out-of-bounds access.

Fix this by adding a range check.

Fixes: 66c7ab1120585d18 ("serial: sirf: Fix out-of-bounds access through DT alias")
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 drivers/tty/serial/sirfsoc_uart.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/tty/serial/sirfsoc_uart.c b/drivers/tty/serial/sirfsoc_uart.c
index 9925b00a97772a1b..701df3319ff7579d 100644
--- a/drivers/tty/serial/sirfsoc_uart.c
+++ b/drivers/tty/serial/sirfsoc_uart.c
@@ -1283,6 +1283,11 @@ static int sirfsoc_uart_probe(struct platform_device *pdev)
 		goto err;
 	}
 	sirfport->port.line = of_alias_get_id(np, "serial");
+	if (sirfport->port.line >= SIRFSOC_UART_NR) {
+		dev_err(&pdev->dev, "serial%d out of range\n",
+			sirfport->port.line);
+		return -EINVAL;
+	}
 	sirf_ports[sirfport->port.line] = sirfport;
 	sirfport->port.iotype = UPIO_MEM;
 	sirfport->port.flags = UPF_BOOT_AUTOCONF;
-- 
2.7.4

Re: [PATCH 8/9] serial: sirf: Fix out-of-bounds access through DT alias

[Geert Uytterhoeven]

On Tue, Feb 20, 2018 at 10:40 AM, Geert Uytterhoeven
<geert+renesas@glider.be> wrote:
> The sirf_ports[] array is indexed using a value derived from the
> "serialN" alias in DT, which may lead to an out-of-bounds access.
>
> Fix this by adding a range check.
>
> Fixes: 66c7ab1120585d18 ("serial: sirf: Fix out-of-bounds access through DT alias")
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

Fixes: a6ffe8966acbb66b ("serial: sirf: use dynamic method allocate
uart structure")

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[PATCH 9/9] serial: xuartps: Fix out-of-bounds access through DT alias

[Geert Uytterhoeven]

The cdns_uart_port[] array is indexed using a value derived from the
"serialN" alias in DT, which may lead to an out-of-bounds access.

Fix this by adding a range check.

Fixes: 1f118c02a1819856 ("serial: xuartps: Fix out-of-bounds access through DT alias")
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 drivers/tty/serial/xilinx_uartps.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/tty/serial/xilinx_uartps.c b/drivers/tty/serial/xilinx_uartps.c
index b9b2bc76bcac606c..abcb4d09a2d866d0 100644
--- a/drivers/tty/serial/xilinx_uartps.c
+++ b/drivers/tty/serial/xilinx_uartps.c
@@ -1110,7 +1110,7 @@ static struct uart_port *cdns_uart_get_port(int id)
 	struct uart_port *port;
 
 	/* Try the given port id if failed use default method */
-	if (cdns_uart_port[id].mapbase != 0) {
+	if (id < CDNS_UART_NR_PORTS && cdns_uart_port[id].mapbase != 0) {
 		/* Find the next unused port */
 		for (id = 0; id < CDNS_UART_NR_PORTS; id++)
 			if (cdns_uart_port[id].mapbase == 0)
-- 
2.7.4

Re: [PATCH 9/9] serial: xuartps: Fix out-of-bounds access through DT alias

[Michal Simek]

On 20.2.2018 10:40, Geert Uytterhoeven wrote:
> The cdns_uart_port[] array is indexed using a value derived from the
> "serialN" alias in DT, which may lead to an out-of-bounds access.
> 
> Fix this by adding a range check.
> 
> Fixes: 1f118c02a1819856 ("serial: xuartps: Fix out-of-bounds access through DT alias")

I didn't find this sha1 - patch name is this one.

> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
>  drivers/tty/serial/xilinx_uartps.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/serial/xilinx_uartps.c b/drivers/tty/serial/xilinx_uartps.c
> index b9b2bc76bcac606c..abcb4d09a2d866d0 100644
> --- a/drivers/tty/serial/xilinx_uartps.c
> +++ b/drivers/tty/serial/xilinx_uartps.c
> @@ -1110,7 +1110,7 @@ static struct uart_port *cdns_uart_get_port(int id)
>  	struct uart_port *port;
>  
>  	/* Try the given port id if failed use default method */
> -	if (cdns_uart_port[id].mapbase != 0) {
> +	if (id < CDNS_UART_NR_PORTS && cdns_uart_port[id].mapbase != 0) {
>  		/* Find the next unused port */
>  		for (id = 0; id < CDNS_UART_NR_PORTS; id++)
>  			if (cdns_uart_port[id].mapbase == 0)
> 

Below should be better fix for this driver.

Thanks,
Michal

diff --git a/drivers/tty/serial/xilinx_uartps.c
b/drivers/tty/serial/xilinx_uartps.c
index b9b2bc76bcac..b77c6477ed93 100644
--- a/drivers/tty/serial/xilinx_uartps.c
+++ b/drivers/tty/serial/xilinx_uartps.c
@@ -1109,6 +1109,9 @@ static struct uart_port *cdns_uart_get_port(int id)
 {
        struct uart_port *port;

+       if (id >= CDNS_UART_NR_PORTS)
+               return NULL;
+
        /* Try the given port id if failed use default method */
        if (cdns_uart_port[id].mapbase != 0) {
                /* Find the next unused port */
@@ -1117,9 +1120,6 @@ static struct uart_port *cdns_uart_get_port(int id)
                                break;
        }

-       if (id >= CDNS_UART_NR_PORTS)
-               return NULL;
-
        port = &cdns_uart_port[id];

        /* At this point, we've got an empty uart_port struct,
initialize it */

Re: [PATCH 9/9] serial: xuartps: Fix out-of-bounds access through DT alias

[Geert Uytterhoeven]

Hi Michal,

On Tue, Feb 20, 2018 at 11:22 AM, Michal Simek <michal.simek@xilinx.com> wrote:
> On 20.2.2018 10:40, Geert Uytterhoeven wrote:
>> The cdns_uart_port[] array is indexed using a value derived from the
>> "serialN" alias in DT, which may lead to an out-of-bounds access.
>>
>> Fix this by adding a range check.
>>
>> Fixes: 1f118c02a1819856 ("serial: xuartps: Fix out-of-bounds access through DT alias")
>
> I didn't find this sha1 - patch name is this one.

Bummer, I totally screwed up my scripting...

Fixes: 928e9263492069ee ("tty: xuartps: Initialize ports according to aliases")

>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
>> ---
>>  drivers/tty/serial/xilinx_uartps.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/tty/serial/xilinx_uartps.c b/drivers/tty/serial/xilinx_uartps.c
>> index b9b2bc76bcac606c..abcb4d09a2d866d0 100644
>> --- a/drivers/tty/serial/xilinx_uartps.c
>> +++ b/drivers/tty/serial/xilinx_uartps.c
>> @@ -1110,7 +1110,7 @@ static struct uart_port *cdns_uart_get_port(int id)
>>       struct uart_port *port;
>>
>>       /* Try the given port id if failed use default method */
>> -     if (cdns_uart_port[id].mapbase != 0) {
>> +     if (id < CDNS_UART_NR_PORTS && cdns_uart_port[id].mapbase != 0) {
>>               /* Find the next unused port */
>>               for (id = 0; id < CDNS_UART_NR_PORTS; id++)
>>                       if (cdns_uart_port[id].mapbase == 0)
>>
>
> Below should be better fix for this driver.

I considered that, too, but...

> --- a/drivers/tty/serial/xilinx_uartps.c
> +++ b/drivers/tty/serial/xilinx_uartps.c
> @@ -1109,6 +1109,9 @@ static struct uart_port *cdns_uart_get_port(int id)
>  {
>         struct uart_port *port;
>
> +       if (id >= CDNS_UART_NR_PORTS)
> +               return NULL;
> +
>         /* Try the given port id if failed use default method */
>         if (cdns_uart_port[id].mapbase != 0) {
>                 /* Find the next unused port */
> @@ -1117,9 +1120,6 @@ static struct uart_port *cdns_uart_get_port(int id)
>                                 break;
>         }
>
> -       if (id >= CDNS_UART_NR_PORTS)
> -               return NULL;
> -

... the above check cannot be removed, as it is needed to support the loop
above to find an unused port.

>         port = &cdns_uart_port[id];
>
>         /* At this point, we've got an empty uart_port struct,
> initialize it */

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 9/9] serial: xuartps: Fix out-of-bounds access through DT alias

[Michal Simek]

On 20.2.2018 11:38, Geert Uytterhoeven wrote:
> Hi Michal,
> 
> On Tue, Feb 20, 2018 at 11:22 AM, Michal Simek <michal.simek@xilinx.com> wrote:
>> On 20.2.2018 10:40, Geert Uytterhoeven wrote:
>>> The cdns_uart_port[] array is indexed using a value derived from the
>>> "serialN" alias in DT, which may lead to an out-of-bounds access.
>>>
>>> Fix this by adding a range check.
>>>
>>> Fixes: 1f118c02a1819856 ("serial: xuartps: Fix out-of-bounds access through DT alias")
>>
>> I didn't find this sha1 - patch name is this one.
> 
> Bummer, I totally screwed up my scripting...
> 
> Fixes: 928e9263492069ee ("tty: xuartps: Initialize ports according to aliases")
> 
>>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
>>> ---
>>>  drivers/tty/serial/xilinx_uartps.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/tty/serial/xilinx_uartps.c b/drivers/tty/serial/xilinx_uartps.c
>>> index b9b2bc76bcac606c..abcb4d09a2d866d0 100644
>>> --- a/drivers/tty/serial/xilinx_uartps.c
>>> +++ b/drivers/tty/serial/xilinx_uartps.c
>>> @@ -1110,7 +1110,7 @@ static struct uart_port *cdns_uart_get_port(int id)
>>>       struct uart_port *port;
>>>
>>>       /* Try the given port id if failed use default method */
>>> -     if (cdns_uart_port[id].mapbase != 0) {
>>> +     if (id < CDNS_UART_NR_PORTS && cdns_uart_port[id].mapbase != 0) {
>>>               /* Find the next unused port */
>>>               for (id = 0; id < CDNS_UART_NR_PORTS; id++)
>>>                       if (cdns_uart_port[id].mapbase == 0)
>>>
>>
>> Below should be better fix for this driver.
> 
> I considered that, too, but...
> 
>> --- a/drivers/tty/serial/xilinx_uartps.c
>> +++ b/drivers/tty/serial/xilinx_uartps.c
>> @@ -1109,6 +1109,9 @@ static struct uart_port *cdns_uart_get_port(int id)
>>  {
>>         struct uart_port *port;
>>
>> +       if (id >= CDNS_UART_NR_PORTS)
>> +               return NULL;
>> +
>>         /* Try the given port id if failed use default method */
>>         if (cdns_uart_port[id].mapbase != 0) {
>>                 /* Find the next unused port */
>> @@ -1117,9 +1120,6 @@ static struct uart_port *cdns_uart_get_port(int id)
>>                                 break;
>>         }
>>
>> -       if (id >= CDNS_UART_NR_PORTS)
>> -               return NULL;
>> -
> 
> ... the above check cannot be removed, as it is needed to support the loop
> above to find an unused port.

You are right.
I have checked 4 patches I have sent in past which didn't reach mainline
(probably because of RFC)
Take a look at
https://www.spinics.net/lists/linux-serial/msg27106.html

I have removed cdns_uart_port array completely there.

Thanks,
Michal

Re: [PATCH 9/9] serial: xuartps: Fix out-of-bounds access through DT alias

[Geert Uytterhoeven]

Hi Michal,

On Tue, Feb 20, 2018 at 12:27 PM, Michal Simek <michal.simek@xilinx.com> wrote:
> On 20.2.2018 11:38, Geert Uytterhoeven wrote:
>> On Tue, Feb 20, 2018 at 11:22 AM, Michal Simek <michal.simek@xilinx.com> wrote:
>>> On 20.2.2018 10:40, Geert Uytterhoeven wrote:
>>>> The cdns_uart_port[] array is indexed using a value derived from the
>>>> "serialN" alias in DT, which may lead to an out-of-bounds access.
>>>>
>>>> Fix this by adding a range check.

> I have checked 4 patches I have sent in past which didn't reach mainline
> (probably because of RFC)
> Take a look at
> https://www.spinics.net/lists/linux-serial/msg27106.html
>
> I have removed cdns_uart_port array completely there.

Nice! I'd love to get rid of fixed arrays in serial...

However, IMHO it's still worthwhile to fix the out-of-bounds access first,
as that fix can be backported to stable kernels easily.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 9/9] serial: xuartps: Fix out-of-bounds access through DT alias

[Michal Simek]

On 20.2.2018 13:27, Geert Uytterhoeven wrote:
> Hi Michal,
> 
> On Tue, Feb 20, 2018 at 12:27 PM, Michal Simek <michal.simek@xilinx.com> wrote:
>> On 20.2.2018 11:38, Geert Uytterhoeven wrote:
>>> On Tue, Feb 20, 2018 at 11:22 AM, Michal Simek <michal.simek@xilinx.com> wrote:
>>>> On 20.2.2018 10:40, Geert Uytterhoeven wrote:
>>>>> The cdns_uart_port[] array is indexed using a value derived from the
>>>>> "serialN" alias in DT, which may lead to an out-of-bounds access.
>>>>>
>>>>> Fix this by adding a range check.
> 
>> I have checked 4 patches I have sent in past which didn't reach mainline
>> (probably because of RFC)
>> Take a look at
>> https://www.spinics.net/lists/linux-serial/msg27106.html
>>
>> I have removed cdns_uart_port array completely there.
> 
> Nice! I'd love to get rid of fixed arrays in serial...
> 
> However, IMHO it's still worthwhile to fix the out-of-bounds access first,
> as that fix can be backported to stable kernels easily.

I agree with you. Not a problem with your patch and for me it won't be
problem to rebase.

I would love to get rid of CDNS_UART_NR_PORTS but unfortunately this is
passed to core via .nr.

Thanks,
Michal