Re: [PATCH v2 2/2] Add new MSR modules (rdmsr/wrmsr)

[Daniel Kiper]

On Tue, Mar 05, 2019 at 11:17:01PM +0100, Jesús Diéguez Fernández wrote:
> Hi,
>    I have some doubts that I comment below.
>
> El 5/3/19 a las 12:09, Daniel Kiper escribió:
> > On Fri, Mar 01, 2019 at 06:17:42PM +0100, Jesús Diéguez Fernández wrote:
> >> In order to be able to read and write from/to model-specific registers,
> >> two new modules are added. They are i386 specific, as the cpuid module.
> >>
> >> rdmsr module registers the command rdmsr that allows reading from a MSR.
> >> wrmsr module registers the command wrmsr that allows writing to a MSR.
> >>
> >> wrmsr module is disabled if UEFI secure boot is enabled.
> >>
> >> Please note that on SMP systems, interacting with a MSR that has a
> >> scope per hardware thread, implies that the value only applies to
> >> the particular cpu/core/thread that ran the command.
> >>
> >> Changelog v1 -> v2:
> >>
> >>     - Patch all source code files with s/__asm__ __volatile__/asm 
> >> volatile/g.
> >>     - Split the module in two (rdmsr/wrmsr).
> >>     - Include the wrmsr module in the forbidden modules efi list.
> >>     - Code indentation and cleanup.
> >>     - Copyright year update.
> >>     - Implicit casting mask removed.
> >>     - Use the same assembly code for x86 and x86_64.
> >>     - Add missing documentation.
> >>     - Patch submited with Signed-off-by.
> >>
> >> Signed-off-by: Jesús Diéguez Fernández <address@hidden>
> >
> > In general it looks much better but I still have some concerns...
> >
> > First of all, two patches and more should have mail introducing them.
> > Good example you can find here
> >   http://lists.gnu.org/archive/html/grub-devel/2018-10/msg00201.html
> > or here
> >   http://lists.gnu.org/archive/html/grub-devel/2018-11/msg00282.html
> >
> > git send-email --compose ... is your friend...
> >
> > [...]
>
> I thought about it before sending the e-mail, but I chose the wrong
> option. :-\

Not big deal...

> For the v3, now that the patch [1/2] has already been reviewed, should I
> send it again, or should I skip it?

Please add my Reviewed-by under your Signed-off-by (SOB) and resend it.

