Re: [PATCH v2 2/5] [RISCV_PM] Support CSRs required for RISC-V PM extension except for ones in hypervisor mode

[Alexey Baturo]

> Surely you don't need MMTE_MASK here because you used it when writing.

> That said, don't you also need to mask vs the current priv level?

> while surely it's a security bug for lower priv code to read anything related to a higher priv

Now I think I get what you've meant:

The spec states that for lower priv level you get WPRI fields that contain bits for higher priv levels.

Applying those masks while reading CSRs on the one hand solves this security breach, but on the other - violates the spec. Let me raise this question at the upcoming J WG meeting.

Meanwhile, do you think applying *MTE masks while reading CSR values is a good solution for now?

Thanks again!

чт, 15 окт. 2020 г. в 20:28, Alexey Baturo <baturo.alexey@gmail.com>:

Richard, again thanks for the review!

> This is a bit clumsy.  I suggest

Sure, will fix.

> If you try to read this on current hardware, without J, then you get an exception not zero.

If I get the spec right, the idea is to read 0 from PM CSRs even if this extension is not present.

>I would have expected this check would actually go into the csr predicate function.

If I understand your proposal correctly, in this case I'd have to introduce 3 new functions for S/M/U PM CSRs as a predicate. I'm okay with that if you suggest so

>Surely you don't need MMTE_MASK here because you used it when writing.

>That said, don't you also need to mask vs the current priv level?

I suppose that applying these masks helps to prepare the correct value for the given priv level. So if I understand correctly if you're in M-mode and try to read UMTE, you'll get only U.Enable and U.Current, while if you'd try to read SMTE, you'll also get XS bits, S.Enable and S.Current.

> it's a security bug for lower priv code to read anything related to a higher priv.

Could you please elaborate a bit more here? I don't see how this scenario is valid: in U-mode you're supposed to be able to read only U* registers, while M/S mode could allow U-mode to write to its U* registers.

As for S-mode, I believe it's allowed to write to any U* registers and it's available to write to S* registers if S.Current is set.

> Do not crash qemu because the guest is doing silly things

Sure, you're right. Will fix.

>Raise an exception if you like, and perhaps qemu_log_mask with LOG_GUEST_ERROR

I think raising exception here might be too much, but logging the error is a great idea, thanks!

Thanks!

чт, 15 окт. 2020 г. в 19:48, Richard Henderson <richard.henderson@linaro.org>:

On 10/15/20 8:21 AM, Alexey Baturo wrote:
> +/* Functions to access Pointer Masking feature registers
> + * We have to check if current priv lvl could modify
> + * csr in given mode
> + */
> +static int check_pm_current_disabled(CPURISCVState *env, int csrno)
> +{
> +    /* m-mode is always allowed to modify registers, so allow */
> +    if (env->priv == PRV_M) {
> +        return 0;
> +    }
> +    int csr_priv = get_field(csrno, 0xC00);
> +    /* If priv lvls differ that means we're accessing csr from higher priv lvl, so allow */
> +    if (env->priv != csr_priv) {
> +        return 0;
> +    }
> +    int cur_bit_pos = (env->priv == PRV_U) ? U_PM_CURRENT :
> +                      (env->priv == PRV_S) ? S_PM_CURRENT : -1;
> +    assert(cur_bit_pos != -1);

This is a bit clumsy.  I suggest

    int cur_bit_pos;
    switch (env->priv) {
    case PRV_M:
        return 0;
    case PRV_S:
        cur_bit_pos = S_PM_CURRENT;
        break;
    case PRV_U:
        cur_bit_pos = U_PM_CURRENT;
        break;
    default:
        g_assert_not_reached();
    }

> +static int read_mmte(CPURISCVState *env, int csrno, target_ulong *val)
> +{
> +#if !defined(CONFIG_USER_ONLY)
> +    if (!riscv_has_ext(env, RVJ)) {
> +        *val = 0;
> +        return 0;
> +    }
> +#endif

This ifdef is wrong.  CONFIG_USER_ONLY doesn't have anything to do with what
features the cpu supports.  Nor can it be correct.  If you try to read this on
current hardware, without J, then you get an exception not zero.

I would have expected this check would actually go into the csr predicate function.

> +    *val = env->mmte & MMTE_MASK;

Surely you don't need MMTE_MASK here because you used it when writing.

That said, don't you also need to mask vs the current priv level?  There's
language in your doc that says "XS bits are available in..." and "PM bits are
available in...".

I'll also note that it says "by default only higher priv code can set the value
for PM bits", while surely it's a security bug for lower priv code to read
anything related to a higher priv.


> +    target_ulong wpri_val = val & SMTE_MASK;
> +    assert(val == wpri_val);

Incorrect assert.  This value is under guest control.  Do not crash qemu
because the guest is doing silly things.  Raise an exception if you like, and
perhaps qemu_log_mask with LOG_GUEST_ERROR.

> +    if (check_pm_current_disabled(env, csrno))
> +        return 0;

Missing braces.


r~