1. Patchew
  2. QEMU
  3. View series

[RFC PATCH] target/i386: allow access_ptr to force slow path on failed probe

Alex Bennée posted 1 patch 3 months, 2 weeks ago 
target/i386/tcg/access.c | 4 ++++
1 file changed, 4 insertions(+)
[RFC PATCH] target/i386: allow access_ptr to force slow path on failed probe
Posted by Alex Bennée 3 months, 2 weeks ago 
When we are using TCG plugin memory callbacks probe_access_internal
will return TLB_MMIO to force the slow path for memory access. This
results in probe_access returning NULL but the x86 access_ptr function
happily accepts an empty haddr resulting in segfault hilarity.

Check for an empty haddr to prevent the segfault and enable plugins to
track all the memory operations for the x86 save/restore helpers.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2489
Fixes: 6d03226b42 (plugins: force slow path when plugins instrument memory ops)
---
 target/i386/tcg/access.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/target/i386/tcg/access.c b/target/i386/tcg/access.c
index 56a1181ea5..8ea5c453a0 100644
--- a/target/i386/tcg/access.c
+++ b/target/i386/tcg/access.c
@@ -58,6 +58,10 @@ static void *access_ptr(X86Access *ac, vaddr addr, unsigned len)
 
     assert(addr >= ac->vaddr);
 
+    if (!ac->haddr1) {
+        return NULL;
+    }
+
 #ifdef CONFIG_USER_ONLY
     assert(offset <= ac->size1 - len);
     return ac->haddr1 + offset;
-- 
2.39.2
Re: [RFC PATCH] target/i386: allow access_ptr to force slow path on failed probe
Posted by Richard Henderson 3 months, 2 weeks ago 
On 8/8/24 02:02, Alex Bennée wrote:
> When we are using TCG plugin memory callbacks probe_access_internal
> will return TLB_MMIO to force the slow path for memory access. This
> results in probe_access returning NULL but the x86 access_ptr function
> happily accepts an empty haddr resulting in segfault hilarity.
> 
> Check for an empty haddr to prevent the segfault and enable plugins to
> track all the memory operations for the x86 save/restore helpers.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2489
> Fixes: 6d03226b42 (plugins: force slow path when plugins instrument memory ops)
> ---
>   target/i386/tcg/access.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/target/i386/tcg/access.c b/target/i386/tcg/access.c
> index 56a1181ea5..8ea5c453a0 100644
> --- a/target/i386/tcg/access.c
> +++ b/target/i386/tcg/access.c
> @@ -58,6 +58,10 @@ static void *access_ptr(X86Access *ac, vaddr addr, unsigned len)
>   
>       assert(addr >= ac->vaddr);
>   
> +    if (!ac->haddr1) {
> +        return NULL;
> +    }
> +
>   #ifdef CONFIG_USER_ONLY
>       assert(offset <= ac->size1 - len);
>       return ac->haddr1 + offset;

You need to remove the test_ptr macro below as well.


r~
Re: [RFC PATCH] target/i386: allow access_ptr to force slow path on failed probe
Posted by Alex Bennée 3 months, 1 week ago 
Richard Henderson <richard.henderson@linaro.org> writes:

> On 8/8/24 02:02, Alex Bennée wrote:
>> When we are using TCG plugin memory callbacks probe_access_internal
>> will return TLB_MMIO to force the slow path for memory access. This
>> results in probe_access returning NULL but the x86 access_ptr function
>> happily accepts an empty haddr resulting in segfault hilarity.
>> Check for an empty haddr to prevent the segfault and enable plugins
>> to
>> track all the memory operations for the x86 save/restore helpers.
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2489
>> Fixes: 6d03226b42 (plugins: force slow path when plugins instrument memory ops)
>> ---
>>   target/i386/tcg/access.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>> diff --git a/target/i386/tcg/access.c b/target/i386/tcg/access.c
>> index 56a1181ea5..8ea5c453a0 100644
>> --- a/target/i386/tcg/access.c
>> +++ b/target/i386/tcg/access.c
>> @@ -58,6 +58,10 @@ static void *access_ptr(X86Access *ac, vaddr addr, unsigned len)
>>         assert(addr >= ac->vaddr);
>>   +    if (!ac->haddr1) {
>> +        return NULL;
>> +    }
>> +
>>   #ifdef CONFIG_USER_ONLY
>>       assert(offset <= ac->size1 - len);
>>       return ac->haddr1 + offset;
>
> You need to remove the test_ptr macro below as well.

