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

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

[Luiz Capitulino]

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?

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

 * 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

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1136 bytes --]

On 09/17/2012 11: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.
> 

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

Libvirt is not currently exposing the -p mode (that is, it currently
blindly passes 'paging':false to the JSON command).  Dropping it would
have no noticeable impact to libvirt.  (Note - libvirt _did_ wire up an
internal flag to qemu_monitor_json.c, QEMU_MONITOR_DUMP_PAGING, but
nothing sets this flag, so libvirt would need a cleanup patch to remove
it as useless code, but that is not user-visible.)

-- 
Eric Blake   eblake@redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 617 bytes --]

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

[Wen Congyang]

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

Thanks
Wen Congyang

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

[Jan Kiszka]

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.

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

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

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.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux

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

[Markus Armbruster]

Jan Kiszka <jan.kiszka@siemens.com> 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.

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

[Jan Kiszka]

On 2012-09-18 14:23, Markus Armbruster wrote:
> Jan Kiszka <jan.kiszka@siemens.com> 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?

In fact, the dump should be taken in a consistent state, means it should
run synchronously /wrt at least the CPU we refer to. So we need to run
the dump over the VCPU thread or with that VCPU stopped.

> 
> WTF is this function supposed to do?

Associate virtual and physical addresses for the whole machine at a
given time. The picture is not fully consistent as we cannot express yet
that different CPUs have different views on memory. IIRC, the first view
is taken, the rests are dropped - correct me if I'm wrong, Wen.

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

That is not what is being discussed here. It's about dropping a feature
because that one user doesn't expose it.

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

It works, thought not under all circumstances.

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

We are in early 1.3 cycle. There is surely no need to rip something
useful out based on these arguments.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux

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

[Luiz Capitulino]

On Tue, 18 Sep 2012 14:41:53 +0200
Jan Kiszka <jan.kiszka@siemens.com> wrote:

> On 2012-09-18 14:23, Markus Armbruster wrote:
> > Jan Kiszka <jan.kiszka@siemens.com> 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?
> 
> In fact, the dump should be taken in a consistent state, means it should
> run synchronously /wrt at least the CPU we refer to. So we need to run
> the dump over the VCPU thread or with that VCPU stopped.

This command stops all vCPUs, so yes, you're right here.

Any idea about WTF#1?

> > WTF is this function supposed to do?
> 
> Associate virtual and physical addresses for the whole machine at a
> given time. The picture is not fully consistent as we cannot express yet
> that different CPUs have different views on memory. IIRC, the first view
> is taken, the rests are dropped - correct me if I'm wrong, Wen.
> 
> > 
> >>>> 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.
> 
> That is not what is being discussed here. It's about dropping a feature
> because that one user doesn't expose it.

No, let me clarify.

First, what's being discussed is whether or not to drop an unsafe
feature. Having an important user relying on the feature would be
a strong indication that feature should not be dropped.

As it turns out, libvirt is the only known open-source project that
is making use of this feature (although they don't use the -p
option). We'd give the same importance to any other project that
makes themselves heard.

Of course, the rule is never to drop anything. There are exceptions
though, and this is one of them as it puts the host in danger.

> >>>>
> >>>>  * 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.
> 
> It works, thought not under all circumstances.

That would be fine if the worst case would a mangled dump file.

> > 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.
> 
> We are in early 1.3 cycle. There is surely no need to rip something
> useful out based on these arguments.

I'd agree if we fix it and the fix is backportable to -stable. Although
I have mentioned documenting it as a possible solution, I'm now unsure
if this is feasible, as we just can't allow qemu to screw up the host.

Btw, I've thought about another solution, although it can turn out to
be as bad as dropping the feature.

Jan, do you intend to use this feature through the mngt stack or do
you use it for debugging purposes only?

If it's the latter case, then maybe we could rip -p from
dump-guest-memory and:

 1. Add a new qmp command namespace for unstable/debugging-only
    commands. This should be done through a vendor extension, so
    maybe something like qemu.org-

 2. Add qemu.org-dump-guest-memory-gdb, which does what -p does
    today

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

[Jan Kiszka]

On 2012-09-18 15:33, Luiz Capitulino wrote:
> On Tue, 18 Sep 2012 14:41:53 +0200
> Jan Kiszka <jan.kiszka@siemens.com> wrote:
> 
>> On 2012-09-18 14:23, Markus Armbruster wrote:
>>> Jan Kiszka <jan.kiszka@siemens.com> 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?

