diff mbox series

[v1,02/36] target/riscv: Don't set write permissions on dirty PTEs

Message ID 6a38e1a02e9a3d38fc311809772a152f0d99fd7a.1575914822.git.alistair.francis@wdc.com
State New
Headers show
Series Add RISC-V Hypervisor Extension v0.5 | expand

Commit Message

Alistair Francis Dec. 9, 2019, 6:10 p.m. UTC 
Setting write permission on dirty PTEs results in userspace inside a
Hypervisor guest (VU) becoming corrupted. This appears to be because it
ends up with write permission in the second stage translation in cases
where we aren't doing a store.

Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
---
 target/riscv/cpu_helper.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

Comments

Palmer Dabbelt Jan. 6, 2020, 5:51 p.m. UTC | #1 
On Mon, 09 Dec 2019 10:10:45 PST (-0800), Alistair Francis wrote:
> Setting write permission on dirty PTEs results in userspace inside a
> Hypervisor guest (VU) becoming corrupted. This appears to be because it
> ends up with write permission in the second stage translation in cases
> where we aren't doing a store.
>
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
> ---
>  target/riscv/cpu_helper.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index 767c8762ac..0de3a468eb 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -344,10 +344,8 @@ restart:
>              if ((pte & PTE_X)) {
>                  *prot |= PAGE_EXEC;
>              }
> -            /* add write permission on stores or if the page is already dirty,
> -               so that we TLB miss on later writes to update the dirty bit */
> -            if ((pte & PTE_W) &&
> -                    (access_type == MMU_DATA_STORE || (pte & PTE_D))) {
> +            /* add write permission on stores */
> +            if ((pte & PTE_W) && (access_type == MMU_DATA_STORE)) {
>                  *prot |= PAGE_WRITE;
>              }
>              return TRANSLATE_SUCCESS;

This is at least the second time I've spent a day or two trying to figure out
what the right thing to do here is, and once again I'm lost.  I think what's
really getting me is the original comment: why would this cause us to TLB miss,
wouldn't it cause us to not TLB miss?

Assuming that's the case, it seems to me more like there's some missing fence
in whatever program is blowing up due to this -- maybe it's just reading from
the page before marking it as read-only, then relying on writes to trap without
doing the requisite fence.
Alistair Francis Jan. 7, 2020, 1:33 a.m. UTC | #2 
On Mon, Jan 6, 2020 at 9:51 AM Palmer Dabbelt <palmerdabbelt@google.com> wrote:
>
> On Mon, 09 Dec 2019 10:10:45 PST (-0800), Alistair Francis wrote:
> > Setting write permission on dirty PTEs results in userspace inside a
> > Hypervisor guest (VU) becoming corrupted. This appears to be because it
> > ends up with write permission in the second stage translation in cases
> > where we aren't doing a store.
> >
> > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> > Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
> > ---
> >  target/riscv/cpu_helper.c | 6 ++----
> >  1 file changed, 2 insertions(+), 4 deletions(-)
> >
> > diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> > index 767c8762ac..0de3a468eb 100644
> > --- a/target/riscv/cpu_helper.c
> > +++ b/target/riscv/cpu_helper.c
> > @@ -344,10 +344,8 @@ restart:
> >              if ((pte & PTE_X)) {
> >                  *prot |= PAGE_EXEC;
> >              }
> > -            /* add write permission on stores or if the page is already dirty,
> > -               so that we TLB miss on later writes to update the dirty bit */
> > -            if ((pte & PTE_W) &&
> > -                    (access_type == MMU_DATA_STORE || (pte & PTE_D))) {
> > +            /* add write permission on stores */
> > +            if ((pte & PTE_W) && (access_type == MMU_DATA_STORE)) {
> >                  *prot |= PAGE_WRITE;
> >              }
> >              return TRANSLATE_SUCCESS;
>
> This is at least the second time I've spent a day or two trying to figure out
> what the right thing to do here is, and once again I'm lost.  I think what's
> really getting me is the original comment: why would this cause us to TLB miss,
> wouldn't it cause us to not TLB miss?
>
> Assuming that's the case, it seems to me more like there's some missing fence
> in whatever program is blowing up due to this -- maybe it's just reading from
> the page before marking it as read-only, then relying on writes to trap without
> doing the requisite fence.

Ok, let's drop this for now then. I'll reinvestigate and if this is
still needed I'll add a more detailed explanation.

Alistair
diff mbox series

Patch

diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index 767c8762ac..0de3a468eb 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -344,10 +344,8 @@  restart:
             if ((pte & PTE_X)) {
                 *prot |= PAGE_EXEC;
             }
-            /* add write permission on stores or if the page is already dirty,
-               so that we TLB miss on later writes to update the dirty bit */
-            if ((pte & PTE_W) &&
-                    (access_type == MMU_DATA_STORE || (pte & PTE_D))) {
+            /* add write permission on stores */
+            if ((pte & PTE_W) && (access_type == MMU_DATA_STORE)) {
                 *prot |= PAGE_WRITE;
             }
             return TRANSLATE_SUCCESS;