exec: Fix non-power-of-2 sized accesses
PATCH v3 1/1 exec: Fix non-power-of-2 sized accesses
Since commit 23326164 we align access sizes to match the alignment of the address, but we don't align the access size itself. This means we let illegal access sizes (ex. 3) slip through if the address is sufficiently aligned (ex. 4). This results in an abort which would be easy for a guest to trigger. Account for aligning the access size.
---
v3: Highest power of 2, not lowest
v2: Remove unnecessary loop condition
exec.c | 7 +++++++ 1 file changed, 7 insertions(+)
exec.c +7 -0
@@ -1924,6 +1924,13 @@ static int memory_access_size(MemoryRegion *mr, unsigned l, hwaddr addr) } } +/* Size must be a power of 2 */ +if (l & (l -1 )) { +while (!(l & access_size_max) && l & (access_size_max -1 )) { + access_size_max >>=1 ; + } + } +/* Don't attempt accesses larger than the maximum. */ if (l > access_size_max) { l = access_size_max;
Quoted 25 lines · Alex Williamson
Since commit 23326164 we align access sizes to match the alignment of the address, but we don't align the access size itself. This means we let illegal access sizes (ex. 3) slip through if the address is sufficiently aligned (ex. 4). This results in an abort which would be easy for a guest to trigger. Account for aligning the access size.
Signed-off-by Alex Williamson <[email protected]>---
v3: Highest power of 2, not lowest
v2: Remove unnecessary loop conditionexec.c | 7 +++++++ 1 file changed, 7 insertions(+)exec.c +0
-0@@ -1924,6 +1924,13 @@ static int memory_access_size(MemoryRegion *mr, unsigned l, hwaddr addr) } }
exec.c static int memory_access_size(MemoryRegion *mr, unsigned l, hwaddr addr) · line 1936
Quoted 10 lines
exec.c +7
-0+/* Size must be a power of 2 */ +if (l & (l -1 )) { +while (!(l & access_size_max) && l & (access_size_max -1 )) { + access_size_max >>=1 ; + } + } +/* Don't attempt accesses larger than the maximum. */ if (l > access_size_max) { l = access_size_max;
Apologies, but I'm now totally confused.
Suppose that the new code is reached with (access_size_max == 4).
Now, l==9 and l==3 will enter the loop just the same, both shifting "access_size_max" right at least once, even though 9 is greater than 4, and 3 is less than 4.
Is that OK? What's the goal here?
Sorry for being dense...
Thanks,
Laszlo
Quoted 39 lines · Laszlo Ersek
Quoted 25 lines · Alex Williamson
Since commit 23326164 we align access sizes to match the alignment of the address, but we don't align the access size itself. This means we let illegal access sizes (ex. 3) slip through if the address is sufficiently aligned (ex. 4). This results in an abort which would be easy for a guest to trigger. Account for aligning the access size.
Signed-off-by Alex Williamson <[email protected]>---
v3: Highest power of 2, not lowest
v2: Remove unnecessary loop conditionexec.c | 7 +++++++ 1 file changed, 7 insertions(+)exec.c +0
-0@@ -1924,6 +1924,13 @@ static int memory_access_size(MemoryRegion *mr, unsigned l, hwaddr addr) } }exec.c static int memory_access_size(MemoryRegion *mr, unsigned l, hwaddr addr) · line 1936
Quoted 10 lines
exec.c +7
-0+/* Size must be a power of 2 */ +if (l & (l -1 )) { +while (!(l & access_size_max) && l & (access_size_max -1 )) { + access_size_max >>=1 ; + } + } +/* Don't attempt accesses larger than the maximum. */ if (l > access_size_max) { l = access_size_max;Apologies, but I'm now totally confused.
Quoted 5 lines
Suppose that the new code is reached with (
access_size_max== 4).Now, l==9 and l==3 will enter the loop just the same, both shifting "
access_size_max" right at least once, even though 9 is greater than 4, and 3 is less than 4.
*sigh*, just as I was getting ready to point out that the above is faster than pow2floor, you have to go and point out that the result is wrong ;) I don't think clz or pow2floor is the answer though, the problem size is too small. Rather than trying to solve this with an algorithm, I think we just need a simple:
if (size >= 8)
size = 8; else if (size >=4) size = 4;
...
We only have 4 cases to deal with and it comes out a couple times faster than pow2floor. v4... Thanks,
Alex