* [Qemu-devel] [PATCH v2] char: cadence: check baud rate generator and divider values
@ 2016-10-24 13:22 P J P
2016-10-25 17:22 ` Alistair Francis
2016-10-25 17:55 ` Peter Maydell
0 siblings, 2 replies; 8+ messages in thread
From: P J P @ 2016-10-24 13:22 UTC (permalink / raw)
To: Qemu Developers
Cc: Peter Maydell, Edgar E . Iglesias, Alistair Francis, qemu-arm,
Huawei PSIRT, Prasad J Pandit
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;
+ }
+ 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;
}
--
2.7.4
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH v2] char: cadence: check baud rate generator and divider values
2016-10-24 13:22 [Qemu-devel] [PATCH v2] char: cadence: check baud rate generator and divider values P J P
@ 2016-10-25 17:22 ` Alistair Francis
2016-10-25 18:24 ` P J P
2016-10-25 17:55 ` Peter Maydell
1 sibling, 1 reply; 8+ messages in thread
From: Alistair Francis @ 2016-10-25 17:22 UTC (permalink / raw)
To: P J P
Cc: Qemu Developers, Peter Maydell, Prasad J Pandit,
Alistair Francis, qemu-arm, Edgar E . Iglesias, Huawei PSIRT
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
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH v2] char: cadence: check baud rate generator and divider values
2016-10-24 13:22 [Qemu-devel] [PATCH v2] char: cadence: check baud rate generator and divider values P J P
2016-10-25 17:22 ` Alistair Francis
@ 2016-10-25 17:55 ` Peter Maydell
1 sibling, 0 replies; 8+ messages in thread
From: Peter Maydell @ 2016-10-25 17:55 UTC (permalink / raw)
To: P J P
Cc: Qemu Developers, Edgar E . Iglesias, Alistair Francis, qemu-arm,
Huawei PSIRT, Prasad J Pandit
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH v2] char: cadence: check baud rate generator and divider values
2016-10-25 17:22 ` Alistair Francis
@ 2016-10-25 18:24 ` P J P
2016-10-25 22:29 ` Alistair Francis
0 siblings, 1 reply; 8+ messages in thread
From: P J P @ 2016-10-25 18:24 UTC (permalink / raw)
To: Alistair Francis
Cc: Qemu Developers, Peter Maydell, qemu-arm, Edgar E . Iglesias,
Huawei PSIRT
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH v2] char: cadence: check baud rate generator and divider values
2016-10-25 18:24 ` P J P
@ 2016-10-25 22:29 ` Alistair Francis
2016-10-26 6:50 ` P J P
0 siblings, 1 reply; 8+ messages in thread
From: Alistair Francis @ 2016-10-25 22:29 UTC (permalink / raw)
To: P J P
Cc: Alistair Francis, Peter Maydell, qemu-arm, Huawei PSIRT,
Qemu Developers, Edgar E . Iglesias
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
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH v2] char: cadence: check baud rate generator and divider values
2016-10-25 22:29 ` Alistair Francis
@ 2016-10-26 6:50 ` P J P
2016-10-26 18:51 ` Alistair Francis
0 siblings, 1 reply; 8+ messages in thread
From: P J P @ 2016-10-26 6:50 UTC (permalink / raw)
To: Alistair Francis
Cc: Peter Maydell, qemu-arm, Huawei PSIRT, Qemu Developers,
Edgar E . Iglesias
+-- 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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH v2] char: cadence: check baud rate generator and divider values
2016-10-26 6:50 ` P J P
@ 2016-10-26 18:51 ` Alistair Francis
2016-10-26 21:23 ` P J P
0 siblings, 1 reply; 8+ messages in thread
From: Alistair Francis @ 2016-10-26 18:51 UTC (permalink / raw)
To: P J P
Cc: Alistair Francis, Peter Maydell, qemu-arm, Huawei PSIRT,
Qemu Developers, Edgar E . Iglesias
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
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH v2] char: cadence: check baud rate generator and divider values
2016-10-26 18:51 ` Alistair Francis
@ 2016-10-26 21:23 ` P J P
0 siblings, 0 replies; 8+ messages in thread
From: P J P @ 2016-10-26 21:23 UTC (permalink / raw)
To: Alistair Francis
Cc: Peter Maydell, qemu-arm, Huawei PSIRT, Qemu Developers,
Edgar E . Iglesias
+-- 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
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2016-10-26 21:23 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-10-24 13:22 [Qemu-devel] [PATCH v2] char: cadence: check baud rate generator and divider values P J P
2016-10-25 17:22 ` Alistair Francis
2016-10-25 18:24 ` P J P
2016-10-25 22:29 ` Alistair Francis
2016-10-26 6:50 ` P J P
2016-10-26 18:51 ` Alistair Francis
2016-10-26 21:23 ` P J P
2016-10-25 17:55 ` Peter Maydell
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.