> >> diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
> >> index 2346bd291..a966a8f28 100644
> >> --- a/grub-core/Makefile.core.def
> >> +++ b/grub-core/Makefile.core.def
> >> @@ -2484,3 +2484,19 @@ module = {
> >>    common = loader/i386/xen_file64.c;
> >>    extra_dist = loader/i386/xen_fileXX.c;
> >>  };
> >> +module = {
> >> +  name = rdmsr;
> >> +  common = commands/i386/rdmsr.c;
> >> +  enable = x86;
> >> +  enable = i386_xen_pvh;
> >> +  enable = i386_xen;
> >> +  enable = x86_64_xen;
> >
> > I think that x86 should suffice? Does not it?
>
> I used the cpuid module as an example since it has many similarities.
> Even so, I thought that accessing MSR from a Xen guest could be interesting:
>
> http://www.brendangregg.com/blog/2014-09-15/the-msrs-of-ec2.html
>
> For my use case, leaving alone x86 is enough. I'll modify it as you tell
> me to.

I am not saying that MSR access is not interesting on Xen. I am saying
that I have a feeling that all "enable ..." except "enable = x86;" are
redundant. So, I think that "enable = x86;" will cover Xen too. Please
double check it. If it does not cover leave xen stuff as is.

> >> +};
> >> +module = {
> >> +  name = wrmsr;
> >> +  common = commands/i386/wrmsr.c;
> >> +  enable = x86;
> >> +  enable = i386_xen_pvh;
> >> +  enable = i386_xen;
> >> +  enable = x86_64_xen;
> >
> > Ditto.
> >
>
> [...]
>
> >> +static grub_err_t
> >> +grub_cmd_msr_read (grub_extcmd_context_t ctxt, int argc, char **argv)
> >> +{
> >> +    grub_uint64_t addr, value;
> >> +    char *ptr;
> >> +    char buf[sizeof("XXXXXXXX")];
> >> +
> >> +    if (argc != 1)
> >> +        return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("one argument 
> >> expected"));
> >> +
> >> +    grub_errno = GRUB_ERR_NONE;
> >> +    ptr = argv[0];
> >> +    addr = grub_strtoul (ptr, &ptr, 0);
> >> +
> >> +    if (grub_errno != GRUB_ERR_NONE)
> >> +        return grub_errno;
> >
> > Should not you display a blurb to the user what happened here?
> > Like you do below...
>
> I used the same method that is used in grub-core/term/terminfo.c
>
> static grub_err_t
> grub_cmd_terminfo (grub_extcmd_context_t ctxt, int argc, char **args)
> {
> ...
>
>       char *ptr = state[OPTION_GEOMETRY].arg;
>       w = grub_strtoul (ptr, &ptr, 0);
>       if (grub_errno)
>         return grub_errno;
>       if (*ptr != 'x')
>         return grub_error (GRUB_ERR_BAD_ARGUMENT,
>                            N_("incorrect terminal dimensions
> specification"));
>
> I tested this with:
>
> rdmsr z => shows "unrecognized number"
> rdmsr 0x1122334455667788991100 => shows "overflow detected"
>
> I can change it to:
>
> if (grub_errno != GRUB_ERR_NONE)
>         return grub_error (grub_errno, N_("invalid argument"));
>
> If that is any better.

If your/terminfo version works correctly then I am fine with it.
You do not need to change it.

> >> +    if (*ptr != '\0')
> >> +        return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("invalid argument"));
> >> +
> >> +    value = grub_msr_read (addr);
> >> +
> >> +    if (ctxt->state[0].set)
> >> +    {
> >> +        grub_snprintf (buf, sizeof(buf), "%llx", value);
> >> +        grub_env_set (ctxt->state[0].arg, buf);
> >> +    }
> >> +    else
> >> +        grub_printf ("0x%llx\n", value);
> >> +
> >> +    return GRUB_ERR_NONE;
> >> +}
>
> [...]
>
> >> +
> >> +GRUB_MOD_LICENSE("GPLv3+");
> >> +
> >> +static grub_command_t cmd_write;
> >> +
> >> +static grub_err_t
> >> +grub_cmd_msr_write (grub_command_t cmd, int argc, char **argv)
> >> +{
> >> +    grub_uint64_t addr, value;
> >> +    char *ptr;
> >> +
> >> +    if (argc != 2)
> >> +        return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("two arguments 
> >> expected"));
> >> +
> >> +    grub_errno = GRUB_ERR_NONE;
> >> +    ptr = argv[0];
> >> +    addr = grub_strtoul (ptr, &ptr, 0);
> >> +
> >> +    if (grub_errno != GRUB_ERR_NONE)
> >> +        return grub_errno;
> >
> > Ditto.
> >
> >> +    if (*ptr != '\0')
> >> +        return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("invalid argument"));
> >> +
> >> +    ptr = argv[1];
> >> +    value = grub_strtoul (ptr, &ptr, 0);
> >> +
> >> +    if (grub_errno != GRUB_ERR_NONE)
> >> +        return grub_errno;
> >
> > Ditto.
> >
> >> +    if (*ptr != '\0')
> >> +        return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("invalid argument"));
> >> +
>
> I found a bug in this function, it is possible to write 64 bit values to
> the MSR, so I should have used grub_strtoull when reading the value.
> It will be fixed in the v3.
>
> >> +    grub_msr_write (addr, value);

There are other issues. IMO addr should be 32-bit type instead of 64-bit.

And Intel spec says:
  The CPUID instruction should be used to determine whether MSRs are
  supported (CPUID.01H:EDX[5] = 1) before using this instruction.

  Specifying a reserved or unimplemented MSR address in ECX will also
  cause a general protection exception.

What will happen if somebody specifies invalid MSR? Does GRUB crashes?

Daniel