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

[Daniel Kiper]

On Thu, Mar 07, 2019 at 07:05:20PM +0100, Jesús Diéguez Fernández wrote:

[...]

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

Great!

> handling the exception.

Yeah, this can be difficult.

> 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.

I think that we should have generic #GP handler which catches general
protection faults.

> - 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)?

I can accept this patch series without #GP handler if:
  - you add warning to the GRUB doc that #GP for rdmsr/wrmsr are not
    handled and system reboots; so, potential users have to be careful
    using these modules,
  - you add TODO notice to the code saying that generic #GP handling
    should be added to the GRUB code,
  - you promise me that at some point, let's say after GRUB release,
    you will try to implement generic #GP handler.

Is this acceptable for you?

Daniel