| Message ID | 20210204014509.882821-5-richard.henderson@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | TCI fixes and cleanups | expand |
Richard Henderson <richard.henderson@linaro.org> writes: > The use in tcg_tb_lookup is given a random pc that comes from the pc > of a signal handler. Do not assert that the pointer is already within > the code gen buffer at all, much less the writable mirror of it. Surely we are asserting that - or at least you can find a rt entry for the pointer passed (which we always expect to work)? > > Fixes: db0c51a3803 > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > > For TCI, this indicates a bug in handle_cpu_signal, in that we > are taking PC from the host signal frame. Which is, nearly, > unrelated to TCI at all. > > The TCI "pc" is tci_tb_ptr (fixed in the next patch to at least > be thread-local). We update this only on calls, since we don't > expect SEGV during the interpretation loop. Which works ok for > softmmu, in which we pass down pc by hand to the helpers, but > is not ok for user-only, where we simply perform the raw memory > operation. > > I don't know how to fix this, exactly. Probably by storing to > tci_tb_ptr before each qemu_ld/qemu_st operation, with barriers. > Then Doing the Right Thing in handle_cpu_signal. And perhaps > by clearing tci_tb_ptr whenever we're not expecting a SEGV on > behalf of the guest (and thus anything left is a qemu host bug). > > --- > v2: Retain full struct initialization > --- > tcg/tcg.c | 20 ++++++++++++++++++-- > 1 file changed, 18 insertions(+), 2 deletions(-) > > diff --git a/tcg/tcg.c b/tcg/tcg.c > index bbe3dcee03..2991112829 100644 > --- a/tcg/tcg.c > +++ b/tcg/tcg.c > @@ -513,11 +513,21 @@ static void tcg_region_trees_init(void) > } > } > > -static struct tcg_region_tree *tc_ptr_to_region_tree(const void *cp) > +static struct tcg_region_tree *tc_ptr_to_region_tree(const void *p) > { > - void *p = tcg_splitwx_to_rw(cp); > size_t region_idx; > > + /* > + * Like tcg_splitwx_to_rw, with no assert. The pc may come from > + * a signal handler over which the caller has no control. > + */ > + if (!in_code_gen_buffer(p)) { > + p -= tcg_splitwx_diff; > + if (!in_code_gen_buffer(p)) { > + return NULL; > + } > + } > + > if (p < region.start_aligned) { > region_idx = 0; > } else { > @@ -536,6 +546,7 @@ void tcg_tb_insert(TranslationBlock *tb) > { > struct tcg_region_tree *rt = tc_ptr_to_region_tree(tb->tc.ptr); > > + g_assert(rt != NULL); > qemu_mutex_lock(&rt->lock); > g_tree_insert(rt->tree, &tb->tc, tb); > qemu_mutex_unlock(&rt->lock); > @@ -545,6 +556,7 @@ void tcg_tb_remove(TranslationBlock *tb) > { > struct tcg_region_tree *rt = tc_ptr_to_region_tree(tb->tc.ptr); > > + g_assert(rt != NULL); > qemu_mutex_lock(&rt->lock); > g_tree_remove(rt->tree, &tb->tc); > qemu_mutex_unlock(&rt->lock); > @@ -561,6 +573,10 @@ TranslationBlock *tcg_tb_lookup(uintptr_t tc_ptr) > TranslationBlock *tb; > struct tb_tc s = { .ptr = (void *)tc_ptr }; > > + if (rt == NULL) { > + return NULL; > + } > + > qemu_mutex_lock(&rt->lock); > tb = g_tree_lookup(rt->tree, &s); > qemu_mutex_unlock(&rt->lock); -- Alex Bennée
On 2/4/21 5:01 AM, Alex Bennée wrote: > > Richard Henderson <richard.henderson@linaro.org> writes: > >> The use in tcg_tb_lookup is given a random pc that comes from the pc >> of a signal handler. Do not assert that the pointer is already within >> the code gen buffer at all, much less the writable mirror of it. > > Surely we are asserting that - or at least you can find a rt entry for > the pointer passed (which we always expect to work)? What? No. The pointer could be anything at all, depending on any other bug within qemu. r~
Richard Henderson <richard.henderson@linaro.org> writes: > On 2/4/21 5:01 AM, Alex Bennée wrote: >> >> Richard Henderson <richard.henderson@linaro.org> writes: >> >>> The use in tcg_tb_lookup is given a random pc that comes from the pc >>> of a signal handler. Do not assert that the pointer is already within >>> the code gen buffer at all, much less the writable mirror of it. >> >> Surely we are asserting that - or at least you can find a rt entry for >> the pointer passed (which we always expect to work)? > > What? No. The pointer could be anything at all, depending on any other bug > within qemu. But you do assert it: struct tcg_region_tree *rt = tc_ptr_to_region_tree(tb->tc.ptr); g_assert(rt != NULL); and rt is only NULL when it's !in_code_gen_buffer(p). In tcg_tb_lookup you haven't removed an assert - you just ensure you don't fail if it's not. > > > r~ -- Alex Bennée
On 2/4/21 8:45 AM, Alex Bennée wrote: > > Richard Henderson <richard.henderson@linaro.org> writes: > >> On 2/4/21 5:01 AM, Alex Bennée wrote: >>> >>> Richard Henderson <richard.henderson@linaro.org> writes: >>> >>>> The use in tcg_tb_lookup is given a random pc that comes from the pc >>>> of a signal handler. Do not assert that the pointer is already within >>>> the code gen buffer at all, much less the writable mirror of it. >>> >>> Surely we are asserting that - or at least you can find a rt entry for >>> the pointer passed (which we always expect to work)? >> >> What? No. The pointer could be anything at all, depending on any other bug >> within qemu. > > But you do assert it: > > struct tcg_region_tree *rt = tc_ptr_to_region_tree(tb->tc.ptr); > > g_assert(rt != NULL); > > and rt is only NULL when it's !in_code_gen_buffer(p). > > In tcg_tb_lookup you haven't removed an assert - you just ensure you > don't fail if it's not. I see your confusion. The arbitrary pointer is the one that is presented to tcg_tb_lookup, and passed on to tc_ptr_to_region_tree. The (dynamic) assert has been removed by not calling tcg_splitwx_to_rw in tc_ptr_to_region_tree, as called from tcg_tb_lookup. But tcg_tb_insert and tcg_tb_remove do not receive arbitrary pointers -- they always get a TranslationBlock. So I retain an assert along those paths. (As opposed to SEGV on the next lines, I suppose.) Suggestions on how to reword the commit? Just include most of the above? r~
diff --git a/tcg/tcg.c b/tcg/tcg.c index bbe3dcee03..2991112829 100644 --- a/tcg/tcg.c +++ b/tcg/tcg.c @@ -513,11 +513,21 @@ static void tcg_region_trees_init(void) } } -static struct tcg_region_tree *tc_ptr_to_region_tree(const void *cp) +static struct tcg_region_tree *tc_ptr_to_region_tree(const void *p) { - void *p = tcg_splitwx_to_rw(cp); size_t region_idx; + /* + * Like tcg_splitwx_to_rw, with no assert. The pc may come from + * a signal handler over which the caller has no control. + */ + if (!in_code_gen_buffer(p)) { + p -= tcg_splitwx_diff; + if (!in_code_gen_buffer(p)) { + return NULL; + } + } + if (p < region.start_aligned) { region_idx = 0; } else { @@ -536,6 +546,7 @@ void tcg_tb_insert(TranslationBlock *tb) { struct tcg_region_tree *rt = tc_ptr_to_region_tree(tb->tc.ptr); + g_assert(rt != NULL); qemu_mutex_lock(&rt->lock); g_tree_insert(rt->tree, &tb->tc, tb); qemu_mutex_unlock(&rt->lock); @@ -545,6 +556,7 @@ void tcg_tb_remove(TranslationBlock *tb) { struct tcg_region_tree *rt = tc_ptr_to_region_tree(tb->tc.ptr); + g_assert(rt != NULL); qemu_mutex_lock(&rt->lock); g_tree_remove(rt->tree, &tb->tc); qemu_mutex_unlock(&rt->lock); @@ -561,6 +573,10 @@ TranslationBlock *tcg_tb_lookup(uintptr_t tc_ptr) TranslationBlock *tb; struct tb_tc s = { .ptr = (void *)tc_ptr }; + if (rt == NULL) { + return NULL; + } + qemu_mutex_lock(&rt->lock); tb = g_tree_lookup(rt->tree, &s); qemu_mutex_unlock(&rt->lock);
The use in tcg_tb_lookup is given a random pc that comes from the pc of a signal handler. Do not assert that the pointer is already within the code gen buffer at all, much less the writable mirror of it. Fixes: db0c51a3803 Signed-off-by: Richard Henderson <richard.henderson@linaro.org> --- For TCI, this indicates a bug in handle_cpu_signal, in that we are taking PC from the host signal frame. Which is, nearly, unrelated to TCI at all. The TCI "pc" is tci_tb_ptr (fixed in the next patch to at least be thread-local). We update this only on calls, since we don't expect SEGV during the interpretation loop. Which works ok for softmmu, in which we pass down pc by hand to the helpers, but is not ok for user-only, where we simply perform the raw memory operation. I don't know how to fix this, exactly. Probably by storing to tci_tb_ptr before each qemu_ld/qemu_st operation, with barriers. Then Doing the Right Thing in handle_cpu_signal. And perhaps by clearing tci_tb_ptr whenever we're not expecting a SEGV on behalf of the guest (and thus anything left is a qemu host bug). --- v2: Retain full struct initialization --- tcg/tcg.c | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) -- 2.25.1