Re: [Qemu-devel] qmp: dump-guest-memory: -p option has issues, fix it or drop it?

[Markus Armbruster]

Jan Kiszka <address@hidden> writes:

> On 2012-09-18 03:52, Wen Congyang wrote:
>> At 09/18/2012 01:56 AM, Luiz Capitulino Wrote:
>>> Hi Wen,
>>>
>>> We've re-reviewed the dump-guest-memory command and found some
>>> possible issues with the -p option.
>>>
>>> The main issue is that it seems possible for a malicious guest to set
>>> page tables in a way that we allocate a MemoryMapping structure for
>>> each possible PTE. If IA-32e paging is used, this could lead to the
>>> allocation of dozens of gigabytes by qemu.
>>>
>>> Of course that this is not expected for the regular case, where a
>>> MemoryMapping allocation can be skipped for several reasons  (I/O memory,
>>> page not present, contiguous/in same range addresses etc), but the
>>> point is what a malicious guest can do.
>>>
>>> Another problem is that the -p option seems to be broken for SMP guests.
>>> The problem is in qemu_get_guest_memory_mapping():
>>>
>>>     first_paging_enabled_cpu = find_paging_enabled_cpu(first_cpu);
>>>     if (first_paging_enabled_cpu) {
>>>         for (env = first_paging_enabled_cpu; env != NULL; env = 
>>> env->next_cpu) {
>>>             ret = cpu_get_memory_mapping(list, env);
>>>             if (ret < 0) {
>>>                 return -1;
>>>             }
>>>         }
>>>         return 0;
>>>     }
>>>
>>> This looks for the first vCPU with paging enabled, and then assumes
>>> that all the following vCPUs also have paging enabled. How does this
>>> hold?
>
> cpu_get_memory_mapping re-validates that paging is one. In fact,
> cpu_get_memory_mapping should handle both cases so that the generic code
> need not worry about paging on/off.

The loop Luiz quoted is confusing.

Actually, the whole function is confusing.  Here's how I understand it:

    if there is a CPU that has paging enabled
        there is a proper prefix of env whose members don't have paging
        enabled; ignore them all [WTF#1]
        for all members of env not in that prefix ("the suffix"):
            get memory mapping for a CPU with paging enabled [WTF#2],
            and merge it into list
    else
        get memory mapping for ram_list,
        and merge it into list

WTF#1: Why is it okay to ignore leading CPUs with paging disabled, but
only if there's at least one CPU with paging enabled?

WTF#2: What if a CPU in the suffix doesn't have paging enabled?  Oh, the
arch-specific function to get its memory map is expected to do nothing
then.

Bonus WTF#3: What if a guest enables/disables paging between
find_paging_enabled_cpu() and the loop?  What if it changes page tables
while we walk them?

WTF is this function supposed to do?

>>> Assuming that this last issue is fixable (ie. we can make the -p
>>> option work well with SMP guests), we should at least document that
>>> -p can make QEMU allocates lots of memory and end up being killed
>>> by the OS.
>>>
>>> However, I also think that we should consider if having the -p
>>> feature is really worth it. It's a complex feature and has a number
>>> of limitations*. If libvirt doesn't use this, dropping it shouldn't
>>> be a big deal (we can return an error when -p is used).
>
> libvirt should surely not be the only reference for debugging features.

No, it's just a user, albeit an important one.  We don't break known
users cavalierly.

>>>
>>>  * The issues discussed in this email plus the fact that the guest
>>>    memory may be corrupted, and the guest may be in real-mode even
>>>    when paging is enabled
>>>
>> 
>> Yes, there are some limitations with this option. Jan said that he
>> always use gdb to deal with vmcore, so he needs such information.
>
> The point is to overcome the focus on Linux-only dump processing tools.

While I don't care for supporting alternate dump processing tools
myself, I certainly don't mind supporting them, as long as the code
satisfies basic safety and reliability requirements.

This code doesn't, as far as I can tell.

If that's correct, we should either rip it out until a satisfactory
replacemnt is available, or at least document -p as unsafe and
unreliable debugging feature (master & stable).

> I'm sure the memory allocation can be avoided by writing out any found
> virt->phys mapping directly to the vmcore file. We know where physical
> RAM will be, we only need the corresponding virtual addresses - IIUC. So
> first prepare the section according to the guest's RAM size and then,
> once we identified a page while walking the tables carefully, seek to
> that file position and write to it.

Sounds like a non-trivial change from the current code.  Makes me lean
towards ripping it out.