[Qemu-devel] [PATCH] char: cadence: check divider against baud rate

[Qemu-devel] [PATCH] char: cadence: check divider against baud rate

[P J P]

From: Prasad J Pandit <pjp@fedoraproject.org>

The Cadence UART device emulator calculates speed by dividing the
baud rate by a divider. If this divider was to be zero or if baud
rate was to be lesser than the divider, it could lead to a divide
by zero error. Add check to avoid it.

Reported-by: Huawei PSIRT <psirt@huawei.com>
Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
---
 hw/char/cadence_uart.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/char/cadence_uart.c b/hw/char/cadence_uart.c
index e3bc52f..b18dd7f 100644
--- a/hw/char/cadence_uart.c
+++ b/hw/char/cadence_uart.c
@@ -170,6 +170,10 @@ static void uart_parameters_setup(CadenceUARTState *s)
     baud_rate = (s->r[R_MR] & UART_MR_CLKS) ?
             UART_INPUT_CLK / 8 : UART_INPUT_CLK;
 
+    if (!s->r[R_BRGR] || !(s->r[R_BDIV] + 1)
+        || baud_rate < (s->r[R_BRGR] * (s->r[R_BDIV] + 1))) {
+        return;
+    }
     ssp.speed = baud_rate / (s->r[R_BRGR] * (s->r[R_BDIV] + 1));
     packet_size = 1;
 
-- 
2.7.4

Re: [Qemu-devel] [Qemu-arm] [PATCH] char: cadence: check divider against baud rate

[Peter Maydell]

On 18 October 2016 at 10:47, 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 divider. If this divider was to be zero or if baud
> rate was to be lesser than the divider, it could lead to a divide
> by zero error. Add check to avoid it.
>
> Reported-by: Huawei PSIRT <psirt@huawei.com>
> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
> ---
>  hw/char/cadence_uart.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/hw/char/cadence_uart.c b/hw/char/cadence_uart.c
> index e3bc52f..b18dd7f 100644
> --- a/hw/char/cadence_uart.c
> +++ b/hw/char/cadence_uart.c
> @@ -170,6 +170,10 @@ static void uart_parameters_setup(CadenceUARTState *s)
>      baud_rate = (s->r[R_MR] & UART_MR_CLKS) ?
>              UART_INPUT_CLK / 8 : UART_INPUT_CLK;
>
> +    if (!s->r[R_BRGR] || !(s->r[R_BDIV] + 1)
> +        || baud_rate < (s->r[R_BRGR] * (s->r[R_BDIV] + 1))) {
> +        return;
> +    }
>      ssp.speed = baud_rate / (s->r[R_BRGR] * (s->r[R_BDIV] + 1));
>      packet_size = 1;

It seems really unlikely that early return here is the correct thing, since
it will result in our not correctly setting a bunch of the other
stuff done later in this function that's unrelated to baud rate.
What does the datasheet for this UART specify for this situation?

thanks
-- PMM

Re: [Qemu-devel] [Qemu-arm] [PATCH] char: cadence: check divider against baud rate

[P J P]

+-- On Tue, 18 Oct 2016, Peter Maydell wrote --+
| > +    if (!s->r[R_BRGR] || !(s->r[R_BDIV] + 1)
| > +        || baud_rate < (s->r[R_BRGR] * (s->r[R_BDIV] + 1))) {
| > +        return;
| > +    }
| >      ssp.speed = baud_rate / (s->r[R_BRGR] * (s->r[R_BDIV] + 1));
| >      packet_size = 1;
| 
| It seems really unlikely that early return here is the correct thing, since
| it will result in our not correctly setting a bunch of the other
| stuff done later in this function that's unrelated to baud rate.
| What does the datasheet for this UART specify for this situation?

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

Re: [Qemu-devel] [Qemu-arm] [PATCH] char: cadence: check divider against baud rate

[Alistair Francis]

On Tue, Oct 18, 2016 at 8:46 PM, P J P <ppandit@redhat.com> wrote:
> +-- On Tue, 18 Oct 2016, Peter Maydell wrote --+
> | > +    if (!s->r[R_BRGR] || !(s->r[R_BDIV] + 1)
> | > +        || baud_rate < (s->r[R_BRGR] * (s->r[R_BDIV] + 1))) {
> | > +        return;
> | > +    }
> | >      ssp.speed = baud_rate / (s->r[R_BRGR] * (s->r[R_BDIV] + 1));
> | >      packet_size = 1;
> |
> | It seems really unlikely that early return here is the correct thing, since
> | it will result in our not correctly setting a bunch of the other
> | stuff done later in this function that's unrelated to baud rate.
> | What does the datasheet for this UART specify for this situation?
>
> @Alistair.. wdyt?

I'm traveling this week, so I won't have a chance to check the data
sheet until next week.

Generally these types of IP just say 'undefined' in invalid situations
like this. If that is the case we shouldn't return but just set some
default value and allow the other functions in the setup to continue
on.

Thanks,

Alistair

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

Re: [Qemu-devel] [Qemu-arm] [PATCH] char: cadence: check divider against baud rate

[P J P]

  Hello Alistair,

+-- On Wed, 19 Oct 2016, Alistair Francis wrote --+
| Generally these types of IP just say 'undefined' in invalid situations
| like this. If that is the case we shouldn't return but just set some
| default value and allow the other functions in the setup to continue
| on.

  Could you please point me to its data sheet? I went through quite a few that 
I came across, but none was helpful.

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

Re: [Qemu-devel] [Qemu-arm] [PATCH] char: cadence: check divider against baud rate

[Alistair Francis]