There is no perfect answer to this as long as we cannot encode sections
on a per-CPU basis and as long as gdb assumes all CPUs (currently
"threads") have the same view on memory. However, that is practically
true for Linux kernels (minus CPUs that went mad), and therefore things
just work right now.

For now we need some heuristic here, and that could be very simple: if
page translation is requested, skip CPUs that are not in paging mode and
fail the dump if none is.

>>>
>>> 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?
>>
>> In fact, the dump should be taken in a consistent state, means it should
>> run synchronously /wrt at least the CPU we refer to. So we need to run
>> the dump over the VCPU thread or with that VCPU stopped.
> 
> This command stops all vCPUs, so yes, you're right here.
> 
> Any idea about WTF#1?
> 
>>> WTF is this function supposed to do?
>>
>> Associate virtual and physical addresses for the whole machine at a
>> given time. The picture is not fully consistent as we cannot express yet
>> that different CPUs have different views on memory. IIRC, the first view
>> is taken, the rests are dropped - correct me if I'm wrong, Wen.
>>
>>>
>>>>>> 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.
>>
>> That is not what is being discussed here. It's about dropping a feature
>> because that one user doesn't expose it.
> 
> No, let me clarify.
> 
> First, what's being discussed is whether or not to drop an unsafe
> feature. Having an important user relying on the feature would be
> a strong indication that feature should not be dropped.

The feature is not inherently unsafe, just unfortunate and - if used in
an unconfined environment - unsafe in its current implementation.

> 
> As it turns out, libvirt is the only known open-source project that
> is making use of this feature (although they don't use the -p
> option). We'd give the same importance to any other project that
> makes themselves heard.
> 
> Of course, the rule is never to drop anything. There are exceptions
> though, and this is one of them as it puts the host in danger.
> 
>>>>>>
>>>>>>  * 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.
>>
>> It works, thought not under all circumstances.
> 
> That would be fine if the worst case would a mangled dump file.

And I agree that we need to make it robust.

> 
>>> 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.
>>
>> We are in early 1.3 cycle. There is surely no need to rip something
>> useful out based on these arguments.
> 
> I'd agree if we fix it and the fix is backportable to -stable. Although
> I have mentioned documenting it as a possible solution, I'm now unsure
> if this is feasible, as we just can't allow qemu to screw up the host.
> 
> Btw, I've thought about another solution, although it can turn out to
> be as bad as dropping the feature.
> 
> Jan, do you intend to use this feature through the mngt stack or do
> you use it for debugging purposes only?

This feature has limited value for VMs that are live operated. Then you
are much better off with attaching gdb directly (locally or via
forwarded ports). So we do need some kind of management stack support to
automate the dump process, can be libvirt, though it need not be. Plans
aren't more concrete here.

> 
> If it's the latter case, then maybe we could rip -p from
> dump-guest-memory and:
> 
>  1. Add a new qmp command namespace for unstable/debugging-only
>     commands. This should be done through a vendor extension, so
>     maybe something like qemu.org-
> 
>  2. Add qemu.org-dump-guest-memory-gdb, which does what -p does
>     today

Let's decide about this after actually having tried to drop all the
problematic memory allocations.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux

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

[Markus Armbruster]

Jan Kiszka <jan.kiszka@siemens.com> writes:

> On 2012-09-18 14:23, Markus Armbruster wrote:
>> Jan Kiszka <jan.kiszka@siemens.com> 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?
>
> In fact, the dump should be taken in a consistent state, means it should
> run synchronously /wrt at least the CPU we refer to. So we need to run
> the dump over the VCPU thread or with that VCPU stopped.

Makes sense.  Unfortunately, it's not what the code does.

>> WTF is this function supposed to do?
>
> Associate virtual and physical addresses for the whole machine at a
> given time. The picture is not fully consistent as we cannot express yet
> that different CPUs have different views on memory. IIRC, the first view
> is taken, the rests are dropped - correct me if I'm wrong, Wen.

As far as I can tell, the code merges the views of all CPUs that have
paging enabled, which makes no sense to me.

>>>>> 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.
>
> That is not what is being discussed here. It's about dropping a feature
> because that one user doesn't expose it.

