[Qemu-devel] Re: [PATCH 5/5] Port apic to new VMState design

[Reimar Döffinger]

On Tue, Aug 18, 2009 at 04:41:42PM +0200, Juan Quintela wrote:
> Reimar Döffinger <address@hidden> wrote:
> > Hello,
> > sorry for replying in the middle of the thread, I was to fast and
> > deleted the other mails already.
> > And just in case I mention I am new around here, so feel free to ignore
> > me if you feel I am completely wrong.
> > One thing I don't like too much about it is that you can't really handle
> > "calculated" fields.
> 
> Calculated fields are not by definition part of the state :)
> The state are the other fields that are used to save the state.  I
> haven't yet seen calculade fields (but I haven't looked throughfully
> yet).  With the current design, basically you can only save things that
> are in one struct (the way it is stored is an offset against a base
> address).

I am not sure we 100% understand each other, so I maybe tell the
specific example.
I made a change to the eepro100 driver to fix dumping the network
statistics.
The main problem is, depending on which device you emulate, the size of
the statistics struct changes.
Since it looked ugly etc. I decided not to calculate the size of these
statistics each time but instead save it in the device state.
But instead of adding it to save_vm and load_vm (which also would change
the version) I just set that statistics size again according to which
device is emulated. This also assures that the emulated device and
the statistics size always fit together, even if someone fiddles with
the saved state.
The "problem" with your approach if I understand it right is that I
couldn't do that since the device never knows when it would have to
re-fill these fields.
Basically what I am asking is if you couldn't just add an optional
callback so some additional stuff can be done after the "basic" state
has been loaded - or if that isn't desired at least a callback that
allows verifying the loaded values and aborting execution.

> > Saving both seems a bit like a bad design: the value of one implies the
> > exact value of the other, so it is at least pointless.
> > More importantly (though I do not know if qemu intends to care about
> > this) it might be able to hand-craft a saved vm that after loading then
> > violates some assumptions of the emulation code, possibly being
> > exploitable.
> 
> It is already that way.  This design don't change anything.  And I am
> not sure how to fix it.  We don't have a "is this value safe for this
> field", around yet.  It is possible to add some support for it, but I
> would like to 1st have an use case.

Well, I meant nowadays it is just possible to add a check in load_vm and
fix any values that are off. While it is quite a bit of work there is
nothing in the API stopping you from doing it, you even can return
-EINVAL and hopefully the core will print some somewhat useful message.

> > If nothing else, I'd at least add support for a "verify" function that
> > gets a "const state *" and can abort loading the VM in case someone
> > tries something evil (or can print some useful hint instead of having
> > qemu crash silently on the user, possibly at some later time).
> 
> This is as different problem that is not solved in qemu either.  I agree
> that it would be nice to have such a function, but I am not sure that I
> know how to do it.  and what is worse.  if you can modify the image, you
> can always change anything in the middle of the RAM.  I don't really see
> too much point trying to get a verify function for devices, when we
> can't have it for RAM.

That is completely different from what I meant.
Changing the RAM compromises the VM and only the VM, an exploit in a
device emulation might allow to compromise the _host_. Is it now clearer
what I meant?

Greetings,
Reimar Döffinger