[Qemu-devel] [PATCH] tcg: assert last byte is in guest space

[Qemu-devel] [PATCH] tcg: assert last byte is in guest space

[Rémi Denis-Courmont]

Rather than assert that the first byte of a checked range is within the
guest address space, assert that the last byte is. The assertion is
moved past the overflow check to ensure that the last byte is actually
the one with the highest address.

Signed-off-by: Rémi Denis-Courmont <remi@remlab.net>
---
 accel/tcg/translate-all.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
index 8f593b926f..ea0c96af71 100644
--- a/accel/tcg/translate-all.c
+++ b/accel/tcg/translate-all.c
@@ -2515,13 +2515,6 @@ int page_check_range(target_ulong start, target_ulong len, int flags)
     target_ulong end;
     target_ulong addr;
 
-    /* This function should never be called with addresses outside the
-       guest address space.  If this assert fires, it probably indicates
-       a missing call to h2g_valid.  */
-#if TARGET_ABI_BITS > L1_MAP_ADDR_SPACE_BITS
-    assert(start < ((target_ulong)1 << L1_MAP_ADDR_SPACE_BITS));
-#endif
-
     if (len == 0) {
         return 0;
     }
@@ -2530,6 +2523,13 @@ int page_check_range(target_ulong start, target_ulong len, int flags)
         return -1;
     }
 
+    /* This function should never be called with addresses outside the
+       guest address space.  If this assert fires, it probably indicates
+       a missing call to h2g_valid.  */
+#if TARGET_ABI_BITS > L1_MAP_ADDR_SPACE_BITS
+    assert((start + len - 1) < ((target_ulong)1 << L1_MAP_ADDR_SPACE_BITS));
+#endif
+
     /* must do before we loose bits in the next step */
     end = TARGET_PAGE_ALIGN(start + len);
     start = start & TARGET_PAGE_MASK;
-- 
2.20.1

Re: [Qemu-devel] [PATCH] tcg: assert last byte is in guest space

[Richard Henderson]

On 2/8/19 8:37 AM, Rémi Denis-Courmont wrote:
> Rather than assert that the first byte of a checked range is within the
> guest address space, assert that the last byte is. The assertion is
> moved past the overflow check to ensure that the last byte is actually
> the one with the highest address.
> 
> Signed-off-by: Rémi Denis-Courmont <remi@remlab.net>
> ---
>  accel/tcg/translate-all.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)

What are you trying to fix here?

You've moved the assert past some returns, so that some cases that should not
be handled by this function no longer get checked.  As the comment says, the
address should already have been filtered by h2g_valid.


r~

Re: [Qemu-devel] [PATCH] tcg: assert last byte is in guest space

[Rémi Denis-Courmont]

Le perjantaina 8. helmikuuta 2019, 20.12.13 EET Richard Henderson a écrit :
> On 2/8/19 8:37 AM, Rémi Denis-Courmont wrote:
> > Rather than assert that the first byte of a checked range is within the
> > guest address space, assert that the last byte is. The assertion is
> > moved past the overflow check to ensure that the last byte is actually
> > the one with the highest address.
> > 
> > Signed-off-by: Rémi Denis-Courmont <remi@remlab.net>
> > ---
> > 
> >  accel/tcg/translate-all.c | 14 +++++++-------
> >  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> What are you trying to fix here?

As far as I can tell, the following code assumes that the entire range of 
checked addresses falls within the guest address range. So it makes sense to 
fail the assertion if the any byte is out of range, rather than only the first 
one.

> You've moved the assert past some returns, so that some cases that should
> not be handled by this function no longer get checked.  As the comment
> says, the address should already have been filtered by h2g_valid.

I find it generally hard to argue what an assertion should or should not do 
insofar as an assertion failure is supposed to be impossible. I am not in a 
position to decide if the assertion should fail in error cases that would be 
handled properly with a safe failure anyway - namely 0 bytes ranges and 
wrapping-around ranges.

However, it seems odd to live some undefined cases not failing the assertion, 
and I would not dare to add an extra assertion, so I replaced the one.

-- 
Rémi Denis-Courmont
http://www.remlab.net/

Re: [Qemu-devel] [PATCH] tcg: assert last byte is in guest space

[Richard Henderson]

On 2/8/19 10:32 AM, Rémi Denis-Courmont wrote:
> Le perjantaina 8. helmikuuta 2019, 20.12.13 EET Richard Henderson a écrit :
>> On 2/8/19 8:37 AM, Rémi Denis-Courmont wrote:
>>> Rather than assert that the first byte of a checked range is within the
>>> guest address space, assert that the last byte is. The assertion is
>>> moved past the overflow check to ensure that the last byte is actually
>>> the one with the highest address.
>>>
>>> Signed-off-by: Rémi Denis-Courmont <remi@remlab.net>
>>> ---
>>>
>>>  accel/tcg/translate-all.c | 14 +++++++-------
>>>  1 file changed, 7 insertions(+), 7 deletions(-)
>>
>> What are you trying to fix here?
> 
> As far as I can tell, the following code assumes that the entire range of 
> checked addresses falls within the guest address range. So it makes sense to 
> fail the assertion if the any byte is out of range, rather than only the first 
> one.

Sure.  But that would call for adding a second assert, rather than removing one
from some paths.  Which you say you "would not dare", which is confusing to me.

Is there a particular problem you are attempting to solve, or is this mere code
inspection?


r~