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

[Greg KH]

On Fri, Oct 28, 2011 at 07:50:59PM +0000, KY Srinivasan wrote:
> > >  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'?

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

> > > +				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
> process all available data before returning. This permits signaling optimizations between
> the producer and the consumer. For instance, if the consumer is still active as seen by the 
> producer (based on the read index seen by the producer), as the producer enqueues additional
> 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.

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/