target/i386: Fix CR2 handling for non-canonical addresses

[PATCH] target/i386: Fix CR2 handling for non-canonical addresses

Posted by Mathias Krause 6 months ago

Commit 3563362ddfae ("target/i386: Introduce structures for mmu_translate")
accidentally modified CR2 for non-canonical address exceptions while these
should lead to a #GP / #SS instead -- without changing CR2.

Fix that.

A KUT test for this was submitted as [1].

[1] https://lore.kernel.org/kvm/20250612141637.131314-1-minipli@grsecurity.net/

Fixes: 3563362ddfae ("target/i386: Introduce structures for mmu_translate")
Signed-off-by: Mathias Krause <minipli@grsecurity.net>
---
 target/i386/tcg/system/excp_helper.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/target/i386/tcg/system/excp_helper.c b/target/i386/tcg/system/excp_helper.c
index c16262158774..5f1023937fa8 100644
--- a/target/i386/tcg/system/excp_helper.c
+++ b/target/i386/tcg/system/excp_helper.c
@@ -593,7 +593,8 @@ static bool get_physical_address(CPUX86State *env, vaddr addr,
                 if (sext != 0 && sext != -1) {
                     *err = (TranslateFault){
                         .exception_index = EXCP0D_GPF,
-                        .cr2 = addr,
+                        /* non-canonical #GP doesn't change CR2 */
+                        .cr2 = env->cr[2],
                     };
                     return false;
                 }
-- 
2.30.2

Re: [PATCH] target/i386: Fix CR2 handling for non-canonical addresses

Posted by Paolo Bonzini 2 months ago

Queued, thanks.

Paolo

Re: [PATCH] target/i386: Fix CR2 handling for non-canonical addresses

Posted by Mathias Krause 4 months, 3 weeks ago

On 12.06.25 16:21, Mathias Krause wrote:
> Commit 3563362ddfae ("target/i386: Introduce structures for mmu_translate")
> accidentally modified CR2 for non-canonical address exceptions while these
> should lead to a #GP / #SS instead -- without changing CR2.
> 
> Fix that.
> 
> A KUT test for this was submitted as [1].
> 
> [1] https://lore.kernel.org/kvm/20250612141637.131314-1-minipli@grsecurity.net/
> 
> Fixes: 3563362ddfae ("target/i386: Introduce structures for mmu_translate")
> Signed-off-by: Mathias Krause <minipli@grsecurity.net>
> ---

Ping!

Paolo, Richard, any take on the patch? It's a regression fix that can
easily be verified with the KUT test failing after the commit mentioned
in the Fixes tag and succeeding again with the below fix applied.

Thanks,
Mathias

Re: [PATCH] target/i386: Fix CR2 handling for non-canonical addresses

Posted by Mathias Krause 2 months ago

On 7/21/25 12:17, Mathias Krause wrote:
> On 12.06.25 16:21, Mathias Krause wrote:
>> Commit 3563362ddfae ("target/i386: Introduce structures for mmu_translate")
>> accidentally modified CR2 for non-canonical address exceptions while these
>> should lead to a #GP / #SS instead -- without changing CR2.
>>
>> Fix that.
>>
>> A KUT test for this was submitted as [1].
>>
>> [1] https://lore.kernel.org/kvm/20250612141637.131314-1-minipli@grsecurity.net/
>>
>> Fixes: 3563362ddfae ("target/i386: Introduce structures for mmu_translate")
>> Signed-off-by: Mathias Krause <minipli@grsecurity.net>
>> ---
> 
> Ping!
> 
> Paolo, Richard, any take on the patch? It's a regression fix that can
> easily be verified with the KUT test failing after the commit mentioned
> in the Fixes tag and succeeding again with the below fix applied.

Ping, once more...

Re: [PATCH] target/i386: Fix CR2 handling for non-canonical addresses

Posted by Michael Tokarev 2 months ago

On 10/10/25 09:23, Mathias Krause wrote:
> On 7/21/25 12:17, Mathias Krause wrote:
>> On 12.06.25 16:21, Mathias Krause wrote:
>>> Commit 3563362ddfae ("target/i386: Introduce structures for mmu_translate")
>>> accidentally modified CR2 for non-canonical address exceptions while these
>>> should lead to a #GP / #SS instead -- without changing CR2.
>>>
>>> Fix that.
>>>
>>> A KUT test for this was submitted as [1].
>>>
>>> [1] https://lore.kernel.org/kvm/20250612141637.131314-1-minipli@grsecurity.net/
>>>
>>> Fixes: 3563362ddfae ("target/i386: Introduce structures for mmu_translate")
>>> Signed-off-by: Mathias Krause <minipli@grsecurity.net>

That feels like it should go to qemu-stable as well (for all active
stable qemu releases) (Cc'ing).

/mjt