| Message ID | 1476784025-27293-1-git-send-email-ppandit@redhat.com |
|---|---|
| State | New |
| Headers | show |
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
+-- 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
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 >
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
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 >
+-- 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
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 >
+-- 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
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 >
+-- 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
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 >
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;