Re: [PATCH] serial: fix parisc boot hang

[James Bottomley <James.Bottomley@HansenPartnership.com>]

On Sun, 2015-01-04 at 15:41 -0500, John David Anglin wrote:
> On 2015-01-04, at 2:12 PM, James Bottomley wrote:
> 
> > On Fri, 2015-01-02 at 10:51 -0800, Greg Kroah-Hartman wrote:
> >> On Fri, Jan 02, 2015 at 10:05:13AM -0800, James Bottomley wrote:
> >>> From: James Bottomley <JBottomley@Parallels.com>
> >>> 
> >>> This is a partial revert of 2f2dafe (serial: serial_core.c: printk
> >>> replacement) which gets us booting again.  The real problem seems to be
> >>> the _emit path in early boot.  However, until we can root cause it, we
> >>> need at least to get boot working.
> >>> 
> >>> Fixes: 2f2dafe77df2c78e189a9fa6b1879dffd06ae5a1
> >>> Cc: stable@vger.kernel.org
> >>> Signed-off-by: James Bottomley <JBottomley@Parallels.com>
> >>> 
> >>> ---
> >>> 
> >>> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> >>> index 57ca61b..984605b 100644
> >>> --- a/drivers/tty/serial/serial_core.c
> >>> +++ b/drivers/tty/serial/serial_core.c
> >>> @@ -2164,7 +2164,9 @@ uart_report_port(struct uart_driver *drv, struct uart_port *port)
> >>> 		break;
> >>> 	}
> >>> 
> >>> -	dev_info(port->dev, "%s%d at %s (irq = %d, base_baud = %d) is a %s\n",
> >>> +	printk(KERN_INFO "%s%s%s%d at %s (irq = %d, base_baud = %d) is a %s\n",
> >>> +	       port->dev ? dev_name(port->dev) : "",
> >>> +	       port->dev ? ": " : "",
> >>> 	       drv->dev_name,
> >>> 	       drv->tty_driver->name_base + port->line,
> >>> 	       address, port->irq, port->uartclk / 16, uart_type(port));
> >> 
> >> Very odd, but I'll go queue it up, thanks.
> > 
> > OK, well this turned out to be one of the weirder fishing expeditions
> > I've been on.  The problem is this strange linux specific printf format
> > flag %pV.  The way to fix the bug is not to indirect the dev_xxx printks
> > via %pV.  What's happening is that in some circumstances, using %pV
> > corrupts the stack.
> > 
> > The reason seems to be that whoever came up with %pV didn't read the man
> > pages carefully enough. In all the examples and use cases, the va_list
> > is passed by *copy* not by reference.  For some inexplicable reason it's
> > passed by reference in struct va_format.  Sure enough when I fix up my
> > local tree to pass by copy it all works (at least as far as I can tell:
> > most of the time the stack corruption passes unnoticed and minor
> > disturbances can affect that.  However, the type and size of the va_list
> > is the same in reference and copy, so I think it's reasonably
> > definitive).
> > 
> > I'd really like one of our gcc experts to comment here because all of
> > these are builtin_ types and functions, so why there's a problem is a
> > mystery (translate: I don't understand enough of gcc to make sense of
> > the source code), but the surmise would be that the builtins are taking
> > some stack frame information from the source and, because it's a pointer
> > not a copy, it's in the wrong frame.
> > 
> > Assuming this turns out to be the problem, fixing it is going to be a
> > real bugger because on most platforms, the type of va_list is void *
> > meaning you can't tell the difference at compile time between a copy and
> > a reference, because typeof(void *) == typeof(void **),  and this %pV is
> > sprayed all over our code base.
> > 
> > We should probably also have the security experts look it over because
> > any way of inducing stack frame corruption is potentially exploitable,
> > although, in this case, I think all of the uses are internal so the user
> > doesn't have the ability to influence the source data.
> 
> 
> Would it be possible to create a relatively simple test case?

Unfortunately not.  I'm no longer even sure this is the root cause: it
reproduced again, even passing va_list by copy.

James

> On 64-bit parisc, the va_list is built up in part by
> hppa_builtin_saveregs() in pa.c.
> It's stored in a region in the callers frame reserved for arguments.
> 
> I tend to think it's dangerous to pass a va_list by reference as in
> general the called
> function is free to use the incoming argument region for its own
> purposes after
> the incoming arguments have been copied.  This might be the cause of
> the stack
> corruption you are seeing.
> 
> On parisc, the callee is responsible for copying incoming arguments if
> necessary.
> This is somewhat unusual...
> 
> Dave
> --
> John David Anglin	dave.anglin@bell.net
> 
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-parisc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>