RE: [PATCH 1/1] Staging: hv: Move the mouse driver out of staging

-----Original Message-----
From: Greg KH [mailto:gregkh@suse.de]
Sent: Friday, October 28, 2011 4:03 PM
To: KY Srinivasan
Cc: Jiri Kosina; linux-kernel@vger.kernel.org; devel@linuxdriverproject.org;
virtualization@lists.osdl.org; ohering@suse.com; Dmitry Torokhov
Subject: Re: [PATCH 1/1] Staging: hv: Move the mouse driver out of staging

On Fri, Oct 28, 2011 at 07:50:59PM +0000, KY Srinivasan wrote:

quoted

quoted

quoted

 obj-$(CONFIG_HID_WACOM)		+= hid-wacom.o
 obj-$(CONFIG_HID_WALTOP)	+= hid-waltop.o
 obj-$(CONFIG_HID_WIIMOTE)	+= hid-wiimote.o
+obj-$(CONFIG_HYPERV_MOUSE)	+= hv_mouse.o

I'd prefer a bit to follow the current naming of the drivers in
drivers/hid directory ... all the device-specific (vendor-specific)
drivers currently are called hid-<vendor> or similar.

We could talk about changing this naming scheme, but before we reach any
conclusion on this, I'd prefer this to stay for all drivers if possible.

Do you want the driver module to conform to the naming scheme you have?
Greg, in the past has resisted changing driver names, but I have no objection
to conforming to the naming convention you have.

I recommend following the proper naming scheme.  As this driver is
auto-loaded when the virtual hardware is seen, the name really doesn't
matter at all.

So, how about 'hid-hyperv' or 'hid-hv'?

I will go with hid-hv  if there are no objections.

quoted

quoted

quoted

+	switch (hid_msg->header.type) {
+	case SYNTH_HID_PROTOCOL_RESPONSE:
+		memcpy(&input_dev->protocol_resp, pipe_msg,
+		       pipe_msg->size + sizeof(struct pipe_prt_msg) -
+		       sizeof(unsigned char));

Is there any guarantee anywhere that packet doesn't get corrupted (either
maliciously or not)?

Seems to me like we are taking it 'raw' in mousevsc_on_channel_callback()
without any sanity checking.

If not, second argument of this memcpy() could easily overflow, causing
quite some memory corruption, right?

Actually the question of sanity of the raw packet is much more general one
throughout this driver. I am not very familiar with things in drivers/hv,
hence the question.

The guest cannot survive a malicious host; so I think it is safe to say that the
guest can assume the host is following the protocol.

That's not good for a very large number of reasons, not the least being
that we have no idea how secure the hyperv hypervisor is, so making it
so that there isn't an obvious hole into linux through it, would be a
good idea.

And yes, I'd say the same thing if this was a KVM or Xen driver as well.
Please be very defensive in this area of the code, especially as there
are no performance issues here.

In the chain of trust, the hypervisor and the host are the foundations
as far as the guest is concerned, since both the hypervisor and the host
can affect the guest in ways that the guest has no obvious way to protect itself.
If the hypervisor/host have security holes, there is not much you can do in the guest
to deal with it. 
In this case, I can add checks but I am not sure how useful it is.

quoted

quoted

quoted

+				return;
+			}
+			break;
+		}
+	} while (1);

Again, not being familiar with 'hv' infrastructure at all, this inifite
loop makes me to ask -- in what context does it run? Why do we need it?

It seems to me that it's some kind of infinite loop for event-driven
programming, which is not something we do in kernel (outside of kernel
threads, perhaps, with very careful handling).

This loop is invoked in a tasklet context. The agreed upon protocol between the
guest and the host is that the consumer of a ring buffer (in this case the guest)

will

quoted

process all available data before returning. This permits signaling optimizations

between

quoted

the producer and the consumer. For instance, if the consumer is still active as

seen by the

quoted

producer (based on the read index seen by the producer), as the producer

enqueues additional

quoted

data, the producer does not have to signal the consumer.

That's great, but again, be defensive and at least have a "time out"
way to get out of this loop in case something goes wrong.  It will be
triggered some day, by someone, I guarantee it.

I am not sure what you are recommending here Greg; the host mandates that
we exit this loop only when there is nothing more to be read; that exit path is in the
current code. If we exit this code for any other reason (such as based on a timeout)
we will not be awakened by the host when in fact there is data to be read.

Regards,

K. Y