Coding bug in the function serial_ioport_write in serial.c
Affects | Status | Importance | Assigned to | Milestone | |
---|---|---|---|---|---|
QEMU |
Fix Released
|
Undecided
|
Unassigned |
Bug Description
Branch hash: b50ea0d (pulled from github).
I was reviewing the code and noticed the following in the function serial_
assert(size == 1 && addr < 8);
.
.
.
switch(addr) {
default:
case 0:
if (s->lcf & UART_LCR_DLAB) {
if (size == 1) {
} else {
}
}
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.
Changed in qemu: | |
status: | New → Confirmed |
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 <email address hidden> /bugs.launchpad .net/qemu/ +bug/1904331
Buglink: https:/
Signed-off-by: Philippe Mathieu-Daudé <email address hidden>
---
Cc: Bug 1904331 <email address hidden>
---
hw/char/serial.c | 13 +++++--------
1 file changed, 5 insertions(+), 8 deletions(-)
diff --git a/hw/char/serial.c b/hw/char/serial.c .62c627f486f 100644
index 97f71879ff2.
--- a/hw/char/serial.c
+++ b/hw/char/serial.c
@@ -24,6 +24,7 @@
*/
#include "qemu/osdep.h" vmstate. h" ioport_ write(void *opaque, hwaddr addr, uint64_t val, s->divider, 8 * addr, 8, val);
serial_ update_ parameters( s); ioport_ write(void *opaque, hwaddr addr, uint64_t val, s->divider, 8 * addr, 8, val);
serial_ update_ parameters( s);
uint8_ t changed = (s->ier ^ val) & 0x0f; ioport_ read(void *opaque, hwaddr addr, unsigned size) s->divider, 8 * addr, 8);
if( s->fcr & UART_FCR_FE) {
ret = fifo8_is_ empty(& s->recv_ fifo) ? ioport_ read(void *opaque, hwaddr addr, unsigned size) s->divider, 8 * addr, 8);
+#include "qemu/bitops.h"
#include "hw/char/serial.h"
#include "hw/irq.h"
#include "migration/
@@ -338,11 +339,7 @@ static void serial_
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(
} else {
s->thr = (uint8_t) val;
@@ -364,7 +361,7 @@ static void serial_
break;
case 1:
if (s->lcr & UART_LCR_DLAB) {
- s->divider = (s->divider & 0x00ff) | (val << 8);
+ s->divider = deposit32(
} else {
@@ -478,7 +475,7 @@ static uint64_t serial_
default:
case 0:
if (s->lcr & UART_LCR_DLAB) {
- ret = s->divider & 0xff;
+ ret = extract16(
} else {
@@ -502,7 +499,7 @@ static uint64_t serial_
break;
case 1:
if (s->lcr & UART_LCR_DLAB) {
- ret = (s->divider >> 8) & 0xff;
+ ret = extract16(
} else {
ret = s->ier;
}
--
2.26.2