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

[Jesús Diéguez Fernández]

El 6/3/19 a las 15:30, Daniel Kiper escribió:
> On Wed, Mar 06, 2019 at 01:01:07PM +0100, Jesús DF wrote:
>> El mié., 6 mar. 2019 a las 11:32, Daniel Kiper (<address@hidden>) escribió:
>>> 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.
>>>
>>
>> Ok.
> 
> [...]
> 
>>>>>> +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
>>
>> Yes, I already did change addr to be 32-bit type in my local branch.
> 
> Great!
> 
>> All Intel CPUs since the first Pentium support that instructions, but
>> to be safe I should check it using CPUID.
>> I'll add that functionality to the cpuid module and use it to verify
>> if its availiable before calling the instruction.
> 
> I am not sure that it pays to pull in cpuid module here. Maybe it will
> be easier to just use asm with cpuid before grub_msr_read()/grub_msr_write()
> calls. Anyway, whichever is simpler.
> 
>> Currently, when you try to read or write from an invalid MSR, the
>> system is rebooted.
>> I'm not familiar with the way to handle a general protection
>> exceptions, do you know where should I look at?
> 
> Intel SDM, Linux kernel or Xen.
> 
> Daniel
> 

I've sorted out the MSR support check, but I'm struggling a bit with
handling the exception.

I've read the Intel SDM and other docs, and I think I understand the
reason why the system reboots: when the rdmsr or wrmsr access a
restricted area, a general protection exception is raised; and because
there's no interrupt 13 (GP#) handler installed, it double faults and
then triple faults, resulting in a cpu halt and motherboard reboot.

The linux kernel uses the following function to read and write to the msr:

https://elixir.bootlin.com/linux/latest/source/arch/x86/include/asm/msr.h#L137

If I understood it correctly, it uses some gcc syntax to setup the
handler of an exception in a different section, but I think that it
relies on the IDT that the linux kernel has already installed, so I
can't use that code.

https://github.com/torvalds/linux/blob/master/Documentation/x86/exception-tables.txt

Apart from setting up the IDT, I've found the opcodes CLI and STI, but
I'm quite sure that trying to use them to ignore the interrupt is a dead
end.

I've looked around in the grub code and there are only a few files that
reference CLI, STI, LIDT or IRET, mainly the code that makes a stage
change.
I also found that the file grub-core/commands/i386/pc/drivemap.c sets up
a int13h trap to handle the disk mappings.

Since I'm not very familiar with this area (and maybe I'm
overcomplicating it), it seems that the approach of handling the
exception could take a lot more effort than the whole module itself.

Now, the questions I have are about what direction I should take:

- Do I really need to make a custom handler (something like drivemap.c
does), or is there any simpler way?

- I assume that I'm not the first one to need to handle a general
protection exception in grub, but I couldn't find any example in other
modules/commands. Any reference would be appreciated.

- And from a practical point of view, in case that I need to setup a
custom handler for this, is it mandatory to ship the patch with it or
could it be added later (maybe adding a TODO now)?

Jesus