Let's not get into that fruitless discussion again.  I'm sure Luiz
didn't mean to suggest that other users (including you) don't matter.

>>>>>  * 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.
>
> It works, thought not under all circumstances.

I don't doubt it works often enough to be useful to somebody.  But basic
safety and reliability requirements are a bit more than that.  They
include "don't explode in ways a reasonable user can't be expected to
foresee".  I don't think a reasonable user can be expected to see that
-p is safe only for trusted guests.

>> 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.
>
> We are in early 1.3 cycle. There is surely no need to rip something
> useful out based on these arguments.

It's an easy way to make sure we it gets fixed.  If you want to fix it
without first ripping it out, go right ahead.

1.2 is a separate issue.  Do you agree fixing it there isn't practical?
If yes, do you want to keep it anyway?  If yes, we need a suitable
documentation patch.  Both master and stable.

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

[Anthony Liguori]

Markus Armbruster <armbru@redhat.com> writes:

> Jan Kiszka <jan.kiszka@siemens.com> writes:
>
>>>>>>  * 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.
>>
>> It works, thought not under all circumstances.
>
> I don't doubt it works often enough to be useful to somebody.  But basic
> safety and reliability requirements are a bit more than that.  They
> include "don't explode in ways a reasonable user can't be expected to
> foresee".  I don't think a reasonable user can be expected to see that
> -p is safe only for trusted guests.

We shipped the API, we're not removing it.  Our compatibility isn't
"whatever libvirt is currently using".

It's perfectly reasonable to ask to document the behavior of the
method.  It's also a trivial patch to qapi-schema.json.

It's unreasonable to ask for an interface to be removed just because it
could be misused when it has a legimitate use-case.

Regards,

Anthony Liguori

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

[Luiz Capitulino]

On Tue, 18 Sep 2012 16:13:30 -0500
Anthony Liguori <anthony@codemonkey.ws> wrote:

> Markus Armbruster <armbru@redhat.com> writes:
> 
> > Jan Kiszka <jan.kiszka@siemens.com> writes:
> >
> >>>>>>  * 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.
> >>
> >> It works, thought not under all circumstances.
> >
> > I don't doubt it works often enough to be useful to somebody.  But basic
> > safety and reliability requirements are a bit more than that.  They
> > include "don't explode in ways a reasonable user can't be expected to
> > foresee".  I don't think a reasonable user can be expected to see that
> > -p is safe only for trusted guests.
> 
> We shipped the API, we're not removing it.  Our compatibility isn't
> "whatever libvirt is currently using".
> 
> It's perfectly reasonable to ask to document the behavior of the
> method.  It's also a trivial patch to qapi-schema.json.

I feel that documenting it is not enough. It would be fine to do that
if the worst case was a bad dump file, but the worst case as the
code stands right now will affect the host.

> It's unreasonable to ask for an interface to be removed just because it
> could be misused when it has a legimitate use-case.

The point is not that it can be misused. The issue we're concerned about
is that a malicious guest could cause qemu to allocate dozens of
gigabytes of RAM.

Jan suggested a fix that could make it less worse, which is to avoid
allocating any memory while walking the guest page tables. However,
it's not clear if this is hard to do and, more importantly, if it's
backportable to -stable.

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

[Anthony Liguori]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Tue, 18 Sep 2012 16:13:30 -0500
> Anthony Liguori <anthony@codemonkey.ws> wrote:
>
>> Markus Armbruster <armbru@redhat.com> writes:
>> 
>> > Jan Kiszka <jan.kiszka@siemens.com> writes:
>> >
>> >>>>>>  * 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.
>> >>
>> >> It works, thought not under all circumstances.
>> >
>> > I don't doubt it works often enough to be useful to somebody.  But basic
>> > safety and reliability requirements are a bit more than that.  They
>> > include "don't explode in ways a reasonable user can't be expected to
>> > foresee".  I don't think a reasonable user can be expected to see that
>> > -p is safe only for trusted guests.
>> 
>> We shipped the API, we're not removing it.  Our compatibility isn't
>> "whatever libvirt is currently using".
>> 
>> It's perfectly reasonable to ask to document the behavior of the
>> method.  It's also a trivial patch to qapi-schema.json.
>
> I feel that documenting it is not enough. It would be fine to do that
> if the worst case was a bad dump file, but the worst case as the
> code stands right now will affect the host.

