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

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

[Alex Bennée]

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

[Alexandre IOOSS]

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

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

[Richard Henderson]

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

[Alex Bennée]

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

[Richard Henderson]

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~