So we fall-back to the slow path for linux-user as well?

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro
Re: [RFC PATCH] target/i386: allow access_ptr to force slow path on failed probe
Posted by Richard Henderson 3 months, 1 week ago 
On 8/13/24 23:12, Alex Bennée wrote:
> Richard Henderson <richard.henderson@linaro.org> writes:
> 
>> On 8/8/24 02:02, Alex Bennée wrote:
>>> When we are using TCG plugin memory callbacks probe_access_internal
>>> will return TLB_MMIO to force the slow path for memory access. This
>>> results in probe_access returning NULL but the x86 access_ptr function
>>> happily accepts an empty haddr resulting in segfault hilarity.
>>> Check for an empty haddr to prevent the segfault and enable plugins
>>> to
>>> track all the memory operations for the x86 save/restore helpers.
>>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2489
>>> Fixes: 6d03226b42 (plugins: force slow path when plugins instrument memory ops)
>>> ---
>>>    target/i386/tcg/access.c | 4 ++++
>>>    1 file changed, 4 insertions(+)
>>> diff --git a/target/i386/tcg/access.c b/target/i386/tcg/access.c
>>> index 56a1181ea5..8ea5c453a0 100644
>>> --- a/target/i386/tcg/access.c
>>> +++ b/target/i386/tcg/access.c
>>> @@ -58,6 +58,10 @@ static void *access_ptr(X86Access *ac, vaddr addr, unsigned len)
>>>          assert(addr >= ac->vaddr);
>>>    +    if (!ac->haddr1) {
>>> +        return NULL;
>>> +    }
>>> +
>>>    #ifdef CONFIG_USER_ONLY
>>>        assert(offset <= ac->size1 - len);
>>>        return ac->haddr1 + offset;
>>
>> You need to remove the test_ptr macro below as well.
> 
> So we fall-back to the slow path for linux-user as well?

So we don't ignore the null, and dereference it anyway.

r~
Re: [RFC PATCH] target/i386: allow access_ptr to force slow path on failed probe
Posted by Alexandre IOOSS 3 months, 2 weeks ago 
On 8/7/24 18:02, Alex Bennée wrote:
> When we are using TCG plugin memory callbacks probe_access_internal
> will return TLB_MMIO to force the slow path for memory access. This
> results in probe_access returning NULL but the x86 access_ptr function
> happily accepts an empty haddr resulting in segfault hilarity.
> 
> Check for an empty haddr to prevent the segfault and enable plugins to
> track all the memory operations for the x86 save/restore helpers.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2489
> Fixes: 6d03226b42 (plugins: force slow path when plugins instrument memory ops)
> ---
>   target/i386/tcg/access.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/target/i386/tcg/access.c b/target/i386/tcg/access.c
> index 56a1181ea5..8ea5c453a0 100644
> --- a/target/i386/tcg/access.c
> +++ b/target/i386/tcg/access.c
> @@ -58,6 +58,10 @@ static void *access_ptr(X86Access *ac, vaddr addr, unsigned len)
>   
>       assert(addr >= ac->vaddr);
>   
> +    if (!ac->haddr1) {
> +        return NULL;
> +    }
> +
>   #ifdef CONFIG_USER_ONLY
>       assert(offset <= ac->size1 - len);
>       return ac->haddr1 + offset;

I confirm that this patch fixes the crash I reported here: 
https://gitlab.com/qemu-project/qemu/-/issues/2489

Reviewed-by: Alexandre Iooss <erdnaxe@crans.org>

Thanks,
-- 
Alexandre

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.