From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1MdQV0-0004Ks-7F for qemu-devel@nongnu.org; Tue, 18 Aug 2009 11:21:22 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1MdQUv-0004FD-Ou for qemu-devel@nongnu.org; Tue, 18 Aug 2009 11:21:21 -0400 Received: from [199.232.76.173] (port=46056 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1MdQUv-0004F2-IL for qemu-devel@nongnu.org; Tue, 18 Aug 2009 11:21:17 -0400 Received: from mail.gmx.net ([213.165.64.20]:38425) by monty-python.gnu.org with smtp (Exim 4.60) (envelope-from ) id 1MdQUu-00042r-TA for qemu-devel@nongnu.org; Tue, 18 Aug 2009 11:21:17 -0400 Date: Tue, 18 Aug 2009 17:21:12 +0200 From: Reimar =?iso-8859-1?Q?D=F6ffinger?= Message-ID: <20090818152112.GA5483@1und1.de> References: <20090818142405.GA16563@1und1.de> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: Subject: [Qemu-devel] Re: [PATCH 5/5] Port apic to new VMState design List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Juan Quintela Cc: qemu-devel@nongnu.org On Tue, Aug 18, 2009 at 04:41:42PM +0200, Juan Quintela wrote: > Reimar Döffinger 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