| Message ID | 20130916183319.GM19256@mwanda (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> -----Original Message----- > From: Dan Carpenter [mailto:dan.carpenter@oracle.com] > Sent: Monday, September 16, 2013 11:33 AM > To: KY Srinivasan > Cc: Dmitry Torokhov; olaf@aepfle.de; gregkh@linuxfoundation.org; > jasowang@redhat.com; linux-kernel@vger.kernel.org; vojtech@suse.cz; linux- > input@vger.kernel.org; apw@canonical.com; devel@linuxdriverproject.org > Subject: Re: [PATCH 1/1] Drivers: input: serio: New driver to support Hyper-V > synthetic keyboard > > Just roll something like the following into your patch. > > regards, > dan carpenter > > diff --git a/drivers/input/serio/hyperv-keyboard.c b/drivers/input/serio/hyperv- > keyboard.c > index 0d4625f..262721b 100644 > --- a/drivers/input/serio/hyperv-keyboard.c > +++ b/drivers/input/serio/hyperv-keyboard.c > @@ -151,15 +151,18 @@ static void hv_kbd_free_device(struct hv_kbd_dev > *device) > } > > static void hv_kbd_on_receive(struct hv_device *device, > - struct vmpacket_descriptor *packet) > + struct vmpacket_descriptor *packet, size_t size) > { > struct synth_kbd_msg *msg; > struct hv_kbd_dev *kbd_dev = hv_get_drvdata(device); > struct synth_kbd_keystroke *ks_msg; > + int offset; > u16 scan_code; > > - msg = (struct synth_kbd_msg *)((unsigned long)packet + > - (packet->offset8 << 3)); > + offset = packet->offset8 << 3; > + if (offset + sizeof(struct synth_kbd_protocol_response) > size) > + return; > + msg = (void *)packet + offset; > > switch (msg->header.type) { > case SYNTH_KBD_PROTOCOL_RESPONSE: > @@ -220,7 +223,7 @@ static void hv_kbd_on_channel_callback(void *context) > break; > > case VM_PKT_DATA_INBAND: > - hv_kbd_on_receive(device, desc); > + hv_kbd_on_receive(device, desc, bytes_recvd); > break; > > default: Dan, Rolling the changes you have indicated is not the issue; this can trivially be done. My contention is that it is not needed given that the underlying function is already doing that. Look at the function vmbus_recvpacket_raw() in drivers/hv/channel.c. Regards, K. Y -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Sep 16, 2013 at 06:42:25PM +0000, KY Srinivasan wrote: > Dan, > > Rolling the changes you have indicated is not the issue; this can trivially be done. > My contention is that it is not needed given that the underlying function is already > doing that. Look at the function vmbus_recvpacket_raw() in drivers/hv/channel.c. > I'm confused. There is no mention of ->offset8 in vmbus_recvpacket_raw(). It's a good idea to add a check there but the lower levels don't know about the sizeof(synth_kbd_protocol_response) so we would still need something like my check. regards, dan carpenter -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----Original Message----- > From: Dan Carpenter [mailto:dan.carpenter@oracle.com] > Sent: Monday, September 16, 2013 1:13 PM > To: KY Srinivasan > Cc: olaf@aepfle.de; gregkh@linuxfoundation.org; jasowang@redhat.com; Dmitry > Torokhov; linux-kernel@vger.kernel.org; vojtech@suse.cz; linux- > input@vger.kernel.org; apw@canonical.com; devel@linuxdriverproject.org > Subject: Re: [PATCH 1/1] Drivers: input: serio: New driver to support Hyper-V > synthetic keyboard > > On Mon, Sep 16, 2013 at 06:42:25PM +0000, KY Srinivasan wrote: > > Dan, > > > > Rolling the changes you have indicated is not the issue; this can trivially be > done. > > My contention is that it is not needed given that the underlying function is > already > > doing that. Look at the function vmbus_recvpacket_raw() in > drivers/hv/channel.c. > > > > I'm confused. > > There is no mention of ->offset8 in vmbus_recvpacket_raw(). As you can see the vmbus_recvpacket_raw() ensures that the complete packet is read and if the buffer specified is not large enough nothing is read. The packet header has information about the length of the packet and the offset where the payload is. > > It's a good idea to add a check there but the lower levels don't know > about the sizeof(synth_kbd_protocol_response) so we would still need > something like my check. Why would the lower level code need to know anything about the layout of a particular message type. The lower level code is guaranteeing that a complete packet has been read in and that should be sufficient - assuming we trust the host. We have already spent more time on this than we should; I will make the necessary changes. Regards, K. Y > > regards, > dan carpenter -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Sep 16, 2013 at 09:55:44PM +0000, KY Srinivasan wrote: > > > > -----Original Message----- > > From: Dan Carpenter [mailto:dan.carpenter@oracle.com] > > Sent: Monday, September 16, 2013 1:13 PM > > To: KY Srinivasan > > Cc: olaf@aepfle.de; gregkh@linuxfoundation.org; jasowang@redhat.com; Dmitry > > Torokhov; linux-kernel@vger.kernel.org; vojtech@suse.cz; linux- > > input@vger.kernel.org; apw@canonical.com; devel@linuxdriverproject.org > > Subject: Re: [PATCH 1/1] Drivers: input: serio: New driver to support Hyper-V > > synthetic keyboard > > > > On Mon, Sep 16, 2013 at 06:42:25PM +0000, KY Srinivasan wrote: > > > Dan, > > > > > > Rolling the changes you have indicated is not the issue; this can trivially be > > done. > > > My contention is that it is not needed given that the underlying function is > > already > > > doing that. Look at the function vmbus_recvpacket_raw() in > > drivers/hv/channel.c. > > > > > > > I'm confused. > > > > There is no mention of ->offset8 in vmbus_recvpacket_raw(). > > As you can see the vmbus_recvpacket_raw() ensures that the complete > packet is read and if the buffer specified is not large enough nothing is > read. The packet header has information about the length of the packet > and the offset where the payload is. > > No one is talking about the ->len8. I'm saying that we should check ->offset8. > > It's a good idea to add a check there but the lower levels don't know > > about the sizeof(synth_kbd_protocol_response) so we would still need > > something like my check. > > Why would the lower level code need to know anything about the layout of a > particular message type. The lower level code is guaranteeing that a complete > packet has been read in and that should be sufficient - assuming we trust the host. > Of course, we don't need to check anything if we trust the host. I said that already. Just add the check for robustness. > We have already spent more time on this than we should; I will make the necessary > changes. Thank you. regards, dan carpenter -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/input/serio/hyperv-keyboard.c b/drivers/input/serio/hyperv-keyboard.c index 0d4625f..262721b 100644 --- a/drivers/input/serio/hyperv-keyboard.c +++ b/drivers/input/serio/hyperv-keyboard.c @@ -151,15 +151,18 @@ static void hv_kbd_free_device(struct hv_kbd_dev *device) } static void hv_kbd_on_receive(struct hv_device *device, - struct vmpacket_descriptor *packet) + struct vmpacket_descriptor *packet, size_t size) { struct synth_kbd_msg *msg; struct hv_kbd_dev *kbd_dev = hv_get_drvdata(device); struct synth_kbd_keystroke *ks_msg; + int offset; u16 scan_code; - msg = (struct synth_kbd_msg *)((unsigned long)packet + - (packet->offset8 << 3)); + offset = packet->offset8 << 3; + if (offset + sizeof(struct synth_kbd_protocol_response) > size) + return; + msg = (void *)packet + offset; switch (msg->header.type) { case SYNTH_KBD_PROTOCOL_RESPONSE: @@ -220,7 +223,7 @@ static void hv_kbd_on_channel_callback(void *context) break; case VM_PKT_DATA_INBAND: - hv_kbd_on_receive(device, desc); + hv_kbd_on_receive(device, desc, bytes_recvd); break; default: