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
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
160
161
162
163
164
165
166
167
168
169
170
171
172
173
174
175
176
177
178
|
From 195f61375aeec9eec16604ec59f6eda2e6058cc1 Mon Sep 17 00:00:00 2001
From: "Luke T. Shumaker" <lukeshu@lukeshu.com>
Date: Thu, 30 May 2024 14:08:33 -0600
Subject: [PATCH 1/1] extract_vmlinuz.c: Fix the bounds check on
vmlinuz_header_{offset,size}
The check on vmlinuz_header_offset and vmlinuz_header_size is obviously
wrong:
if (!vmlinuz_header_size ||
kpart_data + vmlinuz_header_offset + vmlinuz_header_size >
kpart_data) {
return 1;
}
`kpart_data + some_unsigned_values` can obviously never be `> kpart_data`,
unless something has overflowed! And `vmlinuz_header_offset` hasn't even
been set yet (besides being initialized to zero)!
GCC will deduce that if the check didn't cause the function to bail, then
vmlinuz_header_size (a uint32_t) must be "negative"; that is: in the range
[2GiB,4GiB).
On platforms where size_t is 32-bits, this is *especially* broken.
memcpy's size argument must be in the range [0,2GiB). Because GCC has
proved that vmlinuz_header_size is higher than that, it will fail to
compile:
host/lib/extract_vmlinuz.c:67:9: error: 'memcpy' specified bound between 2147483648 and 4294967295 exceeds maximum object size 2147483647 [-Werror=stringop-overflow=]
So, fix the check.
I can now say that what I suspect the original author meant to write would
be the following patch, if `vmlinuz_header_offset` were already set:
-kpart_data + vmlinuz_header_offset + vmlinuz_header_size > kpart_data
+now + vmlinuz_header_offset + vmlinuz_header_size > kpart_size
This hypothesis is supported by `now` not getting incremented by
`kblob_size` the way it is for the keyblock and preamble sizes.
However, we can also see that even this "corrected" bounds check is
insufficient: it does not detect the vmlinuz_header overflowing into
kblob_data.
OK, so let's describe the fix:
Have a `*vmlinuz_header` pointer instead of a
`uint64_t vmlinuz_header_offset`, to be more similar to all the other
regions. With this change, the correct check becomes a simple
vmlinuz_header + vmlinuz_header_size > kblob_data
While we're at it, make some changes that could have helped avoid this in
the first place:
- Add comments.
- Calculate the vmlinuz_header offset right away, instead of waiting.
- Go ahead and increment `now` by `kblob_size`, to increase regularity.
Change-Id: I5c03e49070b6dd2e04459566ef7dd129d27736e4
---
host/lib/extract_vmlinuz.c | 72 +++++++++++++++++++++++++++-----------
1 file changed, 51 insertions(+), 21 deletions(-)
diff --git a/host/lib/extract_vmlinuz.c b/host/lib/extract_vmlinuz.c
index 4ccfcf33..d2c09443 100644
--- a/host/lib/extract_vmlinuz.c
+++ b/host/lib/extract_vmlinuz.c
@@ -15,16 +15,44 @@
int ExtractVmlinuz(void *kpart_data, size_t kpart_size,
void **vmlinuz_out, size_t *vmlinuz_size) {
+ // We're going to be extracting `vmlinuz_header` and
+ // `kblob_data`, and returning the concatenation of them.
+ //
+ // kpart_data = +-[kpart_size]------------------------------------+
+ // | |
+ // keyblock = | +-[keyblock->keyblock_size]-------------------+ |
+ // | | struct vb2_keyblock keyblock | |
+ // | | char [] ...data... | |
+ // | +---------------------------------------------+ |
+ // | |
+ // preamble = | +-[preamble->preamble_size]-------------------+ |
+ // | | struct vb2_kernel_preamble preamble | |
+ // | | char [] ...data... | |
+ // | | char [] vmlinuz_header | |
+ // | | char [] ...data... | |
+ // | +---------------------------------------------+ |
+ // | |
+ // kblob_data= | +-[preamble->body_signature.data_size]--------+ |
+ // | | char [] ...data... | |
+ // | +---------------------------------------------+ |
+ // | |
+ // +-------------------------------------------------+
+
size_t now = 0;
+ // The 3 sections of kpart_data.
+ struct vb2_keyblock *keyblock = NULL;
struct vb2_kernel_preamble *preamble = NULL;
uint8_t *kblob_data = NULL;
uint32_t kblob_size = 0;
+ // vmlinuz_header
+ uint8_t *vmlinuz_header = NULL;
uint32_t vmlinuz_header_size = 0;
- uint64_t vmlinuz_header_address = 0;
- uint64_t vmlinuz_header_offset = 0;
+ // The concatenated result.
void *vmlinuz = NULL;
- struct vb2_keyblock *keyblock = (struct vb2_keyblock *)kpart_data;
+ // Isolate the 3 sections of kpart_data.
+
+ keyblock = (struct vb2_keyblock *)kpart_data;
now += keyblock->keyblock_size;
if (now > kpart_size)
return 1;
@@ -36,37 +64,39 @@ int ExtractVmlinuz(void *kpart_data, size_t kpart_size,
kblob_data = kpart_data + now;
kblob_size = preamble->body_signature.data_size;
-
- if (!kblob_data || (now + kblob_size) > kpart_size)
+ now += kblob_size;
+ if (now > kpart_size)
return 1;
+ // Find `vmlinuz_header` within `preamble`.
+
if (preamble->header_version_minor > 0) {
- vmlinuz_header_address = preamble->vmlinuz_header_address;
+ // calculate the vmlinuz_header offset from
+ // the beginning of the kpart_data. The kblob doesn't
+ // include the body_load_offset, but does include
+ // the keyblock and preamble sections.
+ size_t vmlinuz_header_offset =
+ preamble->vmlinuz_header_address -
+ preamble->body_load_address +
+ keyblock->keyblock_size +
+ preamble->preamble_size;
+
+ vmlinuz_header = kpart_data + vmlinuz_header_offset;
vmlinuz_header_size = preamble->vmlinuz_header_size;
}
- if (!vmlinuz_header_size ||
- kpart_data + vmlinuz_header_offset + vmlinuz_header_size >
- kpart_data) {
+ if (!vmlinuz_header ||
+ !vmlinuz_header_size ||
+ vmlinuz_header + vmlinuz_header_size > kblob_data) {
return 1;
}
- // calculate the vmlinuz_header offset from
- // the beginning of the kpart_data. The kblob doesn't
- // include the body_load_offset, but does include
- // the keyblock and preamble sections.
- vmlinuz_header_offset = vmlinuz_header_address -
- preamble->body_load_address +
- keyblock->keyblock_size +
- preamble->preamble_size;
+ // Concatenate and return.
vmlinuz = malloc(vmlinuz_header_size + kblob_size);
if (vmlinuz == NULL)
return 1;
-
- memcpy(vmlinuz, kpart_data + vmlinuz_header_offset,
- vmlinuz_header_size);
-
+ memcpy(vmlinuz, vmlinuz_header, vmlinuz_header_size);
memcpy(vmlinuz + vmlinuz_header_size, kblob_data, kblob_size);
*vmlinuz_out = vmlinuz;
--
2.45.1
|
