linux-kernel - Re: [PATCH] serial: Initialize spinlocks in 8250 and don't clobber them.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <20081020141750.d0610586.akpm@linux-foundation.org>
Date:	Mon, 20 Oct 2008 14:17:50 -0700
From:	Andrew Morton <akpm@...ux-foundation.org>
To:	David Daney <ddaney@...iumnetworks.com>
Cc:	linux-serial@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-mips@...ux-mips.org, Tomaso.Paoletti@...iumnetworks.com
Subject: Re: [PATCH] serial: Initialize spinlocks in 8250 and don't clobber
 them.

On Tue, 14 Oct 2008 14:37:24 -0700
David Daney <ddaney@...iumnetworks.com> wrote:

> Initialize spinlocks in 8250 and don't clobber them.

That's actually quite bad.  There's no reason why an all-zeroes pattern
for a spinlock_t correctly represents the unlocked state.  I guess we
got lucky on the architectures which use this code.

> Spinlock debugging fails in 8250.c because the lock fields in
> irq_lists are not initialized.  Initialize them.
> 
> In serial8250_isa_init_ports(), the port's lock is initialized.  We
> should not overwrite it.  Only copy in the fields we need.
> 
> Signed-off-by: David Daney <ddaney@...iumnetworks.com>
> Signed-off-by: Tomaso Paoletti <tpaoletti@...iumnetworks.com>
> ---
>  drivers/serial/8250.c |   19 +++++++++++++++++--
>  1 files changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/serial/8250.c b/drivers/serial/8250.c
> index d4104a3..0688799 100644
> --- a/drivers/serial/8250.c
> +++ b/drivers/serial/8250.c
> @@ -2494,6 +2494,9 @@ static void __init serial8250_isa_init_ports(void)
>  		return;
>  	first = 0;
>  
> +	for (i = 0; i < ARRAY_SIZE(irq_lists); i++)
> +		spin_lock_init(&irq_lists[i].lock);

OK..  But serial8250_isa_init_ports() has so many callsites that I'd
worry that we end up running this initialisation multiple times.  Say,
if the right combination of boot options is provided?  This is probably
a benign thing, but it's not desirable.

A simple "fix" would be

static void __init irq_lists_init(void)
{
	static unsigned long done;

	if (!test_and_set_bit(0, &done)) {
		int i;

		for (i = 0; i < ARRAY_SIZE(irq_lists); i++)
			spin_lock_init(&irq_lists[i].lock);
	}
}

A better fix would be to initialise all those spinlocks at compile
time.  But given the need to pass the address of each lock into each
lock's initialiser, that could be tricky.

>  	for (i = 0; i < nr_uarts; i++) {
>  		struct uart_8250_port *up = &serial8250_ports[i];
>  
> @@ -2699,12 +2702,24 @@ static struct uart_driver serial8250_reg = {
>   */
>  int __init early_serial_setup(struct uart_port *port)
>  {
> +	struct uart_port *p;
> +
>  	if (port->line >= ARRAY_SIZE(serial8250_ports))
>  		return -ENODEV;
>  
>  	serial8250_isa_init_ports();
> -	serial8250_ports[port->line].port	= *port;
> -	serial8250_ports[port->line].port.ops	= &serial8250_pops;
> +	p = &serial8250_ports[port->line].port;
> +	p->iobase       = port->iobase;
> +	p->membase      = port->membase;
> +	p->irq          = port->irq;
> +	p->uartclk      = port->uartclk;
> +	p->fifosize     = port->fifosize;
> +	p->regshift     = port->regshift;
> +	p->iotype       = port->iotype;
> +	p->flags        = port->flags;
> +	p->mapbase      = port->mapbase;
> +	p->private_data = port->private_data;
> +	p->ops		= &serial8250_pops;
>  	return 0;
>  }

Having to spell out each member like this is pretty nasty from a
maintainability point of view.  If new fields are added to uart_port,
we surely will forget to update this code.

But yes, copying a spinlock by value is quite wrong.  Perhaps we could
retain the struct assigment and then run spin_lock_init() to get the
spinlock into a sane state?



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/