On 10/15/2015 11:07 PM, Stefan Hajnoczi wrote:
On Wed, Oct 14, 2015 at 10:50:40PM +0800, Xiao Guangrong wrote:
On 10/14/2015 05:40 PM, Stefan Hajnoczi wrote:
On Sun, Oct 11, 2015 at 11:52:59AM +0800, Xiao Guangrong wrote:
+ out = (dsm_out *)in;
+
+ revision = in->arg1;
+ function = in->arg2;
+ handle = in->handle;
+ le32_to_cpus(&revision);
+ le32_to_cpus(&function);
+ le32_to_cpus(&handle);
+
+ nvdebug("UUID " UUID_FMT ".\n", in->arg0[0], in->arg0[1], in->arg0[2],
+ in->arg0[3], in->arg0[4], in->arg0[5], in->arg0[6],
+ in->arg0[7], in->arg0[8], in->arg0[9], in->arg0[10],
+ in->arg0[11], in->arg0[12], in->arg0[13], in->arg0[14],
+ in->arg0[15]);
+ nvdebug("Revision %#x Function %#x Handler %#x.\n", revision, function,
+ handle);
+
+ if (revision != DSM_REVISION) {
+ nvdebug("Revision %#x is not supported, expect %#x.\n",
+ revision, DSM_REVISION);
+ goto exit;
+ }
+
+ if (!handle) {
+ if (!dsm_is_root_uuid(in->arg0)) {
Please don't dereference 'in' or pass it to other functions. Avoid race
conditions with guest vcpus by coping in the entire dsm_in struct.
This is like a system call - the kernel cannot trust userspace memory
and must copy in before accessing data. The same rules apply.
It's little different for QEMU:
- the memory address is always valid to QEMU, it's not always true for Kernel
due to context-switch
- we have checked the header before use it's data, for example, when we get
data from GET_NAMESPACE_DATA, we have got the @offset and @length from the
memory, then copy memory based on these values, that means the userspace
has no chance to cause buffer overflow by increasing these values at runtime.
The scenario for our case is simple but Kernel is difficult to do
check_all_before_use as many paths may be involved.
- guest changes some data is okay, the worst case is that the label data is
corrupted. This is caused by guest itself. Kernel also supports this kind
of behaviour, e,g. network TX zero copy, the userspace page is being
transferred while userspace can still access it.
- it's 4K size on x86, full copy wastes CPU time too much.
This isn't performance-critical code and I don't want to review it
keeping the race conditions in mind the whole time. Also, if the code
is modified in the future, the chance of introducing a race is high.
I see this as premature optimization, please just copy in data.
No strong opinion on it... will do it following your idea.
