diff mbox series

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

Message ID 20240807160236.2478459-1-alex.bennee@linaro.org
State New
Headers show
Series [RFC] target/i386: allow access_ptr to force slow path on failed probe | expand

Commit Message

Alex Bennée Aug. 7, 2024, 4:02 p.m. UTC 
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(+)

Comments

Alexandre IOOSS Aug. 7, 2024, 4:12 p.m. UTC | #1 
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,
Richard Henderson Aug. 7, 2024, 11:10 p.m. UTC | #2 
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~
Alex Bennée Aug. 13, 2024, 1:12 p.m. UTC | #3 
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?
Richard Henderson Aug. 13, 2024, 2:48 p.m. UTC | #4 
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~
diff mbox series

Patch

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;