Re: [Qemu-devel] [PATCH v3] char: cadence: check baud rate generator and divider values

[P J P]

+-- On Thu, 27 Oct 2016, Alistair Francis wrote --+
| No, they will just remain at whatever they were previously set to.
| Which should always be valid.

  I see. Sent patch v4.

Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

Re: [Qemu-devel] [PATCH v3] char: cadence: check baud rate generator and divider values

[P J P]

+-- On Thu, 27 Oct 2016, Alistair Francis wrote --+
| > @Alistair: @Edgar:
| >Would it be possible for you to fix the migration part? I'm preparing for
| > couple weeks leave from Monday 31'st, so trying to close other tasks. I'd
| > really appreciate if you could take it over.
| 
| Yeah, I can take this over.

Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

Re: [Qemu-devel] [PATCH v3] char: cadence: check baud rate generator and divider values

[Alistair Francis]

On Wed, Oct 26, 2016 at 11:35 PM, P J P  wrote:
> +-- On Wed, 26 Oct 2016, Peter Maydell wrote --+
> | You're relying on the register values never being
> | invalid to avoid the divide by zero, which means
> | you need to check them post-migration too.
>
> @Alistair: @Edgar:
>Would it be possible for you to fix the migration part? I'm preparing for
> couple weeks leave from Monday 31'st, so trying to close other tasks. I'd
> really appreciate if you could take it over.

Yeah, I can take this over.

Thanks,

Alistair

>
> Thank you.
> --
> Prasad J Pandit / Red Hat Product Security Team
> 47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
>

Re: [Qemu-devel] [PATCH v3] char: cadence: check baud rate generator and divider values

[Alistair Francis]

On Wed, Oct 26, 2016 at 11:29 PM, P J P  wrote:
> +-- On Wed, 26 Oct 2016, Alistair Francis wrote --+
> | >   * Device model for Cadence UART
> | > + *   -> 
> http://www.xilinx.com/support/documentation/user_guides/ug585-Zynq-7000-TRM.pdf
> |
> | Nit pick, I would put the URL under the title below.
>
>   Okay.
>
> | > +case R_BRGR: /* Baud rate generator */
> | > +s->r[offset] = 0x028B; /* default reset value */
> |
> | Why do we still have the reset value here, I thought we were just
> | ignoring the invalid writes? You don't need to reset it.
>
>   Wouldn't that leave the registers undefined ?

No, they will just remain at whatever they were previously set to.
Which should always be valid.

Thanks,

Alistair

>
> Thank you.
> --
> Prasad J Pandit / Red Hat Product Security Team
> 47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
>

Re: [Qemu-devel] [PATCH v3] char: cadence: check baud rate generator and divider values

[P J P]

+-- On Wed, 26 Oct 2016, Peter Maydell wrote --+
| You're relying on the register values never being
| invalid to avoid the divide by zero, which means
| you need to check them post-migration too.

@Alistair: @Edgar:
   Would it be possible for you to fix the migration part? I'm preparing for 
couple weeks leave from Monday 31'st, so trying to close other tasks. I'd 
really appreciate if you could take it over.

Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

Re: [Qemu-devel] [PATCH v3] char: cadence: check baud rate generator and divider values

[P J P]

+-- On Wed, 26 Oct 2016, Alistair Francis wrote --+
| >   * Device model for Cadence UART
| > + *   -> 
http://www.xilinx.com/support/documentation/user_guides/ug585-Zynq-7000-TRM.pdf
| 
| Nit pick, I would put the URL under the title below.

  Okay.
 
| > +case R_BRGR: /* Baud rate generator */
| > +s->r[offset] = 0x028B; /* default reset value */
| 
| Why do we still have the reset value here, I thought we were just
| ignoring the invalid writes? You don't need to reset it.

  Wouldn't that leave the registers undefined ?

Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

Re: [Qemu-devel] [PATCH v3] char: cadence: check baud rate generator and divider values

[Peter Maydell]

