diff mbox

[v2] char: cadence: check baud rate generator and divider values

Message ID 1477315364-15134-1-git-send-email-ppandit@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Prasad Pandit Oct. 24, 2016, 1:22 p.m. UTC 
From: Prasad J Pandit <pjp@fedoraproject.org>

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 <psirt@huawei.com>
Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
---
 hw/char/cadence_uart.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

Update: set register values as per the specification
  -> https://lists.gnu.org/archive/html/qemu-devel/2016-10/msg04931.html

Comments

Alistair Francis Oct. 25, 2016, 5:22 p.m. UTC | #1 
On Mon, Oct 24, 2016 at 6:22 AM, P J P <ppandit@redhat.com> wrote:
> From: Prasad J Pandit <pjp@fedoraproject.org>
>
> 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 <psirt@huawei.com>
> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
> ---
>  hw/char/cadence_uart.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
>
> Update: set register values as per the specification
>   -> https://lists.gnu.org/archive/html/qemu-devel/2016-10/msg04931.html
>
> diff --git a/hw/char/cadence_uart.c b/hw/char/cadence_uart.c
> index e3bc52f..c176446 100644
> --- a/hw/char/cadence_uart.c
> +++ b/hw/char/cadence_uart.c
> @@ -1,5 +1,6 @@
>  /*
>   * Device model for Cadence UART
> + *  -> http://www.xilinx.com/support/documentation/user_guides/ug585-Zynq-7000-TRM.pdf

Can you say what page/section the UART spec is in the Xilinx TRM?

I think it might also be worth noting that the datasheet is a Xilinx
datasheet that covers the Cadence UART. Others might be using the IP
as well and might get confused why you are referring to a Xilinx
datasheet.

>   *
>   * Copyright (c) 2010 Xilinx Inc.
>   * Copyright (c) 2012 Peter A.G. Crosthwaite (peter.crosthwaite@petalogix.com)
> @@ -410,6 +411,18 @@ static void uart_write(void *opaque, hwaddr offset,
>              break;
>          }
>          break;
> +    case R_BRGR: /* Baud rate generator */
> +        s->r[offset] = 0x028B; /* default reset value */

Is this the correct behavior, or should the write just be ignored?
pg.587 of the TRM doesn't really make this clear, did you find this
somewhere else?

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

Same here.

Thanks,

Alistair

> +        if (value >= 0x04 && value <= 0xFF) {
> +            s->r[offset] = value;
> +        }
> +        break;
>      default:
>          s->r[offset] = value;
>      }
> --
> 2.7.4
>
>
Peter Maydell Oct. 25, 2016, 5:55 p.m. UTC | #2 
On 24 October 2016 at 14:22, P J P <ppandit@redhat.com> wrote:
> From: Prasad J Pandit <pjp@fedoraproject.org>
>
> 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 <psirt@huawei.com>
> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
> ---
>  hw/char/cadence_uart.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
>
> Update: set register values as per the specification
>   -> https://lists.gnu.org/archive/html/qemu-devel/2016-10/msg04931.html
>
> diff --git a/hw/char/cadence_uart.c b/hw/char/cadence_uart.c
> index e3bc52f..c176446 100644
> --- a/hw/char/cadence_uart.c
> +++ b/hw/char/cadence_uart.c
> @@ -1,5 +1,6 @@
>  /*
>   * Device model for Cadence UART
> + *  -> http://www.xilinx.com/support/documentation/user_guides/ug585-Zynq-7000-TRM.pdf
>   *
>   * Copyright (c) 2010 Xilinx Inc.
>   * Copyright (c) 2012 Peter A.G. Crosthwaite (peter.crosthwaite@petalogix.com)
> @@ -410,6 +411,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 && value <= 0xFFFF) {
> +            s->r[offset] = value;
> +        }

Appendix B says that setting this to 0 has a meaning
("disables baud_sample"), though it's less clear what exactly
that ought to do (stop the UART working completely??). It
also says that bits 31:16 are ro, so you should be doing
a value &= 0xffff; rather than doing an upper bounds check.

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

Appendix B says:
8 bit register, so mask value with 0xff.
Values 0 to 3 are "ignored", which is slightly surprising
hardware behaviour but I guess we take them at their word
and ignore the write...

> +        break;
>      default:
>          s->r[offset] = value;
>      }

If you're taking the approach of ensuring that the register
values are within bounds in order to avoid crashes, then
you also need to check after a migration state load.

thanks
-- PMM
Prasad Pandit Oct. 25, 2016, 6:24 p.m. UTC | #3 
Hello Alistair,

+-- On Tue, 25 Oct 2016, Alistair Francis wrote --+
| >   * Device model for Cadence UART
| > + *  -> http://www.xilinx.com/support/documentation/user_guides/ug585-Zynq-7000-TRM.pdf
| 
| Can you say what page/section the UART spec is in the Xilinx TRM?

  Chapter 19 UART Controller, page 585, 19.2.3 Baud Rate Generator.
 
| I think it might also be worth noting that the datasheet is a Xilinx
| datasheet that covers the Cadence UART. Others might be using the IP
| as well and might get confused why you are referring to a Xilinx
| datasheet.

  Right, I'll add above section details in the comment.
 
| > +    case R_BRGR: /* Baud rate generator */
| > +        s->r[offset] = 0x028B; /* default reset value */
| 
| Is this the correct behavior, or should the write just be ignored?
| pg.587 of the TRM doesn't really make this clear, did you find this
| somewhere else?

  True, page 587 does not clearly mention if it should be ignored.
But in Appendix B, Register details for 'Baud_rate_gen_reg0' says

    0: Disables baud_sample
    1: Clock divisor bypass (baud_sample = sel_clk)
    2 - 65535: baud_sample

