| 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 |
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,
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~
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?
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 --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;
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(+)