* [Qemu-devel] [PATCH v2] translate-all: Bugfix for user-mode self-modifying code in 2 page long TB
@ 2016-07-06 7:54 Stanislav Shmarov
2016-07-06 10:21 ` Sergey Fedorov
0 siblings, 1 reply; 6+ messages in thread
From: Stanislav Shmarov @ 2016-07-06 7:54 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Peter Crosthwaite, Richard Henderson,
Sergey Fedorov, Stanislav Shmarov
In user-mode emulation Translation Block can consist of 2 guest pages.
In that case QEMU also mprotects 2 host pages that are dedicated for
guest memory, containing instructions. QEMU detects self-modifying code
with SEGFAULT signal processing.
In case if instruction in 1st page is modifying memory of 2nd
page (or vice versa) QEMU will mark 2nd page with PAGE_WRITE,
invalidate TB, generate new TB contatining 1 guest instruction and
exit to CPU loop. QEMU won't call mprotect, and new TB will cause
same SEGFAULT. Page will have both PAGE_WRITE_ORG and PAGE_WRITE
flags, so QEMU will handle the signal as guest binary problem,
and exit with guest SEGFAULT.
Solution is retranslate TB before marking pages as PAGE_WRITE,
and remove protection with mprotect on second SEGFAULT.
Signed-off-by: Stanislav Shmarov <snarpix@gmail.com>
---
v2: Moved setting PAGE_WRITE flag to separte loop, to cover cases,
pointed by Sergey Fedorov.
translate-all.c | 17 +++++++++++------
1 file changed, 11 insertions(+), 6 deletions(-)
diff --git a/translate-all.c b/translate-all.c
index eaa95e4..fb3743f 100644
--- a/translate-all.c
+++ b/translate-all.c
@@ -2020,13 +2020,8 @@ int page_unprotect(target_ulong address, uintptr_t pc)
host_start = address & qemu_host_page_mask;
host_end = host_start + qemu_host_page_size;
- prot = 0;
for (addr = host_start ; addr < host_end ; addr += TARGET_PAGE_SIZE) {
- p = page_find(addr >> TARGET_PAGE_BITS);
- p->flags |= PAGE_WRITE;
- prot |= p->flags;
-
- /* and since the content will be modified, we must invalidate
+ /* Since the content will be modified, we must invalidate
the corresponding translated code. */
if (tb_invalidate_phys_page(addr, pc)) {
mmap_unlock();
@@ -2036,6 +2031,16 @@ int page_unprotect(target_ulong address, uintptr_t pc)
tb_invalidate_check(addr);
#endif
}
+
+ /* If we got here, current TB have been retranslated (in case of
+ * self-modifying code), now it's safe to remove page protection.
+ */
+ prot = 0;
+ for (addr = host_start ; addr < host_end ; addr += TARGET_PAGE_SIZE) {
+ p = page_find(addr >> TARGET_PAGE_BITS);
+ p->flags |= PAGE_WRITE;
+ prot |= p->flags;
+ }
mprotect((void *)g2h(host_start), qemu_host_page_size,
prot & PAGE_BITS);
--
1.9.3
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH v2] translate-all: Bugfix for user-mode self-modifying code in 2 page long TB
2016-07-06 7:54 [Qemu-devel] [PATCH v2] translate-all: Bugfix for user-mode self-modifying code in 2 page long TB Stanislav Shmarov
@ 2016-07-06 10:21 ` Sergey Fedorov
2016-07-06 12:01 ` Стас Шмаров
0 siblings, 1 reply; 6+ messages in thread
From: Sergey Fedorov @ 2016-07-06 10:21 UTC (permalink / raw)
To: Stanislav Shmarov, qemu-devel
Cc: Paolo Bonzini, Peter Crosthwaite, Richard Henderson
On 06/07/16 10:54, Stanislav Shmarov wrote:
> In user-mode emulation Translation Block can consist of 2 guest pages.
> In that case QEMU also mprotects 2 host pages that are dedicated for
> guest memory, containing instructions. QEMU detects self-modifying code
> with SEGFAULT signal processing.
>
> In case if instruction in 1st page is modifying memory of 2nd
> page (or vice versa) QEMU will mark 2nd page with PAGE_WRITE,
> invalidate TB, generate new TB contatining 1 guest instruction and
> exit to CPU loop. QEMU won't call mprotect, and new TB will cause
> same SEGFAULT. Page will have both PAGE_WRITE_ORG and PAGE_WRITE
> flags, so QEMU will handle the signal as guest binary problem,
> and exit with guest SEGFAULT.
>
> Solution is retranslate TB before marking pages as PAGE_WRITE,
> and remove protection with mprotect on second SEGFAULT.
>
> Signed-off-by: Stanislav Shmarov <snarpix@gmail.com>
Reviewed-by: Sergey Fedorov <sergey.fedorov@linaro.org>
> ---
> v2: Moved setting PAGE_WRITE flag to separte loop, to cover cases,
> pointed by Sergey Fedorov.
>
> translate-all.c | 17 +++++++++++------
> 1 file changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/translate-all.c b/translate-all.c
> index eaa95e4..fb3743f 100644
> --- a/translate-all.c
> +++ b/translate-all.c
> @@ -2020,13 +2020,8 @@ int page_unprotect(target_ulong address, uintptr_t pc)
> host_start = address & qemu_host_page_mask;
> host_end = host_start + qemu_host_page_size;
>
> - prot = 0;
> for (addr = host_start ; addr < host_end ; addr += TARGET_PAGE_SIZE) {
> - p = page_find(addr >> TARGET_PAGE_BITS);
> - p->flags |= PAGE_WRITE;
> - prot |= p->flags;
> -
> - /* and since the content will be modified, we must invalidate
> + /* Since the content will be modified, we must invalidate
> the corresponding translated code. */
> if (tb_invalidate_phys_page(addr, pc)) {
> mmap_unlock();
> @@ -2036,6 +2031,16 @@ int page_unprotect(target_ulong address, uintptr_t pc)
> tb_invalidate_check(addr);
> #endif
> }
> +
> + /* If we got here, current TB have been retranslated (in case of
> + * self-modifying code), now it's safe to remove page protection.
> + */
> + prot = 0;
> + for (addr = host_start ; addr < host_end ; addr += TARGET_PAGE_SIZE) {
> + p = page_find(addr >> TARGET_PAGE_BITS);
> + p->flags |= PAGE_WRITE;
> + prot |= p->flags;
> + }
> mprotect((void *)g2h(host_start), qemu_host_page_size,
> prot & PAGE_BITS);
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH v2] translate-all: Bugfix for user-mode self-modifying code in 2 page long TB
2016-07-06 10:21 ` Sergey Fedorov
@ 2016-07-06 12:01 ` Стас Шмаров
2016-07-06 12:53 ` Sergey Fedorov
0 siblings, 1 reply; 6+ messages in thread
From: Стас Шмаров @ 2016-07-06 12:01 UTC (permalink / raw)
To: Sergey Fedorov
Cc: qemu-devel, Paolo Bonzini, Peter Crosthwaite, Richard Henderson
May be this can be done faster.
After invalidation of current TB, we know next executed TB will be 1
instruction long and will write to same address, and will cause segfault.
In sigfault handler write protection to this page will be disabled.
So, we can retranslate TB, and remove protection before execution of new
TB. It will be one signal less.
2016-07-06 13:21 GMT+03:00 Sergey Fedorov <sergey.fedorov@linaro.org>:
> On 06/07/16 10:54, Stanislav Shmarov wrote:
> > In user-mode emulation Translation Block can consist of 2 guest pages.
> > In that case QEMU also mprotects 2 host pages that are dedicated for
> > guest memory, containing instructions. QEMU detects self-modifying code
> > with SEGFAULT signal processing.
> >
> > In case if instruction in 1st page is modifying memory of 2nd
> > page (or vice versa) QEMU will mark 2nd page with PAGE_WRITE,
> > invalidate TB, generate new TB contatining 1 guest instruction and
> > exit to CPU loop. QEMU won't call mprotect, and new TB will cause
> > same SEGFAULT. Page will have both PAGE_WRITE_ORG and PAGE_WRITE
> > flags, so QEMU will handle the signal as guest binary problem,
> > and exit with guest SEGFAULT.
> >
> > Solution is retranslate TB before marking pages as PAGE_WRITE,
> > and remove protection with mprotect on second SEGFAULT.
> >
> > Signed-off-by: Stanislav Shmarov <snarpix@gmail.com>
>
> Reviewed-by: Sergey Fedorov <sergey.fedorov@linaro.org>
>
> > ---
> > v2: Moved setting PAGE_WRITE flag to separte loop, to cover cases,
> > pointed by Sergey Fedorov.
> >
> > translate-all.c | 17 +++++++++++------
> > 1 file changed, 11 insertions(+), 6 deletions(-)
> >
> > diff --git a/translate-all.c b/translate-all.c
> > index eaa95e4..fb3743f 100644
> > --- a/translate-all.c
> > +++ b/translate-all.c
> > @@ -2020,13 +2020,8 @@ int page_unprotect(target_ulong address,
> uintptr_t pc)
> > host_start = address & qemu_host_page_mask;
> > host_end = host_start + qemu_host_page_size;
> >
> > - prot = 0;
> > for (addr = host_start ; addr < host_end ; addr +=
> TARGET_PAGE_SIZE) {
> > - p = page_find(addr >> TARGET_PAGE_BITS);
> > - p->flags |= PAGE_WRITE;
> > - prot |= p->flags;
> > -
> > - /* and since the content will be modified, we must
> invalidate
> > + /* Since the content will be modified, we must invalidate
> > the corresponding translated code. */
> > if (tb_invalidate_phys_page(addr, pc)) {
> > mmap_unlock();
> > @@ -2036,6 +2031,16 @@ int page_unprotect(target_ulong address,
> uintptr_t pc)
> > tb_invalidate_check(addr);
> > #endif
> > }
> > +
> > + /* If we got here, current TB have been retranslated (in case of
> > + * self-modifying code), now it's safe to remove page
> protection.
> > + */
> > + prot = 0;
> > + for (addr = host_start ; addr < host_end ; addr +=
> TARGET_PAGE_SIZE) {
> > + p = page_find(addr >> TARGET_PAGE_BITS);
> > + p->flags |= PAGE_WRITE;
> > + prot |= p->flags;
> > + }
> > mprotect((void *)g2h(host_start), qemu_host_page_size,
> > prot & PAGE_BITS);
> >
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH v2] translate-all: Bugfix for user-mode self-modifying code in 2 page long TB
2016-07-06 12:01 ` Стас Шмаров
@ 2016-07-06 12:53 ` Sergey Fedorov
2016-07-06 13:22 ` Stanislav Shmarov
0 siblings, 1 reply; 6+ messages in thread
From: Sergey Fedorov @ 2016-07-06 12:53 UTC (permalink / raw)
To: Стас Шмаров
Cc: qemu-devel, Paolo Bonzini, Peter Crosthwaite, Richard Henderson
On 06/07/16 15:01, Стас Шмаров wrote:
> May be this can be done faster.
> After invalidation of current TB, we know next executed TB will be 1
> instruction long and will write to same address, and will cause
> segfault. In sigfault handler write protection to this page will be
> disabled.
> So, we can retranslate TB, and remove protection before execution of
> new TB. It will be one signal less.
You mean not to return 2 immediately but rather to remember the desired
return value and keep invalidating guest pages, then go ahead and call
mprotect() to enable writes to the host page?
Thanks,
Sergey
>
> 2016-07-06 13:21 GMT+03:00 Sergey Fedorov <sergey.fedorov@linaro.org
> <mailto:sergey.fedorov@linaro.org>>:
>
> On 06/07/16 10:54, Stanislav Shmarov wrote:
> > In user-mode emulation Translation Block can consist of 2 guest
> pages.
> > In that case QEMU also mprotects 2 host pages that are dedicated for
> > guest memory, containing instructions. QEMU detects
> self-modifying code
> > with SEGFAULT signal processing.
> >
> > In case if instruction in 1st page is modifying memory of 2nd
> > page (or vice versa) QEMU will mark 2nd page with PAGE_WRITE,
> > invalidate TB, generate new TB contatining 1 guest instruction and
> > exit to CPU loop. QEMU won't call mprotect, and new TB will cause
> > same SEGFAULT. Page will have both PAGE_WRITE_ORG and PAGE_WRITE
> > flags, so QEMU will handle the signal as guest binary problem,
> > and exit with guest SEGFAULT.
> >
> > Solution is retranslate TB before marking pages as PAGE_WRITE,
> > and remove protection with mprotect on second SEGFAULT.
> >
> > Signed-off-by: Stanislav Shmarov <snarpix@gmail.com
> <mailto:snarpix@gmail.com>>
>
> Reviewed-by: Sergey Fedorov <sergey.fedorov@linaro.org
> <mailto:sergey.fedorov@linaro.org>>
>
> > ---
> > v2: Moved setting PAGE_WRITE flag to separte loop, to cover cases,
> > pointed by Sergey Fedorov.
> >
> > translate-all.c | 17 +++++++++++------
> > 1 file changed, 11 insertions(+), 6 deletions(-)
> >
> > diff --git a/translate-all.c b/translate-all.c
> > index eaa95e4..fb3743f 100644
> > --- a/translate-all.c
> > +++ b/translate-all.c
> > @@ -2020,13 +2020,8 @@ int page_unprotect(target_ulong address,
> uintptr_t pc)
> > host_start = address & qemu_host_page_mask;
> > host_end = host_start + qemu_host_page_size;
> >
> > - prot = 0;
> > for (addr = host_start ; addr < host_end ; addr +=
> TARGET_PAGE_SIZE) {
> > - p = page_find(addr >> TARGET_PAGE_BITS);
> > - p->flags |= PAGE_WRITE;
> > - prot |= p->flags;
> > -
> > - /* and since the content will be modified, we must
> invalidate
> > + /* Since the content will be modified, we must
> invalidate
> > the corresponding translated code. */
> > if (tb_invalidate_phys_page(addr, pc)) {
> > mmap_unlock();
> > @@ -2036,6 +2031,16 @@ int page_unprotect(target_ulong address,
> uintptr_t pc)
> > tb_invalidate_check(addr);
> > #endif
> > }
> > +
> > + /* If we got here, current TB have been retranslated
> (in case of
> > + * self-modifying code), now it's safe to remove page
> protection.
> > + */
> > + prot = 0;
> > + for (addr = host_start ; addr < host_end ; addr +=
> TARGET_PAGE_SIZE) {
> > + p = page_find(addr >> TARGET_PAGE_BITS);
> > + p->flags |= PAGE_WRITE;
> > + prot |= p->flags;
> > + }
> > mprotect((void *)g2h(host_start), qemu_host_page_size,
> > prot & PAGE_BITS);
> >
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH v2] translate-all: Bugfix for user-mode self-modifying code in 2 page long TB
2016-07-06 12:53 ` Sergey Fedorov
@ 2016-07-06 13:22 ` Stanislav Shmarov
2016-07-06 13:22 ` Sergey Fedorov
0 siblings, 1 reply; 6+ messages in thread
From: Stanislav Shmarov @ 2016-07-06 13:22 UTC (permalink / raw)
To: Sergey Fedorov
Cc: Richard Henderson, Peter Crosthwaite, Paolo Bonzini, qemu-devel
Yes, exactly.
There is no point for returning to main loop immediately when current TB is
found on host page and is retranslated. We can continue invalidation of
TBs, and finally remove host page write protection. So there will be no
second SEGFAULT.
And when generating TB for next instructions, host page will be locked
again, if TB includes instructions from that page.
Stanislav.
6 Июл 2016 г. 15:53 пользователь "Sergey Fedorov" <sergey.fedorov@linaro.org>
написал:
> On 06/07/16 15:01, Стас Шмаров wrote:
>
> May be this can be done faster.
> After invalidation of current TB, we know next executed TB will be 1
> instruction long and will write to same address, and will cause segfault.
> In sigfault handler write protection to this page will be disabled.
> So, we can retranslate TB, and remove protection before execution of new
> TB. It will be one signal less.
>
>
> You mean not to return 2 immediately but rather to remember the desired
> return value and keep invalidating guest pages, then go ahead and call
> mprotect() to enable writes to the host page?
>
> Thanks,
> Sergey
>
>
> 2016-07-06 13:21 GMT+03:00 Sergey Fedorov <sergey.fedorov@linaro.org>:
>
>> On 06/07/16 10:54, Stanislav Shmarov wrote:
>> > In user-mode emulation Translation Block can consist of 2 guest pages.
>> > In that case QEMU also mprotects 2 host pages that are dedicated for
>> > guest memory, containing instructions. QEMU detects self-modifying code
>> > with SEGFAULT signal processing.
>> >
>> > In case if instruction in 1st page is modifying memory of 2nd
>> > page (or vice versa) QEMU will mark 2nd page with PAGE_WRITE,
>> > invalidate TB, generate new TB contatining 1 guest instruction and
>> > exit to CPU loop. QEMU won't call mprotect, and new TB will cause
>> > same SEGFAULT. Page will have both PAGE_WRITE_ORG and PAGE_WRITE
>> > flags, so QEMU will handle the signal as guest binary problem,
>> > and exit with guest SEGFAULT.
>> >
>> > Solution is retranslate TB before marking pages as PAGE_WRITE,
>> > and remove protection with mprotect on second SEGFAULT.
>> >
>> > Signed-off-by: Stanislav Shmarov <snarpix@gmail.com>
>>
>> Reviewed-by: Sergey Fedorov <sergey.fedorov@linaro.org>
>>
>> > ---
>> > v2: Moved setting PAGE_WRITE flag to separte loop, to cover cases,
>> > pointed by Sergey Fedorov.
>> >
>> > translate-all.c | 17 +++++++++++------
>> > 1 file changed, 11 insertions(+), 6 deletions(-)
>> >
>> > diff --git a/translate-all.c b/translate-all.c
>> > index eaa95e4..fb3743f 100644
>> > --- a/translate-all.c
>> > +++ b/translate-all.c
>> > @@ -2020,13 +2020,8 @@ int page_unprotect(target_ulong address,
>> uintptr_t pc)
>> > host_start = address & qemu_host_page_mask;
>> > host_end = host_start + qemu_host_page_size;
>> >
>> > - prot = 0;
>> > for (addr = host_start ; addr < host_end ; addr +=
>> TARGET_PAGE_SIZE) {
>> > - p = page_find(addr >> TARGET_PAGE_BITS);
>> > - p->flags |= PAGE_WRITE;
>> > - prot |= p->flags;
>> > -
>> > - /* and since the content will be modified, we must
>> invalidate
>> > + /* Since the content will be modified, we must invalidate
>> > the corresponding translated code. */
>> > if (tb_invalidate_phys_page(addr, pc)) {
>> > mmap_unlock();
>> > @@ -2036,6 +2031,16 @@ int page_unprotect(target_ulong address,
>> uintptr_t pc)
>> > tb_invalidate_check(addr);
>> > #endif
>> > }
>> > +
>> > + /* If we got here, current TB have been retranslated (in case
>> of
>> > + * self-modifying code), now it's safe to remove page
>> protection.
>> > + */
>> > + prot = 0;
>> > + for (addr = host_start ; addr < host_end ; addr +=
>> TARGET_PAGE_SIZE) {
>> > + p = page_find(addr >> TARGET_PAGE_BITS);
>> > + p->flags |= PAGE_WRITE;
>> > + prot |= p->flags;
>> > + }
>> > mprotect((void *)g2h(host_start), qemu_host_page_size,
>> > prot & PAGE_BITS);
>> >
>>
>>
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH v2] translate-all: Bugfix for user-mode self-modifying code in 2 page long TB
2016-07-06 13:22 ` Stanislav Shmarov
@ 2016-07-06 13:22 ` Sergey Fedorov
0 siblings, 0 replies; 6+ messages in thread
From: Sergey Fedorov @ 2016-07-06 13:22 UTC (permalink / raw)
To: Stanislav Shmarov
Cc: Richard Henderson, Peter Crosthwaite, Paolo Bonzini, qemu-devel
On 06/07/16 16:22, Stanislav Shmarov wrote:
>
> Yes, exactly.
>
> There is no point for returning to main loop immediately when current
> TB is found on host page and is retranslated. We can continue
> invalidation of TBs, and finally remove host page write protection. So
> there will be no second SEGFAULT.
>
> And when generating TB for next instructions, host page will be locked
> again, if TB includes instructions from that page.
>
I think that could work.
Regards,
Sergey
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-07-06 13:22 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-07-06 7:54 [Qemu-devel] [PATCH v2] translate-all: Bugfix for user-mode self-modifying code in 2 page long TB Stanislav Shmarov
2016-07-06 10:21 ` Sergey Fedorov
2016-07-06 12:01 ` Стас Шмаров
2016-07-06 12:53 ` Sergey Fedorov
2016-07-06 13:22 ` Stanislav Shmarov
2016-07-06 13:22 ` Sergey Fedorov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).