On 26 October 2016 at 22:22, P J P  wrote:
> From: Prasad J Pandit 
>
> The Cadence UART device emulator calculates speed by dividing the
> baud rate by a 'baud rate generator' & 'baud rate divider' value.
> The device specification defines these register values to be
> non-zero and within certain limits. Add checks for these limits
> to avoid errors like divide by zero.
>
> Reported-by: Huawei PSIRT 
> Signed-off-by: Prasad J Pandit 
> ---
>  hw/char/cadence_uart.c | 17 +
>  1 file changed, 17 insertions(+)
>
> Update: mask R_BRGR and R_BDIV register values with 0x and 0xff resp.
>   -> https://lists.gnu.org/archive/html/qemu-devel/2016-10/msg06206.html
>   -> https://lists.gnu.org/archive/html/qemu-devel/2016-10/msg06215.html
>
> diff --git a/hw/char/cadence_uart.c b/hw/char/cadence_uart.c
> index e3bc52f..5341d81 100644
> --- a/hw/char/cadence_uart.c
> +++ b/hw/char/cadence_uart.c
> @@ -1,5 +1,10 @@
>  /*
>   * Device model for Cadence UART
> + *   -> 
> http://www.xilinx.com/support/documentation/user_guides/ug585-Zynq-7000-TRM.pdf
> + *
> + * Reference: Xilinx Zynq 7000 reference manual
> + *   - Chapter 19 UART Controller
> + *   - Appendix B for Register details
>   *
>   * Copyright (c) 2010 Xilinx Inc.
>   * Copyright (c) 2012 Peter A.G. Crosthwaite 
> (peter.crosthwa...@petalogix.com)
> @@ -410,6 +415,18 @@ static void uart_write(void *opaque, hwaddr offset,
>  break;
>  }
>  break;
> +case R_BRGR: /* Baud rate generator */
> +s->r[offset] = 0x028B; /* default reset value */
> +if (value >= 0x01) {
> +s->r[offset] = value & 0x;
> +}
> +break;
> +case R_BDIV:/* Baud rate divider */
> +s->r[offset] = 0x0F;
> +if (value >= 0x04) {
> +s->r[offset] = value & 0xFF;
> +}
> +break;
>  default:
>  s->r[offset] = value;
>  }

You're relying on the register values never being
invalid to avoid the divide by zero, which means
you need to check them post-migration too.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v3] char: cadence: check baud rate generator and divider values

[Alistair Francis]

On Wed, Oct 26, 2016 at 2:22 PM, P J P  wrote:
> From: Prasad J Pandit 
>
> The Cadence UART device emulator calculates speed by dividing the
> baud rate by a 'baud rate generator' & 'baud rate divider' value.
> The device specification defines these register values to be
> non-zero and within certain limits. Add checks for these limits
> to avoid errors like divide by zero.
>
> Reported-by: Huawei PSIRT 
> Signed-off-by: Prasad J Pandit 
> ---
>  hw/char/cadence_uart.c | 17 +
>  1 file changed, 17 insertions(+)
>
> Update: mask R_BRGR and R_BDIV register values with 0x and 0xff resp.
>   -> https://lists.gnu.org/archive/html/qemu-devel/2016-10/msg06206.html
>   -> https://lists.gnu.org/archive/html/qemu-devel/2016-10/msg06215.html
>
> diff --git a/hw/char/cadence_uart.c b/hw/char/cadence_uart.c
> index e3bc52f..5341d81 100644
> --- a/hw/char/cadence_uart.c
> +++ b/hw/char/cadence_uart.c
> @@ -1,5 +1,10 @@
>  /*
>   * Device model for Cadence UART
> + *   -> 
> http://www.xilinx.com/support/documentation/user_guides/ug585-Zynq-7000-TRM.pdf

Nit pick, I would put the URL under the title below.

> + *
> + * Reference: Xilinx Zynq 7000 reference manual
> + *   - Chapter 19 UART Controller
> + *   - Appendix B for Register details
>   *
>   * Copyright (c) 2010 Xilinx Inc.
>   * Copyright (c) 2012 Peter A.G. Crosthwaite 
> (peter.crosthwa...@petalogix.com)
> @@ -410,6 +415,18 @@ static void uart_write(void *opaque, hwaddr offset,
>  break;
>  }
>  break;
> +case R_BRGR: /* Baud rate generator */
> +s->r[offset] = 0x028B; /* default reset value */

Why do we still have the reset value here, I thought we were just
ignoring the invalid writes? You don't need to reset it.

Thanks,

Alistair

> +if (value >= 0x01) {
> +s->r[offset] = value & 0x;
> +}
> +break;
> +case R_BDIV:/* Baud rate divider */
> +s->r[offset] = 0x0F;
> +if (value >= 0x04) {
> +s->r[offset] = value & 0xFF;
> +}
> +break;
>  default:
>  s->r[offset] = value;
>  }
> --
> 2.7.4
>
>