Normally I would agree with you if it was a guest initiated action, but
it's a user initiated command.

Putting a big fat warning in qapi-schema.json is sufficient in my mind.

>> It's unreasonable to ask for an interface to be removed just because it
>> could be misused when it has a legimitate use-case.
>
> The point is not that it can be misused. The issue we're concerned about
> is that a malicious guest could cause qemu to allocate dozens of
> gigabytes of RAM.

By a user initiated action.  A user can assign a USB/PCI device that's being
used by the kernel.  They could hotplug a disk that points to /dev/hda
and overwrite their active root filesystem too.

We should educate users and avoid making it too easy to shoot themselves
in the foot.  But this is an option to an obscure QMP command.  I highly
doubt that there will ever be a case where a root kit in the wild is
prepared to completely hose the guest in the hopes that a user is going
to blindly issue this command with the '-p' option.

> Jan suggested a fix that could make it less worse, which is to avoid
> allocating any memory while walking the guest page tables. However,
> it's not clear if this is hard to do and, more importantly, if it's
> backportable to -stable.

I just don't see the problem here as long as the behavior is
documented.

Regards,

Anthony Liguori

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

[Wen Congyang]

At 09/19/2012 08:18 AM, Luiz Capitulino Wrote:
> On Tue, 18 Sep 2012 16:13:30 -0500
> Anthony Liguori <anthony@codemonkey.ws> wrote:
> 
>> Markus Armbruster <armbru@redhat.com> writes:
>>
>>> Jan Kiszka <jan.kiszka@siemens.com> writes:
>>>
>>>>>>>>  * 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.
>>>>
>>>> It works, thought not under all circumstances.
>>>
>>> I don't doubt it works often enough to be useful to somebody.  But basic
>>> safety and reliability requirements are a bit more than that.  They
>>> include "don't explode in ways a reasonable user can't be expected to
>>> foresee".  I don't think a reasonable user can be expected to see that
>>> -p is safe only for trusted guests.
>>
>> We shipped the API, we're not removing it.  Our compatibility isn't
>> "whatever libvirt is currently using".
>>
>> It's perfectly reasonable to ask to document the behavior of the
>> method.  It's also a trivial patch to qapi-schema.json.
> 
> I feel that documenting it is not enough. It would be fine to do that
> if the worst case was a bad dump file, but the worst case as the
> code stands right now will affect the host.
> 
>> It's unreasonable to ask for an interface to be removed just because it
>> could be misused when it has a legimitate use-case.
> 
> The point is not that it can be misused. The issue we're concerned about
> is that a malicious guest could cause qemu to allocate dozens of
> gigabytes of RAM.

Hmm, I guess the malicious guest's page table provides wrong information.
We allocate memory to store memory mapping. Each memory mapping needs
less than 40 bytes memory. The num of memory mapping is less than
(2^48) / (2^12) = 2^36. And 2^36 * 40 = 64G * 40, too many memory....

What about this:
1. if the num of memory mapping > 100000, we only store 100000 memory
   mappings.

2. The memory mapping which has smaller virtual address will be dropped?

In this case, the memory we need is less than 10MB. So we will not allocate
too many memory.

Thanks
Wen Congyang

> 
> Jan suggested a fix that could make it less worse, which is to avoid
> allocating any memory while walking the guest page tables. However,
> it's not clear if this is hard to do and, more importantly, if it's
> backportable to -stable.
> 
> 

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

[HATAYAMA Daisuke]

From: Wen Congyang <wency@cn.fujitsu.com>
Subject: Re: [Qemu-devel] qmp: dump-guest-memory: -p option has issues, fix it or drop it?
Date: Wed, 19 Sep 2012 10:07:04 +0800

