1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
|
peripherals: 0.705
hypervisor: 0.691
TCG: 0.688
PID: 0.628
performance: 0.625
graphic: 0.622
ppc: 0.612
network: 0.599
user-level: 0.594
virtual: 0.578
device: 0.565
permissions: 0.561
register: 0.558
arm: 0.557
architecture: 0.557
KVM: 0.552
risc-v: 0.541
vnc: 0.536
VMM: 0.526
semantic: 0.518
debug: 0.509
assembly: 0.496
mistranslation: 0.494
boot: 0.486
kernel: 0.476
socket: 0.475
x86: 0.420
files: 0.399
i386: 0.165
Coding bug in the function serial_ioport_write in serial.c
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.
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>
Buglink: https://bugs.launchpad.net/qemu/+bug/1904331
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
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
https://gitlab.com/qemu-project/qemu/-/commit/29daa894b6c31eae074d
|