Patchwork [arm-devs,v1,1/1] char/cadence_uart: Fix reset for unattached instances

login
register
mail settings
Submitter Peter Crosthwaite
Date June 7, 2013, 4:02 a.m.
Message ID <9009578ee10a50d994b2e10aa2840d73765f5968.1370577272.git.peter.crosthwaite@xilinx.com>
Download mbox | patch
Permalink /patch/249592/
State New
Headers show

Comments

Peter Crosthwaite - June 7, 2013, 4:02 a.m.
From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

commit 1db8b5efe0c2b5000e50691eea61264a615f43de introduced an issue
where QEMU would segfault if you have an unattached Cadence UART.

Fix by guarding the flush-on-reset logic on there being a qemu_chr
attachment.

Reported-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 hw/char/cadence_uart.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
Soren Brinkmann - June 7, 2013, 9:45 p.m.
On Fri, Jun 07, 2013 at 02:02:43PM +1000, peter.crosthwaite@xilinx.com wrote:
> From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> 
> commit 1db8b5efe0c2b5000e50691eea61264a615f43de introduced an issue
> where QEMU would segfault if you have an unattached Cadence UART.
> 
> Fix by guarding the flush-on-reset logic on there being a qemu_chr
> attachment.
> 
> Reported-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
Tested-by: Soren Brinkmann <soren.brinkmann@xilinx.com>

	Thanks,
	Sören
Peter Maydell - June 25, 2013, 5:16 p.m.
On 7 June 2013 05:02,  <peter.crosthwaite@xilinx.com> wrote:
> From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>
> commit 1db8b5efe0c2b5000e50691eea61264a615f43de introduced an issue
> where QEMU would segfault if you have an unattached Cadence UART.
>
> Fix by guarding the flush-on-reset logic on there being a qemu_chr
> attachment.
>
> Reported-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> ---
>
>  hw/char/cadence_uart.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/hw/char/cadence_uart.c b/hw/char/cadence_uart.c
> index c2a7834..29827d2 100644
> --- a/hw/char/cadence_uart.c
> +++ b/hw/char/cadence_uart.c
> @@ -157,7 +157,9 @@ static void uart_rx_reset(UartState *s)
>  {
>      s->rx_wpos = 0;
>      s->rx_count = 0;
> -    qemu_chr_accept_input(s->chr);
> +    if (s->chr) {
> +        qemu_chr_accept_input(s->chr);
> +    }

There seem to be a lot of references to s->chr in this device:
is this really the only one which needs a guard against NULL?

thanks
-- PMM
Peter Crosthwaite - June 26, 2013, 12:41 a.m.
Hi Peter,

On Wed, Jun 26, 2013 at 3:16 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 7 June 2013 05:02,  <peter.crosthwaite@xilinx.com> wrote:
>> From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>>
>> commit 1db8b5efe0c2b5000e50691eea61264a615f43de introduced an issue
>> where QEMU would segfault if you have an unattached Cadence UART.
>>
>> Fix by guarding the flush-on-reset logic on there being a qemu_chr
>> attachment.
>>
>> Reported-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
>> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>> ---
>>
>>  hw/char/cadence_uart.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/char/cadence_uart.c b/hw/char/cadence_uart.c
>> index c2a7834..29827d2 100644
>> --- a/hw/char/cadence_uart.c
>> +++ b/hw/char/cadence_uart.c
>> @@ -157,7 +157,9 @@ static void uart_rx_reset(UartState *s)
>>  {
>>      s->rx_wpos = 0;
>>      s->rx_count = 0;
>> -    qemu_chr_accept_input(s->chr);
>> +    if (s->chr) {
>> +        qemu_chr_accept_input(s->chr);
>> +    }
>
> There seem to be a lot of references to s->chr in this device:
> is this really the only one which needs a guard against NULL?
>

Actually no,

We have had some corner cases where other dereferences have segfaulted
on us too. On my todo list to sweep and fix globally.

But this is unique and a much bigger problem than the rest, as it is
in the reset path. So even if the uart is present in system and unused
it segafaults.

Regards,
Peter

> thanks
> -- PMM
>
Peter Maydell - June 27, 2013, 4:20 p.m.
On 7 June 2013 05:02,  <peter.crosthwaite@xilinx.com> wrote:
> From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>
> commit 1db8b5efe0c2b5000e50691eea61264a615f43de introduced an issue
> where QEMU would segfault if you have an unattached Cadence UART.
>
> Fix by guarding the flush-on-reset logic on there being a qemu_chr
> attachment.
>
> Reported-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

Applied to arm-devs.next.

thanks
-- PMM

Patch

diff --git a/hw/char/cadence_uart.c b/hw/char/cadence_uart.c
index c2a7834..29827d2 100644
--- a/hw/char/cadence_uart.c
+++ b/hw/char/cadence_uart.c
@@ -157,7 +157,9 @@  static void uart_rx_reset(UartState *s)
 {
     s->rx_wpos = 0;
     s->rx_count = 0;
-    qemu_chr_accept_input(s->chr);
+    if (s->chr) {
+        qemu_chr_accept_input(s->chr);
+    }
 
     s->r[R_SR] |= UART_SR_INTR_REMPTY;
     s->r[R_SR] &= ~UART_SR_INTR_RFUL;