> At 09/19/2012 08:18 AM, Luiz Capitulino Wrote:
>> On Tue, 18 Sep 2012 16:13:30 -0500
>> Anthony Liguori <anthony@codemonkey.ws> wrote:
>> 
>>> Markus Armbruster <armbru@redhat.com> writes:
>>>
>>>> Jan Kiszka <jan.kiszka@siemens.com> writes:
>>>>
>>>>>>>>>  * 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.
>>>>>
>>>>> It works, thought not under all circumstances.
>>>>
>>>> I don't doubt it works often enough to be useful to somebody.  But basic
>>>> safety and reliability requirements are a bit more than that.  They
>>>> include "don't explode in ways a reasonable user can't be expected to
>>>> foresee".  I don't think a reasonable user can be expected to see that
>>>> -p is safe only for trusted guests.
>>>
>>> We shipped the API, we're not removing it.  Our compatibility isn't
>>> "whatever libvirt is currently using".
>>>
>>> It's perfectly reasonable to ask to document the behavior of the
>>> method.  It's also a trivial patch to qapi-schema.json.
>> 
>> I feel that documenting it is not enough. It would be fine to do that
>> if the worst case was a bad dump file, but the worst case as the
>> code stands right now will affect the host.
>> 
>>> It's unreasonable to ask for an interface to be removed just because it
>>> could be misused when it has a legimitate use-case.
>> 
>> The point is not that it can be misused. The issue we're concerned about
>> is that a malicious guest could cause qemu to allocate dozens of
>> gigabytes of RAM.
> 
> Hmm, I guess the malicious guest's page table provides wrong information.
> We allocate memory to store memory mapping. Each memory mapping needs
> less than 40 bytes memory. The num of memory mapping is less than
> (2^48) / (2^12) = 2^36. And 2^36 * 40 = 64G * 40, too many memory....
> 
> What about this:
> 1. if the num of memory mapping > 100000, we only store 100000 memory
>    mappings.
> 
> 2. The memory mapping which has smaller virtual address will be dropped?
> 
> In this case, the memory we need is less than 10MB. So we will not allocate
> too many memory.
> 

How about dropping making a whole list of memory maps at the same
time, and how about rewriting the code so that it always has at most
one memory mapping by merging virtually consequtive chunks? If
possible, only 40 bytes is needed.

Thanks.
HATAYAMA, Daisuke

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

[Luiz Capitulino]

On Wed, 19 Sep 2012 11:26:51 +0900 (JST)
HATAYAMA Daisuke <d.hatayama@jp.fujitsu.com> wrote:

> From: Wen Congyang <wency@cn.fujitsu.com>
> Subject: Re: [Qemu-devel] qmp: dump-guest-memory: -p option has issues, fix it or drop it?
> Date: Wed, 19 Sep 2012 10:07:04 +0800
> 
> > At 09/19/2012 08:18 AM, Luiz Capitulino Wrote:
> >> On Tue, 18 Sep 2012 16:13:30 -0500
> >> Anthony Liguori <anthony@codemonkey.ws> wrote:
> >> 
> >>> Markus Armbruster <armbru@redhat.com> writes:
> >>>
> >>>> Jan Kiszka <jan.kiszka@siemens.com> writes:
> >>>>
> >>>>>>>>>  * 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.
> >>>>>
> >>>>> It works, thought not under all circumstances.
> >>>>
> >>>> I don't doubt it works often enough to be useful to somebody.  But basic
> >>>> safety and reliability requirements are a bit more than that.  They
> >>>> include "don't explode in ways a reasonable user can't be expected to
> >>>> foresee".  I don't think a reasonable user can be expected to see that
> >>>> -p is safe only for trusted guests.
> >>>
> >>> We shipped the API, we're not removing it.  Our compatibility isn't
> >>> "whatever libvirt is currently using".
> >>>
> >>> It's perfectly reasonable to ask to document the behavior of the
> >>> method.  It's also a trivial patch to qapi-schema.json.
> >> 
> >> I feel that documenting it is not enough. It would be fine to do that
> >> if the worst case was a bad dump file, but the worst case as the
> >> code stands right now will affect the host.
> >> 
> >>> It's unreasonable to ask for an interface to be removed just because it
> >>> could be misused when it has a legimitate use-case.
> >> 
> >> The point is not that it can be misused. The issue we're concerned about
> >> is that a malicious guest could cause qemu to allocate dozens of
> >> gigabytes of RAM.
> > 
> > Hmm, I guess the malicious guest's page table provides wrong information.
> > We allocate memory to store memory mapping. Each memory mapping needs
> > less than 40 bytes memory. The num of memory mapping is less than
> > (2^48) / (2^12) = 2^36. And 2^36 * 40 = 64G * 40, too many memory....
> > 
> > What about this:
> > 1. if the num of memory mapping > 100000, we only store 100000 memory
> >    mappings.
> > 
> > 2. The memory mapping which has smaller virtual address will be dropped?
> > 
> > In this case, the memory we need is less than 10MB. So we will not allocate
> > too many memory.

