Peter Hurley <peter@xxxxxxxxxxxxxxxxxx> writes:
>> 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));
> }
I think I like this one better.
--
Måns Rullgård
mans@xxxxxxxxx
--
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