On Wed, Oct 19, 2016 at 12:11 PM, P J P <ppandit@redhat.com> wrote:
>   Hello Alistair,
>
> +-- On Wed, 19 Oct 2016, Alistair Francis wrote --+
> | Generally these types of IP just say 'undefined' in invalid situations
> | like this. If that is the case we shouldn't return but just set some
> | default value and allow the other functions in the setup to continue
> | on.
>
>   Could you please point me to its data sheet? I went through quite a few that
> I came across, but none was helpful.

I don't think the Cadence datasheets are public. If you can't find it
from a simple Google search I don't think you have access.

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] [Qemu-arm] [PATCH] char: cadence: check divider against baud rate

[P J P]

+-- On Wed, 19 Oct 2016, Alistair Francis wrote --+
| I don't think the Cadence datasheets are public. If you can't find it
| from a simple Google search I don't think you have access.

  I see, okay.

I think it'll greatly help if device emulator writers include a link to its 
specification/datasheet resources in the respective source file. At least if 
these are publicly available. That way one knows for sure which specifications 
to follow while debugging, instead of searching and sifting through multiple 
different versions. OR Maybe there could a field in the MAINTAINERS file which 
would point to respective reference resources. (just a thought)

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

Re: [Qemu-devel] [Qemu-arm] [PATCH] char: cadence: check divider against baud rate

[Edgar E. Iglesias]

On Thu, Oct 20, 2016 at 11:25:47AM +0530, P J P wrote:
> +-- On Wed, 19 Oct 2016, Alistair Francis wrote --+
> | I don't think the Cadence datasheets are public. If you can't find it
> | from a simple Google search I don't think you have access.
> 
>   I see, okay.
> 
> I think it'll greatly help if device emulator writers include a link to its 
> specification/datasheet resources in the respective source file. At least if 
> these are publicly available. That way one knows for sure which specifications 
> to follow while debugging, instead of searching and sifting through multiple 
> different versions. OR Maybe there could a field in the MAINTAINERS file which 
> would point to respective reference resources. (just a thought)

Hi,

The UART is described in the Zynq TRMs, I think you can just
google for zynq TRM and a reference to UG585 and UG1085 will come up.

http://www.xilinx.com/support/documentation/user_guides/ug585-Zynq-7000-TRM.pdf
http://www.xilinx.com/support/documentation/user_guides/ug1085-zynq-ultrascale-trm.pdf

AFAIK, the ZynqMP has a newer version of the cadence UART with a superset of
the feature set. I don't think QEMU models any of the differences though.

Cheers,
Edgar


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

Re: [Qemu-devel] [Qemu-arm] [PATCH] char: cadence: check divider against baud rate

[P J P]

+-- On Thu, 20 Oct 2016, Edgar E. Iglesias wrote --+
| The UART is described in the Zynq TRMs, I think you can just
| google for zynq TRM and a reference to UG585 and UG1085 will come up.

  Unlikely for a novice to think 'zynq TRM' as search term for cadence UART. I 
went with Xilinx Cadence UART specificaiton.
 
| http://www.xilinx.com/support/documentation/user_guides/ug585-Zynq-7000-TRM.pdf
| http://www.xilinx.com/support/documentation/user_guides/ug1085-zynq-ultrascale-trm.pdf
| 
| AFAIK, the ZynqMP has a newer version of the cadence UART with a superset of
| the feature set. I don't think QEMU models any of the differences though.

  I see, thank you for the details. I'll add these to the source file.

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

Re: [Qemu-devel] [Qemu-arm] [PATCH] char: cadence: check divider against baud rate

[Alistair Francis]

On Fri, Oct 21, 2016 at 9:47 AM, P J P <ppandit@redhat.com> wrote:
> +-- On Thu, 20 Oct 2016, Edgar E. Iglesias wrote --+
> | The UART is described in the Zynq TRMs, I think you can just
> | google for zynq TRM and a reference to UG585 and UG1085 will come up.
>
>   Unlikely for a novice to think 'zynq TRM' as search term for cadence UART. I
> went with Xilinx Cadence UART specificaiton.
>
> | http://www.xilinx.com/support/documentation/user_guides/ug585-Zynq-7000-TRM.pdf
> | http://www.xilinx.com/support/documentation/user_guides/ug1085-zynq-ultrascale-trm.pdf
> |
> | AFAIK, the ZynqMP has a newer version of the cadence UART with a superset of
> | the feature set. I don't think QEMU models any of the differences though.
>
>   I see, thank you for the details. I'll add these to the source file.

Did the TRM have enough detail for you to figure out how the hardware behaves?

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] [Qemu-arm] [PATCH] char: cadence: check divider against baud rate

[P J P]

+-- On Mon, 24 Oct 2016, Alistair Francis wrote --+
| > | http://www.xilinx.com/support/documentation/user_guides/ug585-Zynq-7000-TRM.pdf
| 
| Did the TRM have enough detail for you to figure out how the hardware behaves?

  Yes, it defines range for 'Baud rate generator' and 'Baud rate divider' 
register values and their default reset value. I've sent patch v2.

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

Re: [Qemu-devel] [Qemu-arm] [PATCH] char: cadence: check divider against baud rate

[Alistair Francis]

On Mon, Oct 24, 2016 at 6:25 AM, P J P <ppandit@redhat.com> wrote:
> +-- On Mon, 24 Oct 2016, Alistair Francis wrote --+
> | > | http://www.xilinx.com/support/documentation/user_guides/ug585-Zynq-7000-TRM.pdf
> |
> | Did the TRM have enough detail for you to figure out how the hardware behaves?
>
>   Yes, it defines range for 'Baud rate generator' and 'Baud rate divider'
> register values and their default reset value. I've sent patch v2.

Great!

Thanks,

Alistair

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