The problem of artificially limiting it is that we can reach the limit
in "legal" usage (ie. a very large machine).

> How about dropping making a whole list of memory maps at the same
> time, and how about rewriting the code so that it always has at most
> one memory mapping by merging virtually consequtive chunks? If
> possible, only 40 bytes is needed.

It already merges contiguous addresses and addresses that fall in
the same range. It can also skip addresses due to a few reasons (I/O
page, not present page, etc), which makes the problem very unlikely
in practice.

Our concern is with guests intentionally trying to make qemu allocate
more memory.

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

[HATAYAMA Daisuke]

From: Luiz Capitulino <lcapitulino@redhat.com>
Subject: Re: [Qemu-devel] qmp: dump-guest-memory: -p option has issues, fix it or drop it?
Date: Wed, 19 Sep 2012 10:23:26 -0300

> On Wed, 19 Sep 2012 11:26:51 +0900 (JST)
> HATAYAMA Daisuke <d.hatayama@jp.fujitsu.com> wrote:
> 
>> From: Wen Congyang <wency@cn.fujitsu.com>
>> Subject: Re: [Qemu-devel] qmp: dump-guest-memory: -p option has issues, fix it or drop it?
>> Date: Wed, 19 Sep 2012 10:07:04 +0800
>> 
>> > At 09/19/2012 08:18 AM, Luiz Capitulino Wrote:
>> >> On Tue, 18 Sep 2012 16:13:30 -0500
>> >> Anthony Liguori <anthony@codemonkey.ws> wrote:
>> >> 
>> >>> Markus Armbruster <armbru@redhat.com> writes:
>> >>>
>> >>>> Jan Kiszka <jan.kiszka@siemens.com> writes:
>> >>>>

>> How about dropping making a whole list of memory maps at the same
>> time, and how about rewriting the code so that it always has at most
>> one memory mapping by merging virtually consequtive chunks? If
>> possible, only 40 bytes is needed.
> 
> It already merges contiguous addresses and addresses that fall in
> the same range. It can also skip addresses due to a few reasons (I/O
> page, not present page, etc), which makes the problem very unlikely
> in practice.
> 

I've noticed this locally, thanks.

> Our concern is with guests intentionally trying to make qemu allocate
> more memory.

My idea is not to create a list of a whole memory mapping. This needs
O(n), which can be a target of attackers. If possible to drop this
list, memory consumption is O(1), and there's no room for any guest to
attack.

Concretely, the processing becomes: first walking page tables to
identify a memory mapping corresponding to a PT_LOAD entry, then write
it when reaching the part belonging to a next PT_LOAD entry.

But the problem I've noticed just now..., is that then we need to walk
page tables twice: first for writing program header tables and second
for writing memory part. Code becomes not clean.

Thanks.
HATAYAMA, Daisuke

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

[Markus Armbruster]

Anthony Liguori <anthony@codemonkey.ws> writes:

> Markus Armbruster <armbru@redhat.com> writes:
>
>> Jan Kiszka <jan.kiszka@siemens.com> writes:
>>
>>>>>>>  * 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.
>>>
>>> It works, thought not under all circumstances.
>>
>> I don't doubt it works often enough to be useful to somebody.  But basic
>> safety and reliability requirements are a bit more than that.  They
>> include "don't explode in ways a reasonable user can't be expected to
>> foresee".  I don't think a reasonable user can be expected to see that
>> -p is safe only for trusted guests.
>
> We shipped the API, we're not removing it.  Our compatibility isn't
> "whatever libvirt is currently using".

Bah, don't be more catholic than the pope.  It's been out for how many
days?  Have you looked at the code?

> It's perfectly reasonable to ask to document the behavior of the
> method.  It's also a trivial patch to qapi-schema.json.

I'm okay with leaving obscure security holes in upstream QEMU,
indefinitely, as long as they're documented.  I don't think it's a good
idea, but it's something reasonable people can disagree about.

We need to document that -p is unreliable and unsafe, therefore should
only be used with trusted guests, and its result need not be correct
even then.

