On 03/09/2015 09:22 AM, Peter Hurley wrote:
> On 03/09/2015 07:43 AM, Måns Rullgård wrote:
>> Peter Hurley <peter@xxxxxxxxxxxxxxxxxx> writes:
>>
>>> Au1x00/RT2800+ hardware has an alternate register layout which is
>>> remapped with lookup tables by the au_serial_in()/out() i/o accessors.
>>> However, the h/w does not support the complete 8250 register set, and
>>> accesses to unmapped registers cause out-of-bounds lookups. Further,
>>> because the lookup tables are defined by designated initializers, the
>>> tables may contain unmapped entries (although the current tables do not).
>>>
>>> Add the AU_MAP() macro to distinguish mapped and unmapped register
>>> lookups (unmapped register lookups (initialized to 0) result in negative
>>> register offsets). Validate the register index (ie., 'offset') is in the
>>> range [0, table size). Return fixed value for unmapped register reads and
>>> ignore unmapped register writes.
>>>
>>> Reported-by: Mason <slash.tmp@xxxxxxx>
>>> Signed-off-by: Peter Hurley <peter@xxxxxxxxxxxxxxxxxx>
>>> ---
>>> drivers/tty/serial/8250/8250_core.c | 41 ++++++++++++++++++++++---------------
>>> 1 file changed, 25 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
>>> index 5eb95fd..87df908 100644
>>> --- a/drivers/tty/serial/8250/8250_core.c
>>> +++ b/drivers/tty/serial/8250/8250_core.c
>>> @@ -358,34 +358,43 @@ static void default_serial_dl_write(struct uart_8250_port *up, int value)
>>> #if defined(CONFIG_MIPS_ALCHEMY) || defined(CONFIG_SERIAL_8250_RT288X)
>>>
>>> /* Au1x00/RT288x UART hardware has a weird register layout */
>>> +#define AU_MAP(x) (0x80 + x)
>>> static const u8 au_io_in_map[] = {
>>> - [UART_RX] = 0,
>>> - [UART_IER] = 2,
>>> - [UART_IIR] = 3,
>>> - [UART_LCR] = 5,
>>> - [UART_MCR] = 6,
>>> - [UART_LSR] = 7,
>>> - [UART_MSR] = 8,
>>> + [UART_RX] = AU_MAP(0),
>>> + [UART_IER] = AU_MAP(2),
>>> + [UART_IIR] = AU_MAP(3),
>>> + [UART_LCR] = AU_MAP(5),
>>> + [UART_MCR] = AU_MAP(6),
>>> + [UART_LSR] = AU_MAP(7),
>>> + [UART_MSR] = AU_MAP(8),
>>> };
>>>
>>> static const u8 au_io_out_map[] = {
>>> - [UART_TX] = 1,
>>> - [UART_IER] = 2,
>>> - [UART_FCR] = 4,
>>> - [UART_LCR] = 5,
>>> - [UART_MCR] = 6,
>>> + [UART_TX] = AU_MAP(1),
>>> + [UART_IER] = AU_MAP(2),
>>> + [UART_FCR] = AU_MAP(4),
>>> + [UART_LCR] = AU_MAP(5),
>>> + [UART_MCR] = AU_MAP(6),
>>> };
>>>
>>> static unsigned int au_serial_in(struct uart_port *p, int offset)
>>> {
>>> - offset = au_io_in_map[offset] << p->regshift;
>>> - return __raw_readl(p->membase + offset);
>>> + if (offset >= ARRAYSIZE(au_io_in_map))
>>> + return UINT_MAX;
>>> + offset = au_io_in_map[offset] - AU_MAP(0);
>>> + if (offset < 0)
>>> + return UINT_MAX;
>>> + return __raw_readl(p->membase + (offset << p->regshift));
>>> }
>>
>> Maybe the intent would be clearer if you used bitwise operations instead
>> of arithmetic:
>>
>> #define AU_MAP(x) (0x80 | (x))
>> [...]
>> offset = au_io_in_map[offset];
>> if (!(offset & 0x80))
>> return UINT_MAX;
>> offset &= 0x7f;
>> return __raw_readl(...);
>>
>
> That's how I originally wrote it, but I'm not sure that's any better.
>
> Perhaps a better way to do this would be to explicitly declare the
> table sizes for the complete 8250 register set and specifically
> initialize the unmapped registers to -1.
>
> That would do away with AU_MAP() and arithmetic, while it would still
> make it more obvious that unmapped registers need specific initialization
> and would cause compile errors if an initializer extended the register
> map.
FWIW, that patch would look like this:
--- >% ---
From: Peter Hurley <peter@xxxxxxxxxxxxxxxxxx>
Subject: [PATCH] serial: 8250: Validate reg addr for Au1x00/RT288x i/o
accessors
Au1x00/RT2800+ hardware has an alternate register layout which is
remapped with lookup tables by the au_serial_in()/out() i/o accessors.
However, the h/w does not support the complete 8250 register set, and
accesses to unmapped registers cause out-of-bounds lookups. Further,
because the lookup tables are defined by designated initializers, the
tables may contain unmapped entries (although the current tables do not).
Declare fixed-size lookup tables with contiguous initialization for
the complete 8250 register map; unmapped registers are initialized to -1.
Validate the register index (ie., 'offset') is in the range [0, table size).
Return fixed value for unmapped register reads and ignore unmapped register
writes.
Reported-by: Mason <slash.tmp@xxxxxxx>
Signed-off-by: Peter Hurley <peter@xxxxxxxxxxxxxxxxxx>
---
drivers/tty/serial/8250/8250_core.c | 48 +++++++++++++++++++++++--------------
1 file changed, 30 insertions(+), 18 deletions(-)
diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
index 5eb95fd..bfff624 100644
--- a/drivers/tty/serial/8250/8250_core.c
+++ b/drivers/tty/serial/8250/8250_core.c
@@ -358,34 +358,46 @@ static void default_serial_dl_write(struct uart_8250_port *up, int value)
#if defined(CONFIG_MIPS_ALCHEMY) || defined(CONFIG_SERIAL_8250_RT288X)
/* Au1x00/RT288x UART hardware has a weird register layout */
-static const u8 au_io_in_map[] = {
- [UART_RX] = 0,
- [UART_IER] = 2,
- [UART_IIR] = 3,
- [UART_LCR] = 5,
- [UART_MCR] = 6,
- [UART_LSR] = 7,
- [UART_MSR] = 8,
+static const s8 au_io_in_map[8] = {
+ 0, /* UART_RX */
+ 2, /* UART_IER */
+ 3, /* UART_IIR */
+ 5, /* UART_LCR */
+ 6, /* UART_MCR */
+ 7, /* UART_LSR */
+ 8, /* UART_MSR */
+ -1, /* UART_SCR (unmapped) */
};
-static const u8 au_io_out_map[] = {
- [UART_TX] = 1,
- [UART_IER] = 2,
- [UART_FCR] = 4,
- [UART_LCR] = 5,
- [UART_MCR] = 6,
+static const s8 au_io_out_map[8] = {
+ 1, /* UART_TX */
+ 2, /* UART_IER */
+ 4, /* UART_FCR */
+ 5, /* UART_LCR */
+ 6, /* UART_MCR */
+ -1, /* UART_LSR (unmapped) */
+ -1, /* UART_MSR (unmapped) */
+ -1, /* UART_SCR (unmapped) */
};
static unsigned int au_serial_in(struct uart_port *p, int offset)
{
- offset = au_io_in_map[offset] << p->regshift;
- return __raw_readl(p->membase + offset);
+ if (offset >= ARRAYSIZE(au_io_in_map))
+ return UINT_MAX;
+ offset = au_io_in_map[offset];
+ if (offset < 0)
+ return UINT_MAX;
+ return __raw_readl(p->membase + (offset << p->regshift));
}
static void au_serial_out(struct uart_port *p, int offset, int value)
{
- offset = au_io_out_map[offset] << p->regshift;
- __raw_writel(value, p->membase + offset);
+ if (offset >= ARRAYSIZE(au_io_out_map))
+ return;
+ offset = au_io_out_map[offset];
+ if (offset < 0)
+ return;
+ __raw_writel(value, p->membase + (offset << p->regshift));
}
/* Au1x00 haven't got a standard divisor latch */
--
2.3.1
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
