* [Bug 1904331] [NEW] Coding bug in the function serial_ioport_write in serial.c
@ 2020-11-15 15:48 Jonathan D. Belanger
2020-11-18 15:40 ` [Bug 1904331] " Peter Maydell
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Jonathan D. Belanger @ 2020-11-15 15:48 UTC (permalink / raw)
To: qemu-devel
Public bug reported:
Branch hash: b50ea0d (pulled from github).
I was reviewing the code and noticed the following in the function
serial_ioport_write:
assert(size == 1 && addr < 8);
.
.
.
switch(addr) {
default:
case 0:
if (s->lcf & UART_LCR_DLAB) {
if (size == 1) {
s->divider = (s->divider & 0xff00) | val;
} else {
s->divider = val;
}
}
The assert will trigger if the size is > 1, so the else of the if (size
== 1) will never be executed and an attempt to specify a size > 1 will
trigger an assert.
The documentation for the UART indicates that the 16-bit divisor is
broken up amongst 2 8-bit registers (DLL and DLM). There already is
code to handle the DLL and DLM portions of the divider register (as
coded).
This is not exactly going to cause a bug, as there is no code that calls
this function with a value for size other than 1. It is just
unnecessary code.
** Affects: qemu
Importance: Undecided
Status: New
--
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1904331
Title:
Coding bug in the function serial_ioport_write in serial.c
Status in QEMU:
New
Bug description:
Branch hash: b50ea0d (pulled from github).
I was reviewing the code and noticed the following in the function
serial_ioport_write:
assert(size == 1 && addr < 8);
.
.
.
switch(addr) {
default:
case 0:
if (s->lcf & UART_LCR_DLAB) {
if (size == 1) {
s->divider = (s->divider & 0xff00) | val;
} else {
s->divider = val;
}
}
The assert will trigger if the size is > 1, so the else of the if
(size == 1) will never be executed and an attempt to specify a size >
1 will trigger an assert.
The documentation for the UART indicates that the 16-bit divisor is
broken up amongst 2 8-bit registers (DLL and DLM). There already is
code to handle the DLL and DLM portions of the divider register (as
coded).
This is not exactly going to cause a bug, as there is no code that
calls this function with a value for size other than 1. It is just
unnecessary code.
To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1904331/+subscriptions
^ permalink raw reply [flat|nested] 6+ messages in thread
* [Bug 1904331] Re: Coding bug in the function serial_ioport_write in serial.c
2020-11-15 15:48 [Bug 1904331] [NEW] Coding bug in the function serial_ioport_write in serial.c Jonathan D. Belanger
@ 2020-11-18 15:40 ` Peter Maydell
2020-11-20 16:19 ` [PATCH-for-5.2?] hw/char/serial: Clean up unnecessary code Philippe Mathieu-Daudé
2021-04-30 8:26 ` [Bug 1904331] Re: Coding bug in the function serial_ioport_write in serial.c Thomas Huth
2 siblings, 0 replies; 6+ messages in thread
From: Peter Maydell @ 2020-11-18 15:40 UTC (permalink / raw)
To: qemu-devel
** Changed in: qemu
Status: New => Confirmed
--
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1904331
Title:
Coding bug in the function serial_ioport_write in serial.c
Status in QEMU:
Confirmed
Bug description:
Branch hash: b50ea0d (pulled from github).
I was reviewing the code and noticed the following in the function
serial_ioport_write:
assert(size == 1 && addr < 8);
.
.
.
switch(addr) {
default:
case 0:
if (s->lcf & UART_LCR_DLAB) {
if (size == 1) {
s->divider = (s->divider & 0xff00) | val;
} else {
s->divider = val;
}
}
The assert will trigger if the size is > 1, so the else of the if
(size == 1) will never be executed and an attempt to specify a size >
1 will trigger an assert.
The documentation for the UART indicates that the 16-bit divisor is
broken up amongst 2 8-bit registers (DLL and DLM). There already is
code to handle the DLL and DLM portions of the divider register (as
coded).
This is not exactly going to cause a bug, as there is no code that
calls this function with a value for size other than 1. It is just
unnecessary code.
To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1904331/+subscriptions
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH-for-5.2?] hw/char/serial: Clean up unnecessary code
@ 2020-11-20 16:19 ` Philippe Mathieu-Daudé
2020-11-20 16:19 ` [Bug 1904331] " Philippe Mathieu-Daudé
2020-11-20 16:32 ` Paolo Bonzini
0 siblings, 2 replies; 6+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-11-20 16:19 UTC (permalink / raw)
To: qemu-devel
Cc: Bug 1904331, Peter Maydell, Jonathan D . Belanger,
Philippe Mathieu-Daudé,
Paolo Bonzini
Since commit 5ec3a23e6c8 ("serial: convert PIO to new memory
api read/write") we don't need to worry about accesses bigger
than 8-bit. Use the extract()/deposit() functions to access
the correct part of the 16-bit 'divider' register.
Reported-by: Jonathan D. Belanger <jbelanger1@rochester.rr.com>
Buglink: https://bugs.launchpad.net/qemu/+bug/1904331
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
Cc: Bug 1904331 <1904331@bugs.launchpad.net>
---
hw/char/serial.c | 13 +++++--------
1 file changed, 5 insertions(+), 8 deletions(-)
diff --git a/hw/char/serial.c b/hw/char/serial.c
index 97f71879ff2..62c627f486f 100644
--- a/hw/char/serial.c
+++ b/hw/char/serial.c
@@ -24,6 +24,7 @@
*/
#include "qemu/osdep.h"
+#include "qemu/bitops.h"
#include "hw/char/serial.h"
#include "hw/irq.h"
#include "migration/vmstate.h"
@@ -338,11 +339,7 @@ static void serial_ioport_write(void *opaque, hwaddr addr, uint64_t val,
default:
case 0:
if (s->lcr & UART_LCR_DLAB) {
- if (size == 1) {
- s->divider = (s->divider & 0xff00) | val;
- } else {
- s->divider = val;
- }
+ s->divider = deposit32(s->divider, 8 * addr, 8, val);
serial_update_parameters(s);
} else {
s->thr = (uint8_t) val;
@@ -364,7 +361,7 @@ static void serial_ioport_write(void *opaque, hwaddr addr, uint64_t val,
break;
case 1:
if (s->lcr & UART_LCR_DLAB) {
- s->divider = (s->divider & 0x00ff) | (val << 8);
+ s->divider = deposit32(s->divider, 8 * addr, 8, val);
serial_update_parameters(s);
} else {
uint8_t changed = (s->ier ^ val) & 0x0f;
@@ -478,7 +475,7 @@ static uint64_t serial_ioport_read(void *opaque, hwaddr addr, unsigned size)
default:
case 0:
if (s->lcr & UART_LCR_DLAB) {
- ret = s->divider & 0xff;
+ ret = extract16(s->divider, 8 * addr, 8);
} else {
if(s->fcr & UART_FCR_FE) {
ret = fifo8_is_empty(&s->recv_fifo) ?
@@ -502,7 +499,7 @@ static uint64_t serial_ioport_read(void *opaque, hwaddr addr, unsigned size)
break;
case 1:
if (s->lcr & UART_LCR_DLAB) {
- ret = (s->divider >> 8) & 0xff;
+ ret = extract16(s->divider, 8 * addr, 8);
} else {
ret = s->ier;
}
--
2.26.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [Bug 1904331] [PATCH-for-5.2?] hw/char/serial: Clean up unnecessary code
2020-11-20 16:19 ` [PATCH-for-5.2?] hw/char/serial: Clean up unnecessary code Philippe Mathieu-Daudé
@ 2020-11-20 16:19 ` Philippe Mathieu-Daudé
2020-11-20 16:32 ` Paolo Bonzini
1 sibling, 0 replies; 6+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-11-20 16:19 UTC (permalink / raw)
To: qemu-devel
Since commit 5ec3a23e6c8 ("serial: convert PIO to new memory
api read/write") we don't need to worry about accesses bigger
than 8-bit. Use the extract()/deposit() functions to access
the correct part of the 16-bit 'divider' register.
Reported-by: Jonathan D. Belanger <jbelanger1@rochester.rr.com>
Buglink: https://bugs.launchpad.net/qemu/+bug/1904331
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
Cc: Bug 1904331 <1904331@bugs.launchpad.net>
---
hw/char/serial.c | 13 +++++--------
1 file changed, 5 insertions(+), 8 deletions(-)
diff --git a/hw/char/serial.c b/hw/char/serial.c
index 97f71879ff2..62c627f486f 100644
--- a/hw/char/serial.c
+++ b/hw/char/serial.c
@@ -24,6 +24,7 @@
*/
#include "qemu/osdep.h"
+#include "qemu/bitops.h"
#include "hw/char/serial.h"
#include "hw/irq.h"
#include "migration/vmstate.h"
@@ -338,11 +339,7 @@ static void serial_ioport_write(void *opaque, hwaddr addr, uint64_t val,
default:
case 0:
if (s->lcr & UART_LCR_DLAB) {
- if (size == 1) {
- s->divider = (s->divider & 0xff00) | val;
- } else {
- s->divider = val;
- }
+ s->divider = deposit32(s->divider, 8 * addr, 8, val);
serial_update_parameters(s);
} else {
s->thr = (uint8_t) val;
@@ -364,7 +361,7 @@ static void serial_ioport_write(void *opaque, hwaddr addr, uint64_t val,
break;
case 1:
if (s->lcr & UART_LCR_DLAB) {
- s->divider = (s->divider & 0x00ff) | (val << 8);
+ s->divider = deposit32(s->divider, 8 * addr, 8, val);
serial_update_parameters(s);
} else {
uint8_t changed = (s->ier ^ val) & 0x0f;
@@ -478,7 +475,7 @@ static uint64_t serial_ioport_read(void *opaque, hwaddr addr, unsigned size)
default:
case 0:
if (s->lcr & UART_LCR_DLAB) {
- ret = s->divider & 0xff;
+ ret = extract16(s->divider, 8 * addr, 8);
} else {
if(s->fcr & UART_FCR_FE) {
ret = fifo8_is_empty(&s->recv_fifo) ?
@@ -502,7 +499,7 @@ static uint64_t serial_ioport_read(void *opaque, hwaddr addr, unsigned size)
break;
case 1:
if (s->lcr & UART_LCR_DLAB) {
- ret = (s->divider >> 8) & 0xff;
+ ret = extract16(s->divider, 8 * addr, 8);
} else {
ret = s->ier;
}
--
2.26.2
--
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1904331
Title:
Coding bug in the function serial_ioport_write in serial.c
Status in QEMU:
Confirmed
Bug description:
Branch hash: b50ea0d (pulled from github).
I was reviewing the code and noticed the following in the function
serial_ioport_write:
assert(size == 1 && addr < 8);
.
.
.
switch(addr) {
default:
case 0:
if (s->lcf & UART_LCR_DLAB) {
if (size == 1) {
s->divider = (s->divider & 0xff00) | val;
} else {
s->divider = val;
}
}
The assert will trigger if the size is > 1, so the else of the if
(size == 1) will never be executed and an attempt to specify a size >
1 will trigger an assert.
The documentation for the UART indicates that the 16-bit divisor is
broken up amongst 2 8-bit registers (DLL and DLM). There already is
code to handle the DLL and DLM portions of the divider register (as
coded).
This is not exactly going to cause a bug, as there is no code that
calls this function with a value for size other than 1. It is just
unnecessary code.
To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1904331/+subscriptions
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH-for-5.2?] hw/char/serial: Clean up unnecessary code
2020-11-20 16:19 ` [PATCH-for-5.2?] hw/char/serial: Clean up unnecessary code Philippe Mathieu-Daudé
2020-11-20 16:19 ` [Bug 1904331] " Philippe Mathieu-Daudé
@ 2020-11-20 16:32 ` Paolo Bonzini
1 sibling, 0 replies; 6+ messages in thread
From: Paolo Bonzini @ 2020-11-20 16:32 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, qemu-devel
Cc: Peter Maydell, Jonathan D . Belanger, Bug 1904331
On 20/11/20 17:19, Philippe Mathieu-Daudé wrote:
> Since commit 5ec3a23e6c8 ("serial: convert PIO to new memory
> api read/write") we don't need to worry about accesses bigger
> than 8-bit. Use the extract()/deposit() functions to access
> the correct part of the 16-bit 'divider' register.
>
> Reported-by: Jonathan D. Belanger <jbelanger1@rochester.rr.com>
> Buglink: https://bugs.launchpad.net/qemu/+bug/1904331
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> Cc: Bug 1904331 <1904331@bugs.launchpad.net>
> ---
> hw/char/serial.c | 13 +++++--------
> 1 file changed, 5 insertions(+), 8 deletions(-)
Looks good, but certainly not for 5.2.
Paolo
> diff --git a/hw/char/serial.c b/hw/char/serial.c
> index 97f71879ff2..62c627f486f 100644
> --- a/hw/char/serial.c
> +++ b/hw/char/serial.c
> @@ -24,6 +24,7 @@
> */
>
> #include "qemu/osdep.h"
> +#include "qemu/bitops.h"
> #include "hw/char/serial.h"
> #include "hw/irq.h"
> #include "migration/vmstate.h"
> @@ -338,11 +339,7 @@ static void serial_ioport_write(void *opaque, hwaddr addr, uint64_t val,
> default:
> case 0:
> if (s->lcr & UART_LCR_DLAB) {
> - if (size == 1) {
> - s->divider = (s->divider & 0xff00) | val;
> - } else {
> - s->divider = val;
> - }
> + s->divider = deposit32(s->divider, 8 * addr, 8, val);
> serial_update_parameters(s);
> } else {
> s->thr = (uint8_t) val;
> @@ -364,7 +361,7 @@ static void serial_ioport_write(void *opaque, hwaddr addr, uint64_t val,
> break;
> case 1:
> if (s->lcr & UART_LCR_DLAB) {
> - s->divider = (s->divider & 0x00ff) | (val << 8);
> + s->divider = deposit32(s->divider, 8 * addr, 8, val);
> serial_update_parameters(s);
> } else {
> uint8_t changed = (s->ier ^ val) & 0x0f;
> @@ -478,7 +475,7 @@ static uint64_t serial_ioport_read(void *opaque, hwaddr addr, unsigned size)
> default:
> case 0:
> if (s->lcr & UART_LCR_DLAB) {
> - ret = s->divider & 0xff;
> + ret = extract16(s->divider, 8 * addr, 8);
> } else {
> if(s->fcr & UART_FCR_FE) {
> ret = fifo8_is_empty(&s->recv_fifo) ?
> @@ -502,7 +499,7 @@ static uint64_t serial_ioport_read(void *opaque, hwaddr addr, unsigned size)
> break;
> case 1:
> if (s->lcr & UART_LCR_DLAB) {
> - ret = (s->divider >> 8) & 0xff;
> + ret = extract16(s->divider, 8 * addr, 8);
> } else {
> ret = s->ier;
> }
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* [Bug 1904331] Re: Coding bug in the function serial_ioport_write in serial.c
2020-11-15 15:48 [Bug 1904331] [NEW] Coding bug in the function serial_ioport_write in serial.c Jonathan D. Belanger
2020-11-18 15:40 ` [Bug 1904331] " Peter Maydell
2020-11-20 16:19 ` [PATCH-for-5.2?] hw/char/serial: Clean up unnecessary code Philippe Mathieu-Daudé
@ 2021-04-30 8:26 ` Thomas Huth
2 siblings, 0 replies; 6+ messages in thread
From: Thomas Huth @ 2021-04-30 8:26 UTC (permalink / raw)
To: qemu-devel
https://gitlab.com/qemu-project/qemu/-/commit/29daa894b6c31eae074d
** Changed in: qemu
Status: Confirmed => Fix Released
--
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1904331
Title:
Coding bug in the function serial_ioport_write in serial.c
Status in QEMU:
Fix Released
Bug description:
Branch hash: b50ea0d (pulled from github).
I was reviewing the code and noticed the following in the function
serial_ioport_write:
assert(size == 1 && addr < 8);
.
.
.
switch(addr) {
default:
case 0:
if (s->lcf & UART_LCR_DLAB) {
if (size == 1) {
s->divider = (s->divider & 0xff00) | val;
} else {
s->divider = val;
}
}
The assert will trigger if the size is > 1, so the else of the if
(size == 1) will never be executed and an attempt to specify a size >
1 will trigger an assert.
The documentation for the UART indicates that the 16-bit divisor is
broken up amongst 2 8-bit registers (DLL and DLM). There already is
code to handle the DLL and DLM portions of the divider register (as
coded).
This is not exactly going to cause a bug, as there is no code that
calls this function with a value for size other than 1. It is just
unnecessary code.
To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1904331/+subscriptions
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-04-30 8:46 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-15 15:48 [Bug 1904331] [NEW] Coding bug in the function serial_ioport_write in serial.c Jonathan D. Belanger
2020-11-18 15:40 ` [Bug 1904331] " Peter Maydell
2020-11-20 16:19 ` [PATCH-for-5.2?] hw/char/serial: Clean up unnecessary code Philippe Mathieu-Daudé
2020-11-20 16:19 ` [Bug 1904331] " Philippe Mathieu-Daudé
2020-11-20 16:32 ` Paolo Bonzini
2021-04-30 8:26 ` [Bug 1904331] Re: Coding bug in the function serial_ioport_write in serial.c Thomas Huth
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).