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