[PATCH v2 1/1] target/riscv/pmp: Fix bug preventing

[PATCH v2 1/1] target/riscv/pmp: Fix bug preventing

[Chris Williams]

Hello. I hope you don't mind me resubmitting this patch. Please let me know if
I've formatted it incorrectly or if it needs more explanation. My previous
attempt probably wasn't formatted quite right. This is my first time
contributing to Qemu, so I'm keen to get it right - thanks.

This fixes an issue that prevents a RISC-V CPU from executing instructions
immediately from the base address of a PMP TOR region.

When jumping to an instruction in a PMP TOR region, pmp_hart_has_privs() is
called to validate the access. If this instruction is the very first word of a
PMP TOR region, at address 0 relative to the start address of the region, then
the access will fail. This is because pmp_hart_has_privs() is called with size
0 to perform this validation, causing this check...

e = pmp_is_in_range(env, i, addr + size - 1);

... to fail, as (addr + size - 1) falls below the base address of the PMP
region. Really, the access should succeed. For example, if I have a region
spanning 0x80d96000 to 0x88d95fff and the CPU jumps to 0x80d96000, then:

s = 0x80d96000
e = 0x80d95fff

And the validation fails. The size check proposed below catches these zero-size
instruction fetch access probes. The word alignment in pmpaddr{0-15} and
earlier instruction alignment checks should prevent the execution of
instructions over the upper boundary of the PMP region, though I'm happy to give
this more attention if this is a concern.

Signed-off-by: Chris Williams <diodesign@tuta.io <mailto:diodesign@tuta.io>>

diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
index d4f1007109..9308672e20 100644
--- a/target/riscv/pmp.c
+++ b/target/riscv/pmp.c
@@ -235,8 +235,9 @@ bool pmp_hart_has_privs(CPURISCVState *env, target_ulong
addr,
     /* 1.10 draft priv spec states there is an implicit order
          from low to high */
     for (i = 0; i < MAX_RISCV_PMPS; i++) {
+        /* catch zero-size instruction checks */
         s = pmp_is_in_range(env, i, addr);
-        e = pmp_is_in_range(env, i, addr + size - 1);
+        e = pmp_is_in_range(env, i, (size == 0) ? addr : addr + size - 1);

         /* partially inside */
         if ((s + e) == 1) {

Re: [PATCH v2 1/1] target/riscv/pmp: Fix bug preventing

[Alistair Francis]

On Sun, Oct 6, 2019 at 1:32 AM Chris Williams <diodesign@tuta.io> wrote:
>
> Hello. I hope you don't mind me resubmitting this patch. Please let me know if

Not at all!

Thanks for the patch.

In future if people don't respond you can just keep pinging your patch.

> I've formatted it incorrectly or if it needs more explanation. My previous
> attempt probably wasn't formatted quite right. This is my first time
> contributing to Qemu, so I'm keen to get it right - thanks.

This whole paragraph should not be in the commit. It can go below the
line though.

Also please use `git format-patch` to format the patch and then `git
send-email` to send the patch. There is a whole heap of detail here:
https://wiki.qemu.org/Contribute/SubmitAPatch

>
> This fixes an issue that prevents a RISC-V CPU from executing instructions
> immediately from the base address of a PMP TOR region.
>
> When jumping to an instruction in a PMP TOR region, pmp_hart_has_privs() is
> called to validate the access. If this instruction is the very first word of a
> PMP TOR region, at address 0 relative to the start address of the region, then
> the access will fail. This is because pmp_hart_has_privs() is called with size
> 0 to perform this validation, causing this check...
>
> e = pmp_is_in_range(env, i, addr + size - 1);
>
> ... to fail, as (addr + size - 1) falls below the base address of the PMP
> region. Really, the access should succeed. For example, if I have a region
> spanning 0x80d96000 to 0x88d95fff and the CPU jumps to 0x80d96000, then:
>
> s = 0x80d96000
> e = 0x80d95fff
>
> And the validation fails. The size check proposed below catches these zero-size
> instruction fetch access probes. The word alignment in pmpaddr{0-15} and
> earlier instruction alignment checks should prevent the execution of
> instructions over the upper boundary of the PMP region, though I'm happy to give
> this more attention if this is a concern.

This seems like a similar issue to this patch as well:
https://lore.kernel.org/qemu-devel/20191007052813.25814-1-dayeol@berkeley.edu/

From that discussion:

"In general, size 0 means "unknown size".  In this case, the one tlb lookup is
going to be used by lots of instructions -- everything that fits on the page."

Richard's last comment seems like a better fix:

"You certainly could do

    if (size == 0) {
        size = -(addr | TARGET_PAGE_MASK);
    }

to assume that all bytes from addr to the end of the page are accessed.  That
would avoid changing too much of the rest of the logic.

That said, this code will continue to not work for mis-aligned boundaries."

So I don't think this is the correct solution. I'm not sure if Dayeol
is planning on sending a follow up version. If not feel free to send
it.

>
> Signed-off-by: Chris Williams <diodesign@tuta.io <mailto:diodesign@tuta.io>>

It looks like this is a HTML patch, also ensure all patches are just
plain text, `git send-email` will do this.

Thanks for the patch though! Please send any more fixes that you find :)

Alistair

>
> diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
> index d4f1007109..9308672e20 100644
> --- a/target/riscv/pmp.c
> +++ b/target/riscv/pmp.c
> @@ -235,8 +235,9 @@ bool pmp_hart_has_privs(CPURISCVState *env, target_ulong
> addr,
>      /* 1.10 draft priv spec states there is an implicit order
>           from low to high */
>      for (i = 0; i < MAX_RISCV_PMPS; i++) {
> +        /* catch zero-size instruction checks */
>          s = pmp_is_in_range(env, i, addr);
> -        e = pmp_is_in_range(env, i, addr + size - 1);
> +        e = pmp_is_in_range(env, i, (size == 0) ? addr : addr + size - 1);
>
>          /* partially inside */
>          if ((s + e) == 1) {
>
>

Re: [PATCH v2 1/1] target/riscv/pmp: Fix bug preventing

[Dayeol Lee]

[-- Attachment #1: Type: text/plain, Size: 4203 bytes --]

Hi, Alistair,

Thank you for reminding me.
I already had the local patch, so I re-submitted the patch.
Please let me know if that's fair enough (or you have any other comments)

Thanks,

Dayeol

On Fri, Oct 11, 2019 at 3:24 PM Alistair Francis <alistair23@gmail.com>
wrote:

> On Sun, Oct 6, 2019 at 1:32 AM Chris Williams <diodesign@tuta.io> wrote:
> >
> > Hello. I hope you don't mind me resubmitting this patch. Please let me
> know if
>
> Not at all!
>
> Thanks for the patch.
>
> In future if people don't respond you can just keep pinging your patch.
>
> > I've formatted it incorrectly or if it needs more explanation. My
> previous
> > attempt probably wasn't formatted quite right. This is my first time
> > contributing to Qemu, so I'm keen to get it right - thanks.
>
> This whole paragraph should not be in the commit. It can go below the
> line though.
>
> Also please use `git format-patch` to format the patch and then `git
> send-email` to send the patch. There is a whole heap of detail here:
> https://wiki.qemu.org/Contribute/SubmitAPatch
>
> >
> > This fixes an issue that prevents a RISC-V CPU from executing
> instructions
> > immediately from the base address of a PMP TOR region.
> >
> > When jumping to an instruction in a PMP TOR region, pmp_hart_has_privs()
> is
> > called to validate the access. If this instruction is the very first
> word of a
> > PMP TOR region, at address 0 relative to the start address of the
> region, then
> > the access will fail. This is because pmp_hart_has_privs() is called
> with size
> > 0 to perform this validation, causing this check...
> >
> > e = pmp_is_in_range(env, i, addr + size - 1);
> >
> > ... to fail, as (addr + size - 1) falls below the base address of the PMP
> > region. Really, the access should succeed. For example, if I have a
> region
> > spanning 0x80d96000 to 0x88d95fff and the CPU jumps to 0x80d96000, then:
> >
> > s = 0x80d96000
> > e = 0x80d95fff
> >
> > And the validation fails. The size check proposed below catches these
> zero-size
> > instruction fetch access probes. The word alignment in pmpaddr{0-15} and
> > earlier instruction alignment checks should prevent the execution of
> > instructions over the upper boundary of the PMP region, though I'm happy
> to give
> > this more attention if this is a concern.
>
> This seems like a similar issue to this patch as well:
>
> https://lore.kernel.org/qemu-devel/20191007052813.25814-1-dayeol@berkeley.edu/
>
> From that discussion:
>
> "In general, size 0 means "unknown size".  In this case, the one tlb
> lookup is
> going to be used by lots of instructions -- everything that fits on the
> page."
>
> Richard's last comment seems like a better fix:
>
> "You certainly could do
>
>     if (size == 0) {
>         size = -(addr | TARGET_PAGE_MASK);
>     }
>
> to assume that all bytes from addr to the end of the page are accessed.
> That
> would avoid changing too much of the rest of the logic.
>
> That said, this code will continue to not work for mis-aligned boundaries."
>
> So I don't think this is the correct solution. I'm not sure if Dayeol
> is planning on sending a follow up version. If not feel free to send
> it.
>
> >
> > Signed-off-by: Chris Williams <diodesign@tuta.io <mailto:
> diodesign@tuta.io>>
>
> It looks like this is a HTML patch, also ensure all patches are just
> plain text, `git send-email` will do this.
>
> Thanks for the patch though! Please send any more fixes that you find :)
>
> Alistair
>
> >
> > diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
> > index d4f1007109..9308672e20 100644
> > --- a/target/riscv/pmp.c
> > +++ b/target/riscv/pmp.c
> > @@ -235,8 +235,9 @@ bool pmp_hart_has_privs(CPURISCVState *env,
> target_ulong
> > addr,
> >      /* 1.10 draft priv spec states there is an implicit order
> >           from low to high */
> >      for (i = 0; i < MAX_RISCV_PMPS; i++) {
> > +        /* catch zero-size instruction checks */
> >          s = pmp_is_in_range(env, i, addr);
> > -        e = pmp_is_in_range(env, i, addr + size - 1);
> > +        e = pmp_is_in_range(env, i, (size == 0) ? addr : addr + size -
> 1);
> >
> >          /* partially inside */
> >          if ((s + e) == 1) {
> >
> >
>

[-- Attachment #2: Type: text/html, Size: 5540 bytes --]

Re: [PATCH v2 1/1] target/riscv/pmp: Fix bug preventing

[Chris Williams]

Hi,
Oct 11, 2019, 15:18 by alistair23@gmail.com:

> On Sun, Oct 6, 2019 at 1:32 AM Chris Williams <diodesign@tuta.io> wrote:
>
> Also please use `git format-patch` to format the patch and then `git
> send-email` to send the patch. There is a whole heap of detail here:
> https://wiki.qemu.org/Contribute/SubmitAPatch <https://wiki.qemu.org/Contribute/SubmitAPatch>
>
OK, I will do in future. I read the page but failed to get it right. Thanks for spotting my patch, and the advice, though.

>> This fixes an issue that prevents a RISC-V CPU from executing instructions
>> immediately from the base address of a PMP TOR region.
>>
>> When jumping to an instruction in a PMP TOR region, pmp_hart_has_privs() is
>> called to validate the access. If this instruction is the very first word of a
>> PMP TOR region, at address 0 relative to the start address of the region, then
>> the access will fail. This is because pmp_hart_has_privs() is called with size
>> 0 to perform this validation, causing this check...
>>
>> e = pmp_is_in_range(env, i, addr + size - 1);
>>
>> ... to fail, as (addr + size - 1) falls below the base address of the PMP
>> region. Really, the access should succeed. For example, if I have a region
>> spanning 0x80d96000 to 0x88d95fff and the CPU jumps to 0x80d96000, then:
>>
>> s = 0x80d96000
>> e = 0x80d95fff
>>
>> And the validation fails. The size check proposed below catches these zero-size
>> instruction fetch access probes. The word alignment in pmpaddr{0-15} and
>> earlier instruction alignment checks should prevent the execution of
>> instructions over the upper boundary of the PMP region, though I'm happy to give
>> this more attention if this is a concern.
>>
>
> This seems like a similar issue to this patch as well:
> https://lore.kernel.org/qemu-devel/20191007052813.25814-1-dayeol@berkeley.edu/ <https://lore.kernel.org/qemu-devel/20191007052813.25814-1-dayeol@berkeley.edu/>
>
Yes, it appears Dayeol and I have encountered the same issue.

> From that discussion:
>
> "In general, size 0 means "unknown size".  In this case, the one tlb lookup is
> going to be used by lots of instructions -- everything that fits on the page."
>
> Richard's last comment seems like a better fix:
>
> "You certainly could do
>
>  if (size == 0) {
>  size = -(addr | TARGET_PAGE_MASK);
>  }
>
> to assume that all bytes from addr to the end of the page are accessed.  That
> would avoid changing too much of the rest of the logic.
>
> That said, this code will continue to not work for mis-aligned boundaries."
>
> So I don't think this is the correct solution. I'm not sure if Dayeol
> is planning on sending a follow up version. If not feel free to send
> it.
>
I'm happy for Dayeol to submit a better patch, if necessary. 
>> Signed-off-by: Chris Williams <diodesign@tuta.io <mailto:diodesign@tuta.io>>
>>
>
> It looks like this is a HTML patch, also ensure all patches are just
> plain text, `git send-email` will do this.
>
Yes, you're right: my webmail client isn't particularly neighborly with respect to Qemu's submission process.

C.