> It's unreasonable to ask for an interface to be removed just because it
> could be misused when it has a legimitate use-case.

The issue isn't "misuse" (operator does something stupid), it's abuse
(guest can make it blow up when operator uses it as intended), and
fragility (produces crap when operator uses it as intended, but the
guest isn't in a sufficiently "nice" state).

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

[Anthony Liguori]

Markus Armbruster <armbru@redhat.com> writes:

> Anthony Liguori <anthony@codemonkey.ws> writes:
>
>> Markus Armbruster <armbru@redhat.com> writes:
>>
>>> Jan Kiszka <jan.kiszka@siemens.com> writes:
>>>
>>>>>>>>  * 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.
>>>>
>>>> It works, thought not under all circumstances.
>>>
>>> I don't doubt it works often enough to be useful to somebody.  But basic
>>> safety and reliability requirements are a bit more than that.  They
>>> include "don't explode in ways a reasonable user can't be expected to
>>> foresee".  I don't think a reasonable user can be expected to see that
>>> -p is safe only for trusted guests.
>>
>> We shipped the API, we're not removing it.  Our compatibility isn't
>> "whatever libvirt is currently using".
>
> Bah, don't be more catholic than the pope.  It's been out for how many
> days?  Have you looked at the code?
>
>> It's perfectly reasonable to ask to document the behavior of the
>> method.  It's also a trivial patch to qapi-schema.json.
>
> I'm okay with leaving obscure security holes in upstream QEMU,
> indefinitely, as long as they're documented.  I don't think it's a good
> idea, but it's something reasonable people can disagree about.

It's *not* a security hole.

The "malicious guest" you mention is just a guest with fully populated
PTEs and PDEs, no?

There's nothing malicious about that.

It just so happens that the memory usage of this command is proportional
to the amount of page mappings in the guest.

I think your making a mountain out of a mole hill here.

Regards,

Anthony Liguori

>
> We need to document that -p is unreliable and unsafe, therefore should
> only be used with trusted guests, and its result need not be correct
> even then.
>
>> It's unreasonable to ask for an interface to be removed just because it
>> could be misused when it has a legimitate use-case.
>
> The issue isn't "misuse" (operator does something stupid), it's abuse
> (guest can make it blow up when operator uses it as intended), and
> fragility (produces crap when operator uses it as intended, but the
> guest isn't in a sufficiently "nice" state).

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

[Markus Armbruster]

Anthony Liguori <anthony@codemonkey.ws> writes:

> Markus Armbruster <armbru@redhat.com> writes:
>
>> Anthony Liguori <anthony@codemonkey.ws> writes:
>>
>>> Markus Armbruster <armbru@redhat.com> writes:
>>>
>>>> Jan Kiszka <jan.kiszka@siemens.com> writes:
>>>>
>>>>>>>>>  * 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.
>>>>>
>>>>> It works, thought not under all circumstances.
>>>>
>>>> I don't doubt it works often enough to be useful to somebody.  But basic
>>>> safety and reliability requirements are a bit more than that.  They
>>>> include "don't explode in ways a reasonable user can't be expected to
>>>> foresee".  I don't think a reasonable user can be expected to see that
>>>> -p is safe only for trusted guests.
>>>
>>> We shipped the API, we're not removing it.  Our compatibility isn't
>>> "whatever libvirt is currently using".
>>
>> Bah, don't be more catholic than the pope.  It's been out for how many
>> days?  Have you looked at the code?
>>
>>> It's perfectly reasonable to ask to document the behavior of the
>>> method.  It's also a trivial patch to qapi-schema.json.
>>
>> I'm okay with leaving obscure security holes in upstream QEMU,
>> indefinitely, as long as they're documented.  I don't think it's a good
>> idea, but it's something reasonable people can disagree about.
>
> It's *not* a security hole.

Since we agree that documenting the existing crappy behavior is okay for
stable, let's not argue what exactly to call it.

> The "malicious guest" you mention is just a guest with fully populated
> PTEs and PDEs, no?

A "normal" guest uses a small fraction of its RAM for page tables.  QEMU
allocates 10x of that fraction for dump -p.  Not so nice.

A not-so-normal guest uses most of its RAM for page tables.  QEMU
allocates in the order of 10x of the guest RAM for -p.  Oops.

[...]