| > +    case R_BDIV:    /* Baud rate divider */
| > +        s->r[offset] = 0x0F;

  Appendix B, Register details for 'Baud_rate_divider_reg0' says

    0 - 3: ignored
    4 - 255: Baud rate


ie. values 0-3 are ignored. But should we avoid writing 's->r[R_BRGR]' & 
's->r[R_BDIV]' for these values? That would lead to undefined values being 
using in 'uart_parameters_setup()', no?

Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
Alistair Francis Oct. 25, 2016, 10:29 p.m. UTC | #4 
On Tue, Oct 25, 2016 at 11:24 AM, P J P <ppandit@redhat.com> wrote:
>    Hello Alistair,
>
> +-- On Tue, 25 Oct 2016, Alistair Francis wrote --+
> | >   * Device model for Cadence UART
> | > + *  -> http://www.xilinx.com/support/documentation/user_guides/ug585-Zynq-7000-TRM.pdf
> |
> | Can you say what page/section the UART spec is in the Xilinx TRM?
>
>   Chapter 19 UART Controller, page 585, 19.2.3 Baud Rate Generator.
>
> | I think it might also be worth noting that the datasheet is a Xilinx
> | datasheet that covers the Cadence UART. Others might be using the IP
> | as well and might get confused why you are referring to a Xilinx
> | datasheet.
>
>   Right, I'll add above section details in the comment.
>
> | > +    case R_BRGR: /* Baud rate generator */
> | > +        s->r[offset] = 0x028B; /* default reset value */
> |
> | Is this the correct behavior, or should the write just be ignored?
> | pg.587 of the TRM doesn't really make this clear, did you find this
> | somewhere else?
>
>   True, page 587 does not clearly mention if it should be ignored.
> But in Appendix B, Register details for 'Baud_rate_gen_reg0' says
>
>     0: Disables baud_sample
>     1: Clock divisor bypass (baud_sample = sel_clk)
>     2 - 65535: baud_sample
>
> | > +    case R_BDIV:    /* Baud rate divider */
> | > +        s->r[offset] = 0x0F;
>
>   Appendix B, Register details for 'Baud_rate_divider_reg0' says
>
>     0 - 3: ignored
>     4 - 255: Baud rate
>
>
> ie. values 0-3 are ignored. But should we avoid writing 's->r[R_BRGR]' &
> 's->r[R_BDIV]' for these values? That would lead to undefined values being
> using in 'uart_parameters_setup()', no?

I think your email crossed with Peter. Have a look at what he said.
That should clarify everything.

Thanks,

Alistair

>
> Thank you.
> --
> Prasad J Pandit / Red Hat Product Security Team
> 47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
>
Prasad Pandit Oct. 26, 2016, 6:50 a.m. UTC | #5 
+-- On Tue, 25 Oct 2016, Alistair Francis wrote --+
| I think your email crossed with Peter. Have a look at what he said.
| That should clarify everything.

  I saw, I'll mask values with 0xFFFF and 0xFF; But it's not clear if ignoring 
values 0 - 3, means leaving registers uninitialised and adding checks at each 
usage point to ensure their value is valid. If so, do we return from 
uart_parameters_setup(), when either value is invalid ? Earlier you'd 
suggested using default values

  -> https://lists.gnu.org/archive/html/qemu-devel/2016-10/msg04397.html

Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
Alistair Francis Oct. 26, 2016, 6:51 p.m. UTC | #6 
On Tue, Oct 25, 2016 at 11:50 PM, P J P <ppandit@redhat.com> wrote:
> +-- On Tue, 25 Oct 2016, Alistair Francis wrote --+
> | I think your email crossed with Peter. Have a look at what he said.
> | That should clarify everything.
>
>   I saw, I'll mask values with 0xFFFF and 0xFF; But it's not clear if ignoring
> values 0 - 3, means leaving registers uninitialised and adding checks at each
> usage point to ensure their value is valid. If so, do we return from
> uart_parameters_setup(), when either value is invalid ? Earlier you'd
> suggested using default values
>
>   -> https://lists.gnu.org/archive/html/qemu-devel/2016-10/msg04397.html

From what I can tell I would say that any writes to the divider
register that have a value lower then 3 are ignored and the register
isn't updated.

That way we will always have a valid value as the register is reset to 0xF.

Thanks,

Alistair

>
> Thank you.
> --
> Prasad J Pandit / Red Hat Product Security Team
> 47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
>
Prasad Pandit Oct. 26, 2016, 9:23 p.m. UTC | #7 
+-- On Wed, 26 Oct 2016, Alistair Francis wrote --+
| >From what I can tell I would say that any writes to the divider
| register that have a value lower then 3 are ignored and the register
| isn't updated.
| 
| That way we will always have a valid value as the register is reset to 0xF.

Sent a revised patch v3. Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
diff mbox

Patch

diff --git a/hw/char/cadence_uart.c b/hw/char/cadence_uart.c
index e3bc52f..c176446 100644
--- a/hw/char/cadence_uart.c
+++ b/hw/char/cadence_uart.c
@@ -1,5 +1,6 @@ 
 /*
  * Device model for Cadence UART
+ *  -> http://www.xilinx.com/support/documentation/user_guides/ug585-Zynq-7000-TRM.pdf
  *
  * Copyright (c) 2010 Xilinx Inc.
  * Copyright (c) 2012 Peter A.G. Crosthwaite (peter.crosthwaite@petalogix.com)
@@ -410,6 +411,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 && value <= 0xFFFF) {
+            s->r[offset] = value;
+        }
+        break;
+    case R_BDIV:    /* Baud rate divider */
+        s->r[offset] = 0x0F;
+        if (value >= 0x04 && value <= 0xFF) {
+            s->r[offset] = value;
+        }
+        break;
     default:
         s->r[offset] = value;
     }