[PATCH 1/1] Drivers: input: serio: New driver to support Hyper-V synthetic keyboard

[PATCH 1/1] Drivers: input: serio: New driver to support Hyper-V synthetic keyboard

[K. Y. Srinivasan]

Add a new driver to support synthetic keyboard. On the next generation
Hyper-V guest firmware, many legacy devices will not be emulated and this
driver will be required.

I would like to thank Vojtech Pavlik <vojtech@suse.cz> for helping me with the
details of the AT keyboard driver. 

Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
---
 drivers/input/serio/Kconfig           |    7 +
 drivers/input/serio/Makefile          |    1 +
 drivers/input/serio/hyperv-keyboard.c |  379 +++++++++++++++++++++++++++++++++
 3 files changed, 387 insertions(+), 0 deletions(-)
 create mode 100644 drivers/input/serio/hyperv-keyboard.c

diff --git a/drivers/input/serio/Kconfig b/drivers/input/serio/Kconfig
index 1e691a3..f3996e7 100644
--- a/drivers/input/serio/Kconfig
+++ b/drivers/input/serio/Kconfig
@@ -267,4 +267,11 @@ config SERIO_OLPC_APSP
 	  To compile this driver as a module, choose M here: the module will
 	  be called olpc_apsp.
 
+config HYPERV_KEYBOARD
+	tristate "Microsoft Synthetic Keyboard driver"
+	depends on HYPERV
+	default HYPERV
+	help
+	  Select this option to enable the Hyper-V Keyboard driver.
+
 endif
diff --git a/drivers/input/serio/Makefile b/drivers/input/serio/Makefile
index 12298b1..815d874 100644
--- a/drivers/input/serio/Makefile
+++ b/drivers/input/serio/Makefile
@@ -28,3 +28,4 @@ obj-$(CONFIG_SERIO_ALTERA_PS2)	+= altera_ps2.o
 obj-$(CONFIG_SERIO_ARC_PS2)	+= arc_ps2.o
 obj-$(CONFIG_SERIO_APBPS2)	+= apbps2.o
 obj-$(CONFIG_SERIO_OLPC_APSP)	+= olpc_apsp.o
+obj-$(CONFIG_HYPERV_KEYBOARD)	+= hyperv-keyboard.o
diff --git a/drivers/input/serio/hyperv-keyboard.c b/drivers/input/serio/hyperv-keyboard.c
new file mode 100644
index 0000000..0d4625f
--- /dev/null
+++ b/drivers/input/serio/hyperv-keyboard.c
@@ -0,0 +1,379 @@
+/*
+ *  Copyright (c) 2013, Microsoft Corporation.
+ *
+ *  This program is free software; you can redistribute it and/or modify it
+ *  under the terms and conditions of the GNU General Public License,
+ *  version 2, as published by the Free Software Foundation.
+ *
+ *  This program is distributed in the hope it will be useful, but WITHOUT
+ *  ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ *  FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ *  more details.
+ */
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/device.h>
+#include <linux/completion.h>
+#include <linux/hyperv.h>
+#include <linux/serio.h>
+#include <linux/slab.h>
+
+
+/*
+ * Current version 1.0
+ *
+ */
+#define SYNTH_KBD_VERSION_MAJOR 1
+#define SYNTH_KBD_VERSION_MINOR	0
+#define SYNTH_KBD_VERSION		(SYNTH_KBD_VERSION_MINOR | \
+					 (SYNTH_KBD_VERSION_MAJOR << 16))
+
+
+/*
+ * Message types in the synthetic input protocol
+ */
+enum synth_kbd_msg_type {
+	SYNTH_KBD_PROTOCOL_REQUEST = 1,
+	SYNTH_KBD_PROTOCOL_RESPONSE = 2,
+	SYNTH_KBD_EVENT = 3,
+	SYNTH_KBD_LED_INDICATORS = 4,
+};
+
+/*
+ * Basic message structures.
+ */
+struct synth_kbd_msg_hdr {
+	enum synth_kbd_msg_type type;
+};
+
+struct synth_kbd_msg {
+	struct synth_kbd_msg_hdr header;
+	char data[1]; /* Enclosed message */
+};
+
+union synth_kbd_version {
+	struct {
+		u16 minor_version;
+		u16 major_version;
+	};
+	u32 version;
+};
+
+/*
+ * Protocol messages
+ */
+struct synth_kbd_protocol_request {
+	struct synth_kbd_msg_hdr header;
+	union synth_kbd_version version_requested;
+};
+
+struct synth_kbd_protocol_response {
+	struct synth_kbd_msg_hdr header;
+	u32 accepted:1;
+	u32 reserved:31;
+};
+
+struct synth_kbd_keystroke {
+	struct synth_kbd_msg_hdr header;
+	u16 make_code;
+	u16 reserved0;
+	u32 is_unicode:1;
+	u32 is_break:1;
+	u32 is_e0:1;
+	u32 is_e1:1;
+	u32 reserved:28;
+};
+
+
+#define HK_MAXIMUM_MESSAGE_SIZE 256
+
+#define KBD_VSC_SEND_RING_BUFFER_SIZE		(10 * PAGE_SIZE)
+#define KBD_VSC_RECV_RING_BUFFER_SIZE		(10 * PAGE_SIZE)
+
+#define XTKBD_EMUL0     0xe0
+#define XTKBD_EMUL1     0xe1
+#define XTKBD_RELEASE   0x80
+
+
+/*
+ * Represents a keyboard device
+ */
+struct hv_kbd_dev {
+	unsigned char keycode[256];
+	struct hv_device	*device;
+	struct synth_kbd_protocol_request protocol_req;
+	struct synth_kbd_protocol_response protocol_resp;
+	/* Synchronize the request/response if needed */
+	struct completion	wait_event;
+	struct serio		*hv_serio;
+};
+
+
+static struct hv_kbd_dev *hv_kbd_alloc_device(struct hv_device *device)
+{
+	struct hv_kbd_dev *kbd_dev;
+	struct serio	*hv_serio;
+
+	kbd_dev = kzalloc(sizeof(struct hv_kbd_dev), GFP_KERNEL);
+
+	if (!kbd_dev)
+		return NULL;
+
+	hv_serio = kzalloc(sizeof(struct serio), GFP_KERNEL);
+
+	if (hv_serio == NULL) {
+		kfree(kbd_dev);
+		return NULL;
+	}
+
+	hv_serio->id.type	= SERIO_8042_XL;
+	strlcpy(hv_serio->name, dev_name(&device->device),
+		sizeof(hv_serio->name));
+	strlcpy(hv_serio->phys, dev_name(&device->device),
+		sizeof(hv_serio->phys));
+	hv_serio->dev.parent  = &device->device;
+
+
+	kbd_dev->device = device;
+	kbd_dev->hv_serio = hv_serio;
+	hv_set_drvdata(device, kbd_dev);
+	init_completion(&kbd_dev->wait_event);
+
+	return kbd_dev;
+}
+
+static void hv_kbd_free_device(struct hv_kbd_dev *device)
+{
+	serio_unregister_port(device->hv_serio);
+	kfree(device->hv_serio);
+	hv_set_drvdata(device->device, NULL);
+	kfree(device);
+}
+
+static void hv_kbd_on_receive(struct hv_device *device,
+				struct vmpacket_descriptor *packet)
+{
+	struct synth_kbd_msg *msg;
+	struct hv_kbd_dev *kbd_dev = hv_get_drvdata(device);
+	struct synth_kbd_keystroke *ks_msg;
+	u16 scan_code;
+
+	msg = (struct synth_kbd_msg *)((unsigned long)packet +
+					(packet->offset8 << 3));
+
+	switch (msg->header.type) {
+	case SYNTH_KBD_PROTOCOL_RESPONSE:
+		memcpy(&kbd_dev->protocol_resp, msg,
+			sizeof(struct synth_kbd_protocol_response));
+		complete(&kbd_dev->wait_event);
+		break;
+	case SYNTH_KBD_EVENT:
+		ks_msg = (struct synth_kbd_keystroke *)msg;
+		scan_code = ks_msg->make_code;
+
+		/*
+		 * Inject the information through the serio interrupt.
+		 */
+		if (ks_msg->is_e0)
+			serio_interrupt(kbd_dev->hv_serio, XTKBD_EMUL0, 0);
+		serio_interrupt(kbd_dev->hv_serio,
+				scan_code | (ks_msg->is_break ?
+				XTKBD_RELEASE : 0),
+				0);
+
+		break;
+
+	default:
+		pr_err("unhandled message type %d\n", msg->header.type);
+	}
+}
+
+static void hv_kbd_on_channel_callback(void *context)
+{
+	const int packet_size = 0x100;
+	int ret;
+	struct hv_device *device = context;
+	u32 bytes_recvd;
+	u64 req_id;
+	struct vmpacket_descriptor *desc;
+	unsigned char	*buffer;
+	int	bufferlen = packet_size;
+
+	buffer = kmalloc(bufferlen, GFP_ATOMIC);
+	if (!buffer)
+		return;
+
+	do {
+		ret = vmbus_recvpacket_raw(device->channel, buffer,
+					bufferlen, &bytes_recvd, &req_id);
+
+		switch (ret) {
+		case 0:
+			if (bytes_recvd <= 0) {
+				kfree(buffer);
+				return;
+			}
+			desc = (struct vmpacket_descriptor *)buffer;
+
+			switch (desc->type) {
+			case VM_PKT_COMP:
+				break;
+
+			case VM_PKT_DATA_INBAND:
+				hv_kbd_on_receive(device, desc);
+				break;
+
+			default:
+				pr_err("unhandled packet type %d, tid %llx len %d\n",
+					desc->type, req_id, bytes_recvd);
+				break;
+			}
+
+			break;
+
+		case -ENOBUFS:
+			kfree(buffer);
+			/* Handle large packet */
+			bufferlen = bytes_recvd;
+			buffer = kmalloc(bytes_recvd, GFP_ATOMIC);
+
+			if (!buffer)
+				return;
+
+			break;
+		}
+	} while (1);
+
+}
+
+static int hv_kbd_connect_to_vsp(struct hv_device *device)
+{
+	int ret = 0;
+	int t;
+	struct hv_kbd_dev *kbd_dev = hv_get_drvdata(device);
+	struct synth_kbd_protocol_request *request;
+	struct synth_kbd_protocol_response *response;
+
+	request = &kbd_dev->protocol_req;
+	memset(request, 0, sizeof(struct synth_kbd_protocol_request));
+
+	request->header.type = SYNTH_KBD_PROTOCOL_REQUEST;
+	request->version_requested.version = SYNTH_KBD_VERSION;
+
+	ret = vmbus_sendpacket(device->channel, request,
+				sizeof(struct synth_kbd_protocol_request),
+				(unsigned long)request,
+				VM_PKT_DATA_INBAND,
+				VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
+	if (ret)
+		goto cleanup;
+
+	t = wait_for_completion_timeout(&kbd_dev->wait_event, 10 * HZ);
+	if (!t) {
+		ret = -ETIMEDOUT;
+		goto cleanup;
+	}
+
+	response = &kbd_dev->protocol_resp;
+
+	if (!response->accepted) {
+		pr_err("synth_kbd protocol request failed (version %d)\n",
+		       SYNTH_KBD_VERSION);
+		ret = -ENODEV;
+		goto cleanup;
+	}
+
+
+cleanup:
+	return ret;
+}
+
+static int hv_kbd_probe(struct hv_device *device,
+			const struct hv_vmbus_device_id *dev_id)
+{
+	int ret;
+	struct hv_kbd_dev *kbd_dev;
+
+	kbd_dev = hv_kbd_alloc_device(device);
+
+	if (!kbd_dev)
+		return -ENOMEM;
+
+	ret = vmbus_open(device->channel,
+		KBD_VSC_SEND_RING_BUFFER_SIZE,
+		KBD_VSC_RECV_RING_BUFFER_SIZE,
+		NULL,
+		0,
+		hv_kbd_on_channel_callback,
+		device
+		);
+
+	if (ret)
+		goto probe_err0;
+
+	ret = hv_kbd_connect_to_vsp(device);
+
+	if (ret)
+		goto probe_err1;
+
+	serio_register_port(kbd_dev->hv_serio);
+
+	return ret;
+
+probe_err1:
+	vmbus_close(device->channel);
+
+probe_err0:
+	hv_kbd_free_device(kbd_dev);
+
+	return ret;
+}
+
+
+static int hv_kbd_remove(struct hv_device *dev)
+{
+	struct hv_kbd_dev *kbd_dev = hv_get_drvdata(dev);
+
+	vmbus_close(dev->channel);
+	hv_kbd_free_device(kbd_dev);
+	return 0;
+}
+
+/*
+ * Keyboard GUID
+ * {f912ad6d-2b17-48ea-bd65-f927a61c7684}
+ */
+#define HV_KBD_GUID \
+	.guid = { \
+			0x6d, 0xad, 0x12, 0xf9, 0x17, 0x2b, 0xea, 0x48, \
+			0xbd, 0x65, 0xf9, 0x27, 0xa6, 0x1c, 0x76, 0x84 \
+	}
+
+static const struct hv_vmbus_device_id id_table[] = {
+	/* Keyboard guid */
+	{ HV_KBD_GUID, },
+	{ },
+};
+
+MODULE_DEVICE_TABLE(vmbus, id_table);
+
+static struct  hv_driver hv_kbd_drv = {
+	.name = KBUILD_MODNAME,
+	.id_table = id_table,
+	.probe = hv_kbd_probe,
+	.remove = hv_kbd_remove,
+};
+
+static int __init hv_kbd_init(void)
+{
+	return vmbus_driver_register(&hv_kbd_drv);
+}
+
+static void __exit hv_kbd_exit(void)
+{
+	vmbus_driver_unregister(&hv_kbd_drv);
+}
+
+MODULE_LICENSE("GPL");
+module_init(hv_kbd_init);
+module_exit(hv_kbd_exit);
-- 
1.7.4.1

Re: [PATCH 1/1] Drivers: input: serio: New driver to support Hyper-V synthetic keyboard

[Dan Carpenter]

The main thing is that could you improve the error handling in
hv_kbd_on_channel_callback() explained inline.

On Sun, Sep 15, 2013 at 10:28:54PM -0700, K. Y. Srinivasan wrote:
> Add a new driver to support synthetic keyboard. On the next generation
> Hyper-V guest firmware, many legacy devices will not be emulated and this
> driver will be required.
> 
> I would like to thank Vojtech Pavlik <vojtech@suse.cz> for helping me with the
> details of the AT keyboard driver. 
> 
> Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> ---
>  drivers/input/serio/Kconfig           |    7 +
>  drivers/input/serio/Makefile          |    1 +
>  drivers/input/serio/hyperv-keyboard.c |  379 +++++++++++++++++++++++++++++++++
>  3 files changed, 387 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/input/serio/hyperv-keyboard.c
> 
> diff --git a/drivers/input/serio/Kconfig b/drivers/input/serio/Kconfig
> index 1e691a3..f3996e7 100644
> --- a/drivers/input/serio/Kconfig
> +++ b/drivers/input/serio/Kconfig
> @@ -267,4 +267,11 @@ config SERIO_OLPC_APSP
>  	  To compile this driver as a module, choose M here: the module will
>  	  be called olpc_apsp.
>  
> +config HYPERV_KEYBOARD
> +	tristate "Microsoft Synthetic Keyboard driver"
> +	depends on HYPERV
> +	default HYPERV
> +	help
> +	  Select this option to enable the Hyper-V Keyboard driver.
> +
>  endif
> diff --git a/drivers/input/serio/Makefile b/drivers/input/serio/Makefile
> index 12298b1..815d874 100644
> --- a/drivers/input/serio/Makefile
> +++ b/drivers/input/serio/Makefile
> @@ -28,3 +28,4 @@ obj-$(CONFIG_SERIO_ALTERA_PS2)	+= altera_ps2.o
>  obj-$(CONFIG_SERIO_ARC_PS2)	+= arc_ps2.o
>  obj-$(CONFIG_SERIO_APBPS2)	+= apbps2.o
>  obj-$(CONFIG_SERIO_OLPC_APSP)	+= olpc_apsp.o
> +obj-$(CONFIG_HYPERV_KEYBOARD)	+= hyperv-keyboard.o
> diff --git a/drivers/input/serio/hyperv-keyboard.c b/drivers/input/serio/hyperv-keyboard.c
> new file mode 100644
> index 0000000..0d4625f
> --- /dev/null
> +++ b/drivers/input/serio/hyperv-keyboard.c
> @@ -0,0 +1,379 @@
> +/*
> + *  Copyright (c) 2013, Microsoft Corporation.
> + *
> + *  This program is free software; you can redistribute it and/or modify it
> + *  under the terms and conditions of the GNU General Public License,
> + *  version 2, as published by the Free Software Foundation.
> + *
> + *  This program is distributed in the hope it will be useful, but WITHOUT
> + *  ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + *  FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + *  more details.
> + */
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/device.h>
> +#include <linux/completion.h>
> +#include <linux/hyperv.h>
> +#include <linux/serio.h>
> +#include <linux/slab.h>
> +
> +

Extra blank line.

> +/*
> + * Current version 1.0
> + *
> + */
> +#define SYNTH_KBD_VERSION_MAJOR 1
> +#define SYNTH_KBD_VERSION_MINOR	0
> +#define SYNTH_KBD_VERSION		(SYNTH_KBD_VERSION_MINOR | \
> +					 (SYNTH_KBD_VERSION_MAJOR << 16))
> +
> +
> +/*
> + * Message types in the synthetic input protocol
> + */
> +enum synth_kbd_msg_type {
> +	SYNTH_KBD_PROTOCOL_REQUEST = 1,
> +	SYNTH_KBD_PROTOCOL_RESPONSE = 2,
> +	SYNTH_KBD_EVENT = 3,
> +	SYNTH_KBD_LED_INDICATORS = 4,
> +};
> +
> +/*
> + * Basic message structures.
> + */
> +struct synth_kbd_msg_hdr {
> +	enum synth_kbd_msg_type type;
> +};

Enum type is wrong here because sizeof(enum) is up to the compiler to
decide.

> +
> +struct synth_kbd_msg {
> +	struct synth_kbd_msg_hdr header;
> +	char data[1]; /* Enclosed message */
> +};

You could use a zero size array instead.

> +
> +union synth_kbd_version {
> +	struct {
> +		u16 minor_version;
> +		u16 major_version;
> +	};
> +	u32 version;
> +};
> +
> +/*
> + * Protocol messages
> + */
> +struct synth_kbd_protocol_request {
> +	struct synth_kbd_msg_hdr header;
> +	union synth_kbd_version version_requested;
> +};
> +
> +struct synth_kbd_protocol_response {
> +	struct synth_kbd_msg_hdr header;
> +	u32 accepted:1;
> +	u32 reserved:31;
> +};
> +
> +struct synth_kbd_keystroke {
> +	struct synth_kbd_msg_hdr header;
> +	u16 make_code;
> +	u16 reserved0;
> +	u32 is_unicode:1;
> +	u32 is_break:1;
> +	u32 is_e0:1;
> +	u32 is_e1:1;
> +	u32 reserved:28;
> +};
> +
> +

Extra blank line.

> +#define HK_MAXIMUM_MESSAGE_SIZE 256
> +
> +#define KBD_VSC_SEND_RING_BUFFER_SIZE		(10 * PAGE_SIZE)
> +#define KBD_VSC_RECV_RING_BUFFER_SIZE		(10 * PAGE_SIZE)
> +
> +#define XTKBD_EMUL0     0xe0
> +#define XTKBD_EMUL1     0xe1
> +#define XTKBD_RELEASE   0x80
> +
> +

Extra blank.

> +/*
> + * Represents a keyboard device
> + */
> +struct hv_kbd_dev {
> +	unsigned char keycode[256];
> +	struct hv_device	*device;
> +	struct synth_kbd_protocol_request protocol_req;
> +	struct synth_kbd_protocol_response protocol_resp;
> +	/* Synchronize the request/response if needed */
> +	struct completion	wait_event;
> +	struct serio		*hv_serio;
> +};
> +
> +

Extra blank.

> +static struct hv_kbd_dev *hv_kbd_alloc_device(struct hv_device *device)
> +{
> +	struct hv_kbd_dev *kbd_dev;
> +	struct serio	*hv_serio;
> +
> +	kbd_dev = kzalloc(sizeof(struct hv_kbd_dev), GFP_KERNEL);
> +

Spurious blank line.

> +	if (!kbd_dev)
> +		return NULL;
> +
> +	hv_serio = kzalloc(sizeof(struct serio), GFP_KERNEL);
> +

Spurious blank.

> +	if (hv_serio == NULL) {
> +		kfree(kbd_dev);
> +		return NULL;
> +	}
> +
> +	hv_serio->id.type	= SERIO_8042_XL;

Pointless tab before the '='.

> +	strlcpy(hv_serio->name, dev_name(&device->device),
> +		sizeof(hv_serio->name));
> +	strlcpy(hv_serio->phys, dev_name(&device->device),
> +		sizeof(hv_serio->phys));
> +	hv_serio->dev.parent  = &device->device;

Why are there two spaces before the '='?

> +
> +

Extra blank line.

> +	kbd_dev->device = device;
> +	kbd_dev->hv_serio = hv_serio;
> +	hv_set_drvdata(device, kbd_dev);
> +	init_completion(&kbd_dev->wait_event);
> +
> +	return kbd_dev;
> +}
> +
> +static void hv_kbd_free_device(struct hv_kbd_dev *device)
> +{
> +	serio_unregister_port(device->hv_serio);
> +	kfree(device->hv_serio);
> +	hv_set_drvdata(device->device, NULL);
> +	kfree(device);
> +}
> +
> +static void hv_kbd_on_receive(struct hv_device *device,
> +				struct vmpacket_descriptor *packet)
> +{
> +	struct synth_kbd_msg *msg;
> +	struct hv_kbd_dev *kbd_dev = hv_get_drvdata(device);
> +	struct synth_kbd_keystroke *ks_msg;
> +	u16 scan_code;
> +
> +	msg = (struct synth_kbd_msg *)((unsigned long)packet +
> +					(packet->offset8 << 3));
> +
> +	switch (msg->header.type) {
> +	case SYNTH_KBD_PROTOCOL_RESPONSE:
> +		memcpy(&kbd_dev->protocol_resp, msg,
> +			sizeof(struct synth_kbd_protocol_response));
> +		complete(&kbd_dev->wait_event);
> +		break;
> +	case SYNTH_KBD_EVENT:
> +		ks_msg = (struct synth_kbd_keystroke *)msg;
> +		scan_code = ks_msg->make_code;
> +
> +		/*
> +		 * Inject the information through the serio interrupt.
> +		 */
> +		if (ks_msg->is_e0)
> +			serio_interrupt(kbd_dev->hv_serio, XTKBD_EMUL0, 0);
> +		serio_interrupt(kbd_dev->hv_serio,
> +				scan_code | (ks_msg->is_break ?
> +				XTKBD_RELEASE : 0),
> +				0);
> +

It would be more readable to do:

		if (ks_msg->is_break)
			scan_code |= XTKBD_RELEASE;
		serio_interrupt(kbd_dev->hv_serio, scan_code, 0);


> +		break;
> +
> +	default:
> +		pr_err("unhandled message type %d\n", msg->header.type);

Just a question.  This can only be triggered by the hyperviser, right?

> +	}
> +}
> +
> +static void hv_kbd_on_channel_callback(void *context)
> +{
> +	const int packet_size = 0x100;
> +	int ret;
> +	struct hv_device *device = context;
> +	u32 bytes_recvd;
> +	u64 req_id;
> +	struct vmpacket_descriptor *desc;
> +	unsigned char	*buffer;
> +	int	bufferlen = packet_size;
> +
> +	buffer = kmalloc(bufferlen, GFP_ATOMIC);
> +	if (!buffer)
> +		return;
> +
> +	do {


Forever loops should be while (1) { instead of do { } while (1).


> +		ret = vmbus_recvpacket_raw(device->channel, buffer,
> +					bufferlen, &bytes_recvd, &req_id);
> +
> +		switch (ret) {
> +		case 0:
> +			if (bytes_recvd <= 0) {

There can never be a negative number of bytes_recvd.

> +				kfree(buffer);
> +				return;
> +			}
> +			desc = (struct vmpacket_descriptor *)buffer;
> +
> +			switch (desc->type) {
> +			case VM_PKT_COMP:
> +				break;
> +
> +			case VM_PKT_DATA_INBAND:
> +				hv_kbd_on_receive(device, desc);

This is the error handling I mentioned at the top.  hv_kbd_on_receive()
doesn't take into consideration the amount of data we recieved, it
trusts the offset we recieved from the user.  There is an out of bounds
read.

> +				break;
> +
> +			default:
> +				pr_err("unhandled packet type %d, tid %llx len %d\n",
> +					desc->type, req_id, bytes_recvd);
> +				break;
> +			}
> +
> +			break;
> +
> +		case -ENOBUFS:
> +			kfree(buffer);
> +			/* Handle large packet */
> +			bufferlen = bytes_recvd;
> +			buffer = kmalloc(bytes_recvd, GFP_ATOMIC);
> +
> +			if (!buffer)
> +				return;
> +
> +			break;
> +		}
> +	} while (1);
> +
> +}
> +
> +static int hv_kbd_connect_to_vsp(struct hv_device *device)
> +{
> +	int ret = 0;

Don't initialize this.

> +	int t;
> +	struct hv_kbd_dev *kbd_dev = hv_get_drvdata(device);
> +	struct synth_kbd_protocol_request *request;
> +	struct synth_kbd_protocol_response *response;
> +
> +	request = &kbd_dev->protocol_req;
> +	memset(request, 0, sizeof(struct synth_kbd_protocol_request));
> +
> +	request->header.type = SYNTH_KBD_PROTOCOL_REQUEST;
> +	request->version_requested.version = SYNTH_KBD_VERSION;
> +
> +	ret = vmbus_sendpacket(device->channel, request,
> +				sizeof(struct synth_kbd_protocol_request),
> +				(unsigned long)request,
> +				VM_PKT_DATA_INBAND,
> +				VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
> +	if (ret)
> +		goto cleanup;

There is no cleanup.  Just return directly.

> +
> +	t = wait_for_completion_timeout(&kbd_dev->wait_event, 10 * HZ);
> +	if (!t) {
> +		ret = -ETIMEDOUT;
> +		goto cleanup;

	return -ETIMEOUT;

> +	}
> +
> +	response = &kbd_dev->protocol_resp;
> +
> +	if (!response->accepted) {
> +		pr_err("synth_kbd protocol request failed (version %d)\n",
> +		       SYNTH_KBD_VERSION);
> +		ret = -ENODEV;
> +		goto cleanup;

Just return -ENODEV;

> +	}
> +

	return 0;

> +
> +cleanup:
> +	return ret;
> +}
> +
> +static int hv_kbd_probe(struct hv_device *device,
> +			const struct hv_vmbus_device_id *dev_id)
> +{
> +	int ret;
> +	struct hv_kbd_dev *kbd_dev;
> +
> +	kbd_dev = hv_kbd_alloc_device(device);
> +

Delete the blank line.

> +	if (!kbd_dev)
> +		return -ENOMEM;
> +
> +	ret = vmbus_open(device->channel,
> +		KBD_VSC_SEND_RING_BUFFER_SIZE,
> +		KBD_VSC_RECV_RING_BUFFER_SIZE,
> +		NULL,
> +		0,
> +		hv_kbd_on_channel_callback,
> +		device
> +		);
> +

Delete the blank line.

> +	if (ret)
> +		goto probe_err0;
> +
> +	ret = hv_kbd_connect_to_vsp(device);
> +

Delete the blank line.

> +	if (ret)
> +		goto probe_err1;
> +
> +	serio_register_port(kbd_dev->hv_serio);
> +
> +	return ret;

	return 0;

> +
> +probe_err1:
> +	vmbus_close(device->channel);

The label here should be "err_close:".  The number is more GW-Basic
style than C.

> +
> +probe_err0:

The label should be "err_free_dev:".

> +	hv_kbd_free_device(kbd_dev);
> +
> +	return ret;
> +}
> +
> +

Extra blank line.

> +static int hv_kbd_remove(struct hv_device *dev)

regards,
dan carpenter

RE: [PATCH 1/1] Drivers: input: serio: New driver to support Hyper-V synthetic keyboard

[KY Srinivasan]

> -----Original Message-----
> From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
> Sent: Monday, September 16, 2013 1:21 AM
> To: KY Srinivasan
> Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; linux-input@vger.kernel.org;
> dmitry.torokhov@gmail.com; vojtech@suse.cz; olaf@aepfle.de;
> apw@canonical.com; jasowang@redhat.com
> Subject: Re: [PATCH 1/1] Drivers: input: serio: New driver to support Hyper-V
> synthetic keyboard
> 
> The main thing is that could you improve the error handling in
> hv_kbd_on_channel_callback() explained inline.

Thanks Dan. I will address all the relevant issues you have pointed out.
I have answered/responded to your comments in-line.
> 
> On Sun, Sep 15, 2013 at 10:28:54PM -0700, K. Y. Srinivasan wrote:
> > Add a new driver to support synthetic keyboard. On the next generation
> > Hyper-V guest firmware, many legacy devices will not be emulated and this
> > driver will be required.
> >
> > I would like to thank Vojtech Pavlik <vojtech@suse.cz> for helping me with the
> > details of the AT keyboard driver.
> >
> > Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> > ---
> >  drivers/input/serio/Kconfig           |    7 +
> >  drivers/input/serio/Makefile          |    1 +
> >  drivers/input/serio/hyperv-keyboard.c |  379
> +++++++++++++++++++++++++++++++++
> >  3 files changed, 387 insertions(+), 0 deletions(-)
> >  create mode 100644 drivers/input/serio/hyperv-keyboard.c
> >
> > diff --git a/drivers/input/serio/Kconfig b/drivers/input/serio/Kconfig
> > index 1e691a3..f3996e7 100644
> > --- a/drivers/input/serio/Kconfig
> > +++ b/drivers/input/serio/Kconfig
> > @@ -267,4 +267,11 @@ config SERIO_OLPC_APSP
> >  	  To compile this driver as a module, choose M here: the module will
> >  	  be called olpc_apsp.
> >
> > +config HYPERV_KEYBOARD
> > +	tristate "Microsoft Synthetic Keyboard driver"
> > +	depends on HYPERV
> > +	default HYPERV
> > +	help
> > +	  Select this option to enable the Hyper-V Keyboard driver.
> > +
> >  endif
> > diff --git a/drivers/input/serio/Makefile b/drivers/input/serio/Makefile
> > index 12298b1..815d874 100644
> > --- a/drivers/input/serio/Makefile
> > +++ b/drivers/input/serio/Makefile
> > @@ -28,3 +28,4 @@ obj-$(CONFIG_SERIO_ALTERA_PS2)	+= altera_ps2.o
> >  obj-$(CONFIG_SERIO_ARC_PS2)	+= arc_ps2.o
> >  obj-$(CONFIG_SERIO_APBPS2)	+= apbps2.o
> >  obj-$(CONFIG_SERIO_OLPC_APSP)	+= olpc_apsp.o
> > +obj-$(CONFIG_HYPERV_KEYBOARD)	+= hyperv-keyboard.o
> > diff --git a/drivers/input/serio/hyperv-keyboard.c
> b/drivers/input/serio/hyperv-keyboard.c
> > new file mode 100644
> > index 0000000..0d4625f
> > --- /dev/null
> > +++ b/drivers/input/serio/hyperv-keyboard.c
> > @@ -0,0 +1,379 @@
> > +/*
> > + *  Copyright (c) 2013, Microsoft Corporation.
> > + *
> > + *  This program is free software; you can redistribute it and/or modify it
> > + *  under the terms and conditions of the GNU General Public License,
> > + *  version 2, as published by the Free Software Foundation.
> > + *
> > + *  This program is distributed in the hope it will be useful, but WITHOUT
> > + *  ANY WARRANTY; without even the implied warranty of MERCHANTABILITY
> or
> > + *  FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
> for
> > + *  more details.
> > + */
> > +#include <linux/init.h>
> > +#include <linux/module.h>
> > +#include <linux/device.h>
> > +#include <linux/completion.h>
> > +#include <linux/hyperv.h>
> > +#include <linux/serio.h>
> > +#include <linux/slab.h>
> > +
> > +
> 
> Extra blank line.
> 
> > +/*
> > + * Current version 1.0
> > + *
> > + */
> > +#define SYNTH_KBD_VERSION_MAJOR 1
> > +#define SYNTH_KBD_VERSION_MINOR	0
> > +#define SYNTH_KBD_VERSION		(SYNTH_KBD_VERSION_MINOR |
> \
> > +					 (SYNTH_KBD_VERSION_MAJOR << 16))
> > +
> > +
> > +/*
> > + * Message types in the synthetic input protocol
> > + */
> > +enum synth_kbd_msg_type {
> > +	SYNTH_KBD_PROTOCOL_REQUEST = 1,
> > +	SYNTH_KBD_PROTOCOL_RESPONSE = 2,
> > +	SYNTH_KBD_EVENT = 3,
> > +	SYNTH_KBD_LED_INDICATORS = 4,
> > +};
> > +
> > +/*
> > + * Basic message structures.
> > + */
> > +struct synth_kbd_msg_hdr {
> > +	enum synth_kbd_msg_type type;
> > +};
> 
> Enum type is wrong here because sizeof(enum) is up to the compiler to
> decide.
> 
> > +
> > +struct synth_kbd_msg {
> > +	struct synth_kbd_msg_hdr header;
> > +	char data[1]; /* Enclosed message */
> > +};
> 
> You could use a zero size array instead.
> 
> > +
> > +union synth_kbd_version {
> > +	struct {
> > +		u16 minor_version;
> > +		u16 major_version;
> > +	};
> > +	u32 version;
> > +};
> > +
> > +/*
> > + * Protocol messages
> > + */
> > +struct synth_kbd_protocol_request {
> > +	struct synth_kbd_msg_hdr header;
> > +	union synth_kbd_version version_requested;
> > +};
> > +
> > +struct synth_kbd_protocol_response {
> > +	struct synth_kbd_msg_hdr header;
> > +	u32 accepted:1;
> > +	u32 reserved:31;
> > +};
> > +
> > +struct synth_kbd_keystroke {
> > +	struct synth_kbd_msg_hdr header;
> > +	u16 make_code;
> > +	u16 reserved0;
> > +	u32 is_unicode:1;
> > +	u32 is_break:1;
> > +	u32 is_e0:1;
> > +	u32 is_e1:1;
> > +	u32 reserved:28;
> > +};
> > +
> > +
> 
> Extra blank line.
> 
> > +#define HK_MAXIMUM_MESSAGE_SIZE 256
> > +
> > +#define KBD_VSC_SEND_RING_BUFFER_SIZE		(10 * PAGE_SIZE)
> > +#define KBD_VSC_RECV_RING_BUFFER_SIZE		(10 * PAGE_SIZE)
> > +
> > +#define XTKBD_EMUL0     0xe0
> > +#define XTKBD_EMUL1     0xe1
> > +#define XTKBD_RELEASE   0x80
> > +
> > +
> 
> Extra blank.
> 
> > +/*
> > + * Represents a keyboard device
> > + */
> > +struct hv_kbd_dev {
> > +	unsigned char keycode[256];
> > +	struct hv_device	*device;
> > +	struct synth_kbd_protocol_request protocol_req;
> > +	struct synth_kbd_protocol_response protocol_resp;
> > +	/* Synchronize the request/response if needed */
> > +	struct completion	wait_event;
> > +	struct serio		*hv_serio;
> > +};
> > +
> > +
> 
> Extra blank.
> 
> > +static struct hv_kbd_dev *hv_kbd_alloc_device(struct hv_device *device)
> > +{
> > +	struct hv_kbd_dev *kbd_dev;
> > +	struct serio	*hv_serio;
> > +
> > +	kbd_dev = kzalloc(sizeof(struct hv_kbd_dev), GFP_KERNEL);
> > +
> 
> Spurious blank line.
> 
> > +	if (!kbd_dev)
> > +		return NULL;
> > +
> > +	hv_serio = kzalloc(sizeof(struct serio), GFP_KERNEL);
> > +
> 
> Spurious blank.
> 
> > +	if (hv_serio == NULL) {
> > +		kfree(kbd_dev);
> > +		return NULL;
> > +	}
> > +
> > +	hv_serio->id.type	= SERIO_8042_XL;
> 
> Pointless tab before the '='.
> 
> > +	strlcpy(hv_serio->name, dev_name(&device->device),
> > +		sizeof(hv_serio->name));
> > +	strlcpy(hv_serio->phys, dev_name(&device->device),
> > +		sizeof(hv_serio->phys));
> > +	hv_serio->dev.parent  = &device->device;
> 
> Why are there two spaces before the '='?
> 
> > +
> > +
> 
> Extra blank line.
> 
> > +	kbd_dev->device = device;
> > +	kbd_dev->hv_serio = hv_serio;
> > +	hv_set_drvdata(device, kbd_dev);
> > +	init_completion(&kbd_dev->wait_event);
> > +
> > +	return kbd_dev;
> > +}
> > +
> > +static void hv_kbd_free_device(struct hv_kbd_dev *device)
> > +{
> > +	serio_unregister_port(device->hv_serio);
> > +	kfree(device->hv_serio);
> > +	hv_set_drvdata(device->device, NULL);
> > +	kfree(device);
> > +}
> > +
> > +static void hv_kbd_on_receive(struct hv_device *device,
> > +				struct vmpacket_descriptor *packet)
> > +{
> > +	struct synth_kbd_msg *msg;
> > +	struct hv_kbd_dev *kbd_dev = hv_get_drvdata(device);
> > +	struct synth_kbd_keystroke *ks_msg;
> > +	u16 scan_code;
> > +
> > +	msg = (struct synth_kbd_msg *)((unsigned long)packet +
> > +					(packet->offset8 << 3));
> > +
> > +	switch (msg->header.type) {
> > +	case SYNTH_KBD_PROTOCOL_RESPONSE:
> > +		memcpy(&kbd_dev->protocol_resp, msg,
> > +			sizeof(struct synth_kbd_protocol_response));
> > +		complete(&kbd_dev->wait_event);
> > +		break;
> > +	case SYNTH_KBD_EVENT:
> > +		ks_msg = (struct synth_kbd_keystroke *)msg;
> > +		scan_code = ks_msg->make_code;
> > +
> > +		/*
> > +		 * Inject the information through the serio interrupt.
> > +		 */
> > +		if (ks_msg->is_e0)
> > +			serio_interrupt(kbd_dev->hv_serio, XTKBD_EMUL0, 0);
> > +		serio_interrupt(kbd_dev->hv_serio,
> > +				scan_code | (ks_msg->is_break ?
> > +				XTKBD_RELEASE : 0),
> > +				0);
> > +
> 
> It would be more readable to do:
> 
> 		if (ks_msg->is_break)
> 			scan_code |= XTKBD_RELEASE;
> 		serio_interrupt(kbd_dev->hv_serio, scan_code, 0);
> 
> 
> > +		break;
> > +
> > +	default:
> > +		pr_err("unhandled message type %d\n", msg->header.type);
> 
> Just a question.  This can only be triggered by the hyperviser, right?

Yes.

> 
> > +	}
> > +}
> > +
> > +static void hv_kbd_on_channel_callback(void *context)
> > +{
> > +	const int packet_size = 0x100;
> > +	int ret;
> > +	struct hv_device *device = context;
> > +	u32 bytes_recvd;
> > +	u64 req_id;
> > +	struct vmpacket_descriptor *desc;
> > +	unsigned char	*buffer;
> > +	int	bufferlen = packet_size;
> > +
> > +	buffer = kmalloc(bufferlen, GFP_ATOMIC);
> > +	if (!buffer)
> > +		return;
> > +
> > +	do {
> 
> 
> Forever loops should be while (1) { instead of do { } while (1).
> 
> 
> > +		ret = vmbus_recvpacket_raw(device->channel, buffer,
> > +					bufferlen, &bytes_recvd, &req_id);
> > +
> > +		switch (ret) {
> > +		case 0:
> > +			if (bytes_recvd <= 0) {
> 
> There can never be a negative number of bytes_recvd.
> 
> > +				kfree(buffer);
> > +				return;
> > +			}
> > +			desc = (struct vmpacket_descriptor *)buffer;
> > +
> > +			switch (desc->type) {
> > +			case VM_PKT_COMP:
> > +				break;
> > +
> > +			case VM_PKT_DATA_INBAND:
> > +				hv_kbd_on_receive(device, desc);
> 
> This is the error handling I mentioned at the top.  hv_kbd_on_receive()
> doesn't take into consideration the amount of data we recieved, it
> trusts the offset we recieved from the user.  There is an out of bounds
> read.

What user are you referring to. The message is sent by the host - the user keystroke
is normalized into a fixed size packet by the host and sent to the  guest. We will parse this
packet, based on the host specified layout here.

> 
> > +				break;
> > +
> > +			default:
> > +				pr_err("unhandled packet type %d, tid %llx len
> %d\n",
> > +					desc->type, req_id, bytes_recvd);
> > +				break;
> > +			}
> > +
> > +			break;
> > +
> > +		case -ENOBUFS:
> > +			kfree(buffer);
> > +			/* Handle large packet */
> > +			bufferlen = bytes_recvd;
> > +			buffer = kmalloc(bytes_recvd, GFP_ATOMIC);
> > +
> > +			if (!buffer)
> > +				return;
> > +
> > +			break;
> > +		}
> > +	} while (1);
> > +
> > +}
> > +
> > +static int hv_kbd_connect_to_vsp(struct hv_device *device)
> > +{
> > +	int ret = 0;
> 
> Don't initialize this.
> 
> > +	int t;
> > +	struct hv_kbd_dev *kbd_dev = hv_get_drvdata(device);
> > +	struct synth_kbd_protocol_request *request;
> > +	struct synth_kbd_protocol_response *response;
> > +
> > +	request = &kbd_dev->protocol_req;
> > +	memset(request, 0, sizeof(struct synth_kbd_protocol_request));
> > +
> > +	request->header.type = SYNTH_KBD_PROTOCOL_REQUEST;
> > +	request->version_requested.version = SYNTH_KBD_VERSION;
> > +
> > +	ret = vmbus_sendpacket(device->channel, request,
> > +				sizeof(struct synth_kbd_protocol_request),
> > +				(unsigned long)request,
> > +				VM_PKT_DATA_INBAND,
> > +
> 	VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
> > +	if (ret)
> > +		goto cleanup;
> 
> There is no cleanup.  Just return directly.
> 
> > +
> > +	t = wait_for_completion_timeout(&kbd_dev->wait_event, 10 * HZ);
> > +	if (!t) {
> > +		ret = -ETIMEDOUT;
> > +		goto cleanup;
> 
> 	return -ETIMEOUT;
> 
> > +	}
> > +
> > +	response = &kbd_dev->protocol_resp;
> > +
> > +	if (!response->accepted) {
> > +		pr_err("synth_kbd protocol request failed (version %d)\n",
> > +		       SYNTH_KBD_VERSION);
> > +		ret = -ENODEV;
> > +		goto cleanup;
> 
> Just return -ENODEV;
> 
> > +	}
> > +
> 
> 	return 0;
> 
> > +
> > +cleanup:
> > +	return ret;
> > +}
> > +
> > +static int hv_kbd_probe(struct hv_device *device,
> > +			const struct hv_vmbus_device_id *dev_id)
> > +{
> > +	int ret;
> > +	struct hv_kbd_dev *kbd_dev;
> > +
> > +	kbd_dev = hv_kbd_alloc_device(device);
> > +
> 
> Delete the blank line.
> 
> > +	if (!kbd_dev)
> > +		return -ENOMEM;
> > +
> > +	ret = vmbus_open(device->channel,
> > +		KBD_VSC_SEND_RING_BUFFER_SIZE,
> > +		KBD_VSC_RECV_RING_BUFFER_SIZE,
> > +		NULL,
> > +		0,
> > +		hv_kbd_on_channel_callback,
> > +		device
> > +		);
> > +
> 
> Delete the blank line.
> 
> > +	if (ret)
> > +		goto probe_err0;
> > +
> > +	ret = hv_kbd_connect_to_vsp(device);
> > +
> 
> Delete the blank line.
> 
> > +	if (ret)
> > +		goto probe_err1;
> > +
> > +	serio_register_port(kbd_dev->hv_serio);
> > +
> > +	return ret;
> 
> 	return 0;
> 
> > +
> > +probe_err1:
> > +	vmbus_close(device->channel);
> 
> The label here should be "err_close:".  The number is more GW-Basic
> style than C.
> 
> > +
> > +probe_err0:
> 
> The label should be "err_free_dev:".
> 
> > +	hv_kbd_free_device(kbd_dev);
> > +
> > +	return ret;
> > +}
> > +
> > +
> 
> Extra blank line.
> 
> > +static int hv_kbd_remove(struct hv_device *dev)
> 
> regards,
> dan carpenter

Regards,

K. Y

Re: [PATCH 1/1] Drivers: input: serio: New driver to support Hyper-V synthetic keyboard

[Dan Carpenter]

On Mon, Sep 16, 2013 at 02:46:24PM +0000, KY Srinivasan wrote:
> > > +			case VM_PKT_DATA_INBAND:
> > > +				hv_kbd_on_receive(device, desc);
> > 
> > This is the error handling I mentioned at the top.  hv_kbd_on_receive()
> > doesn't take into consideration the amount of data we recieved, it
> > trusts the offset we recieved from the user.  There is an out of bounds
> > read.
> 
> What user are you referring to. The message is sent by the host - the user keystroke
> is normalized into a fixed size packet by the host and sent to the  guest. We will parse this
> packet, based on the host specified layout here.
> 

The user means the hypervisor, yes.

I don't want the hypervisor accessing outside of the buffer.  It is
robustness issue.  Just check the offset against "bytes_recvd".  It's
not complicated.

If you have a different place where the guest does this then tell me
which function to look at.

regards,
dan carpenter

Re: [PATCH 1/1] Drivers: input: serio: New driver to support Hyper-V synthetic keyboard

[Dmitry Torokhov]

Hi K. Y.

On Sun, Sep 15, 2013 at 10:28:54PM -0700, K. Y. Srinivasan wrote:
> Add a new driver to support synthetic keyboard. On the next generation
> Hyper-V guest firmware, many legacy devices will not be emulated and this
> driver will be required.
> 
> I would like to thank Vojtech Pavlik <vojtech@suse.cz> for helping me with the
> details of the AT keyboard driver. 
> 

In addition to what Dan said:

> +
> +struct synth_kbd_protocol_response {
> +	struct synth_kbd_msg_hdr header;
> +	u32 accepted:1;
> +	u32 reserved:31;
> +};

Use of bitfields for on the wire structures makes me uneasy. I know that
currently you only going to run LE on LE, but still, maybe using
explicit shifts and masks would be better,

> +
> +struct synth_kbd_keystroke {
> +	struct synth_kbd_msg_hdr header;
> +	u16 make_code;
> +	u16 reserved0;
> +	u32 is_unicode:1;
> +	u32 is_break:1;
> +	u32 is_e0:1;
> +	u32 is_e1:1;
> +	u32 reserved:28;
> +};

Same here.

> +
> +
> +#define HK_MAXIMUM_MESSAGE_SIZE 256
> +
> +#define KBD_VSC_SEND_RING_BUFFER_SIZE		(10 * PAGE_SIZE)
> +#define KBD_VSC_RECV_RING_BUFFER_SIZE		(10 * PAGE_SIZE)
> +
> +#define XTKBD_EMUL0     0xe0
> +#define XTKBD_EMUL1     0xe1
> +#define XTKBD_RELEASE   0x80
> +
> +
> +/*
> + * Represents a keyboard device
> + */
> +struct hv_kbd_dev {
> +	unsigned char keycode[256];

I do not see anything using keycode table? This is wrong level for it
regardless.

> +	struct hv_device	*device;
> +	struct synth_kbd_protocol_request protocol_req;
> +	struct synth_kbd_protocol_response protocol_resp;
> +	/* Synchronize the request/response if needed */
> +	struct completion	wait_event;
> +	struct serio		*hv_serio;
> +};
> +
> +
> +static struct hv_kbd_dev *hv_kbd_alloc_device(struct hv_device *device)
> +{
> +	struct hv_kbd_dev *kbd_dev;
> +	struct serio	*hv_serio;

You are aligning with tabs half of declarations, leaving the others. Can
we not align at all?

> +
> +	kbd_dev = kzalloc(sizeof(struct hv_kbd_dev), GFP_KERNEL);
> +
> +	if (!kbd_dev)
> +		return NULL;
> +
> +	hv_serio = kzalloc(sizeof(struct serio), GFP_KERNEL);
> +
> +	if (hv_serio == NULL) {
> +		kfree(kbd_dev);
> +		return NULL;
> +	}
> +
> +	hv_serio->id.type	= SERIO_8042_XL;
> +	strlcpy(hv_serio->name, dev_name(&device->device),
> +		sizeof(hv_serio->name));
> +	strlcpy(hv_serio->phys, dev_name(&device->device),
> +		sizeof(hv_serio->phys));
> +	hv_serio->dev.parent  = &device->device;

Do you also want to set up serio's parent device to point to hv device?

> +
> +
> +	kbd_dev->device = device;
> +	kbd_dev->hv_serio = hv_serio;
> +	hv_set_drvdata(device, kbd_dev);
> +	init_completion(&kbd_dev->wait_event);
> +
> +	return kbd_dev;
> +}
> +
> +static void hv_kbd_free_device(struct hv_kbd_dev *device)
> +{
> +	serio_unregister_port(device->hv_serio);
> +	kfree(device->hv_serio);

Serio ports are refcounted, do not free them explicitly after they have
been registered.

> +	hv_set_drvdata(device->device, NULL);
> +	kfree(device);
> +}


Thanks.

-- 
Dmitry

RE: [PATCH 1/1] Drivers: input: serio: New driver to support Hyper-V synthetic keyboard

[KY Srinivasan]

> -----Original Message-----
> From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com]
> Sent: Monday, September 16, 2013 8:20 AM
> To: KY Srinivasan
> Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; linux-input@vger.kernel.org; vojtech@suse.cz;
> olaf@aepfle.de; apw@canonical.com; jasowang@redhat.com
> Subject: Re: [PATCH 1/1] Drivers: input: serio: New driver to support Hyper-V
> synthetic keyboard
> 
> Hi K. Y.
> 
> On Sun, Sep 15, 2013 at 10:28:54PM -0700, K. Y. Srinivasan wrote:
> > Add a new driver to support synthetic keyboard. On the next generation
> > Hyper-V guest firmware, many legacy devices will not be emulated and this
> > driver will be required.
> >
> > I would like to thank Vojtech Pavlik <vojtech@suse.cz> for helping me with the
> > details of the AT keyboard driver.
> >
> 
> In addition to what Dan said:
> 
> > +
> > +struct synth_kbd_protocol_response {
> > +	struct synth_kbd_msg_hdr header;
> > +	u32 accepted:1;
> > +	u32 reserved:31;
> > +};
> 
> Use of bitfields for on the wire structures makes me uneasy. I know that
> currently you only going to run LE on LE, but still, maybe using
> explicit shifts and masks would be better,

This definition of the data structure is defined by the host. I will see what I
can do here.

> 
> > +
> > +struct synth_kbd_keystroke {
> > +	struct synth_kbd_msg_hdr header;
> > +	u16 make_code;
> > +	u16 reserved0;
> > +	u32 is_unicode:1;
> > +	u32 is_break:1;
> > +	u32 is_e0:1;
> > +	u32 is_e1:1;
> > +	u32 reserved:28;
> > +};
> 
> Same here.
> 
> > +
> > +
> > +#define HK_MAXIMUM_MESSAGE_SIZE 256
> > +
> > +#define KBD_VSC_SEND_RING_BUFFER_SIZE		(10 * PAGE_SIZE)
> > +#define KBD_VSC_RECV_RING_BUFFER_SIZE		(10 * PAGE_SIZE)
> > +
> > +#define XTKBD_EMUL0     0xe0
> > +#define XTKBD_EMUL1     0xe1
> > +#define XTKBD_RELEASE   0x80
> > +
> > +
> > +/*
> > + * Represents a keyboard device
> > + */
> > +struct hv_kbd_dev {
> > +	unsigned char keycode[256];
> 
> I do not see anything using keycode table? This is wrong level for it
> regardless.
> 
> > +	struct hv_device	*device;
> > +	struct synth_kbd_protocol_request protocol_req;
> > +	struct synth_kbd_protocol_response protocol_resp;
> > +	/* Synchronize the request/response if needed */
> > +	struct completion	wait_event;
> > +	struct serio		*hv_serio;
> > +};
> > +
> > +
> > +static struct hv_kbd_dev *hv_kbd_alloc_device(struct hv_device *device)
> > +{
> > +	struct hv_kbd_dev *kbd_dev;
> > +	struct serio	*hv_serio;
> 
> You are aligning with tabs half of declarations, leaving the others. Can
> we not align at all?
> 
> > +
> > +	kbd_dev = kzalloc(sizeof(struct hv_kbd_dev), GFP_KERNEL);
> > +
> > +	if (!kbd_dev)
> > +		return NULL;
> > +
> > +	hv_serio = kzalloc(sizeof(struct serio), GFP_KERNEL);
> > +
> > +	if (hv_serio == NULL) {
> > +		kfree(kbd_dev);
> > +		return NULL;
> > +	}
> > +
> > +	hv_serio->id.type	= SERIO_8042_XL;
> > +	strlcpy(hv_serio->name, dev_name(&device->device),
> > +		sizeof(hv_serio->name));
> > +	strlcpy(hv_serio->phys, dev_name(&device->device),
> > +		sizeof(hv_serio->phys));
> > +	hv_serio->dev.parent  = &device->device;
> 
> Do you also want to set up serio's parent device to point to hv device?

Is there an issue here; I could setup the parent as the vmbus device.

> 
> > +
> > +
> > +	kbd_dev->device = device;
> > +	kbd_dev->hv_serio = hv_serio;
> > +	hv_set_drvdata(device, kbd_dev);
> > +	init_completion(&kbd_dev->wait_event);
> > +
> > +	return kbd_dev;
> > +}
> > +
> > +static void hv_kbd_free_device(struct hv_kbd_dev *device)
> > +{
> > +	serio_unregister_port(device->hv_serio);
> > +	kfree(device->hv_serio);
> 
> Serio ports are refcounted, do not free them explicitly after they have
> been registered.

Thank you. I will fix this.

> 
> > +	hv_set_drvdata(device->device, NULL);
> > +	kfree(device);
> > +}
> 
> 
> Thanks.


Thank you!.

Regards,

K. Y

RE: [PATCH 1/1] Drivers: input: serio: New driver to support Hyper-V synthetic keyboard

[KY Srinivasan]

> -----Original Message-----
> From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
> Sent: Monday, September 16, 2013 8:06 AM
> To: KY Srinivasan
> Cc: olaf@aepfle.de; gregkh@linuxfoundation.org; jasowang@redhat.com;
> dmitry.torokhov@gmail.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
> 
> On Mon, Sep 16, 2013 at 02:46:24PM +0000, KY Srinivasan wrote:
> > > > +			case VM_PKT_DATA_INBAND:
> > > > +				hv_kbd_on_receive(device, desc);
> > >
> > > This is the error handling I mentioned at the top.  hv_kbd_on_receive()
> > > doesn't take into consideration the amount of data we recieved, it
> > > trusts the offset we recieved from the user.  There is an out of bounds
> > > read.
> >
> > What user are you referring to. The message is sent by the host - the user
> keystroke
> > is normalized into a fixed size packet by the host and sent to the  guest. We will
> parse this
> > packet, based on the host specified layout here.
> >
> 
> The user means the hypervisor, yes.
> 
> I don't want the hypervisor accessing outside of the buffer.  It is
> robustness issue.  Just check the offset against "bytes_recvd".  It's
> not complicated.

At the outset, let me apologize for not understanding your concern.
You say: " I don't want the hypervisor accessing outside of the buffer"
Where did you see the hypervisor accessing anything outside the buffer?
The buffer is allocated by this driver and a packet from vmbus is read into this
buffer - this is the call to vmbus_recvpacket(). If the specified buffer is smaller
than the packet that needs to be read, then nothing will be read. Once the read
completes, we can be sure we have read a valid packet and can proceed to parse it in
this driver.

I don't want to be argumentative, I am just not seeing the issue.

Regards,

K. Y
 

Re: [PATCH 1/1] Drivers: input: serio: New driver to support Hyper-V synthetic keyboard

[Dmitry Torokhov]

On Mon, Sep 16, 2013 at 04:56:03PM +0000, KY Srinivasan wrote:
> 
> 
> > -----Original Message-----
> > From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
> > Sent: Monday, September 16, 2013 8:06 AM
> > To: KY Srinivasan
> > Cc: olaf@aepfle.de; gregkh@linuxfoundation.org; jasowang@redhat.com;
> > dmitry.torokhov@gmail.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
> > 
> > On Mon, Sep 16, 2013 at 02:46:24PM +0000, KY Srinivasan wrote:
> > > > > +			case VM_PKT_DATA_INBAND:
> > > > > +				hv_kbd_on_receive(device, desc);
> > > >
> > > > This is the error handling I mentioned at the top.  hv_kbd_on_receive()
> > > > doesn't take into consideration the amount of data we recieved, it
> > > > trusts the offset we recieved from the user.  There is an out of bounds
> > > > read.
> > >
> > > What user are you referring to. The message is sent by the host - the user
> > keystroke
> > > is normalized into a fixed size packet by the host and sent to the  guest. We will
> > parse this
> > > packet, based on the host specified layout here.
> > >
> > 
> > The user means the hypervisor, yes.
> > 
> > I don't want the hypervisor accessing outside of the buffer.  It is
> > robustness issue.  Just check the offset against "bytes_recvd".  It's
> > not complicated.
> 
> At the outset, let me apologize for not understanding your concern.
> You say: " I don't want the hypervisor accessing outside of the buffer"
> Where did you see the hypervisor accessing anything outside the buffer?
> The buffer is allocated by this driver and a packet from vmbus is read into this
> buffer - this is the call to vmbus_recvpacket(). If the specified buffer is smaller
> than the packet that needs to be read, then nothing will be read. Once the read
> completes, we can be sure we have read a valid packet and can proceed to parse it in
> this driver.

The concern is that number of bytes received and contents of a packet
are not in sync. Imagine if we were told that 16 butes was received but
in the packet offset is 78. Then we'll try reading well past the buffer
boundary that we allocated for the packets.

Thanks.

-- 
Dmitry

Re: [PATCH 1/1] Drivers: input: serio: New driver to support Hyper-V synthetic keyboard

[Dmitry Torokhov]

On Mon, Sep 16, 2013 at 03:52:18PM +0000, KY Srinivasan wrote:
> 
> 
> > -----Original Message-----
> > From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com]
> > Sent: Monday, September 16, 2013 8:20 AM
> > To: KY Srinivasan
> > Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> > devel@linuxdriverproject.org; linux-input@vger.kernel.org; vojtech@suse.cz;
> > olaf@aepfle.de; apw@canonical.com; jasowang@redhat.com
> > Subject: Re: [PATCH 1/1] Drivers: input: serio: New driver to support Hyper-V
> > synthetic keyboard
> > 
> > Hi K. Y.
> > 
> > On Sun, Sep 15, 2013 at 10:28:54PM -0700, K. Y. Srinivasan wrote:
> > > Add a new driver to support synthetic keyboard. On the next generation
> > > Hyper-V guest firmware, many legacy devices will not be emulated and this
> > > driver will be required.
> > >
> > > I would like to thank Vojtech Pavlik <vojtech@suse.cz> for helping me with the
> > > details of the AT keyboard driver.
> > >
> > 
> > In addition to what Dan said:
> > 
> > > +
> > > +struct synth_kbd_protocol_response {
> > > +	struct synth_kbd_msg_hdr header;
> > > +	u32 accepted:1;
> > > +	u32 reserved:31;
> > > +};
> > 
> > Use of bitfields for on the wire structures makes me uneasy. I know that
> > currently you only going to run LE on LE, but still, maybe using
> > explicit shifts and masks would be better,
> 
> This definition of the data structure is defined by the host. I will see what I
> can do here.

You do not really need to change protocol, you just sat that accepted is
the bit 0 of the word and define endianness (LE in your case). Then you
do:

	struct synth_kbd_protocol_response {
		struct synth_kbd_msg_hdr header;
		__le32 status;
	}

	#define KBD_PROTOCOL_ACCEPTED BIT(0)

	...

	status = _le32_to_cpu(response->status);
	accepted = status & KBD_PROTOCOL_ACCEPTED;

Thanks.

-- 
Dmitry

RE: [PATCH 1/1] Drivers: input: serio: New driver to support Hyper-V synthetic keyboard

[KY Srinivasan]

> -----Original Message-----
> From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com]
> Sent: Monday, September 16, 2013 10:10 AM
> To: KY Srinivasan
> Cc: Dan Carpenter; 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
> 
> On Mon, Sep 16, 2013 at 04:56:03PM +0000, KY Srinivasan wrote:
> >
> >
> > > -----Original Message-----
> > > From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
> > > Sent: Monday, September 16, 2013 8:06 AM
> > > To: KY Srinivasan
> > > Cc: olaf@aepfle.de; gregkh@linuxfoundation.org; jasowang@redhat.com;
> > > dmitry.torokhov@gmail.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
> > >
> > > On Mon, Sep 16, 2013 at 02:46:24PM +0000, KY Srinivasan wrote:
> > > > > > +			case VM_PKT_DATA_INBAND:
> > > > > > +				hv_kbd_on_receive(device, desc);
> > > > >
> > > > > This is the error handling I mentioned at the top.  hv_kbd_on_receive()
> > > > > doesn't take into consideration the amount of data we recieved, it
> > > > > trusts the offset we recieved from the user.  There is an out of bounds
> > > > > read.
> > > >
> > > > What user are you referring to. The message is sent by the host - the user
> > > keystroke
> > > > is normalized into a fixed size packet by the host and sent to the  guest. We
> will
> > > parse this
> > > > packet, based on the host specified layout here.
> > > >
> > >
> > > The user means the hypervisor, yes.
> > >
> > > I don't want the hypervisor accessing outside of the buffer.  It is
> > > robustness issue.  Just check the offset against "bytes_recvd".  It's
> > > not complicated.
> >
> > At the outset, let me apologize for not understanding your concern.
> > You say: " I don't want the hypervisor accessing outside of the buffer"
> > Where did you see the hypervisor accessing anything outside the buffer?
> > The buffer is allocated by this driver and a packet from vmbus is read into this
> > buffer - this is the call to vmbus_recvpacket(). If the specified buffer is smaller
> > than the packet that needs to be read, then nothing will be read. Once the read
> > completes, we can be sure we have read a valid packet and can proceed to
> parse it in
> > this driver.
> 
> The concern is that number of bytes received and contents of a packet
> are not in sync. Imagine if we were told that 16 butes was received but
> in the packet offset is 78. Then we'll try reading well past the buffer
> boundary that we allocated for the packets.

I am not sure how this would be the case. Following are the semantics of the function
vmbus_recvpacket_raw():

If the packet to be read is larger than the buffer specified; nothing will be read and 
appropriate error is returned. If a  packet is read, the complete packet is read and 
so we can safely peek into this packet based on the information in the header.

Regards,


K. Y

Re: [PATCH 1/1] Drivers: input: serio: New driver to support Hyper-V synthetic keyboard

[Dan Carpenter]

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:

RE: [PATCH 1/1] Drivers: input: serio: New driver to support Hyper-V synthetic keyboard

[KY Srinivasan]

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

Re: [PATCH 1/1] Drivers: input: serio: New driver to support Hyper-V synthetic keyboard

[Dan Carpenter]

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

RE: [PATCH 1/1] Drivers: input: serio: New driver to support Hyper-V synthetic keyboard

[KY Srinivasan]

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

Re: [PATCH 1/1] Drivers: input: serio: New driver to support Hyper-V synthetic keyboard

[Dan Carpenter]

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

Re: [PATCH 1/1] Drivers: input: serio: New driver to support Hyper-V synthetic keyboard

[Dmitry Torokhov]

On Mon, Sep 16, 2013 at 06:29:45PM +0000, KY Srinivasan wrote:
> 
> 
> > -----Original Message-----
> > From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com]
> > Sent: Monday, September 16, 2013 10:10 AM
> > To: KY Srinivasan
> > Cc: Dan Carpenter; 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
> > 
> > On Mon, Sep 16, 2013 at 04:56:03PM +0000, KY Srinivasan wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
> > > > Sent: Monday, September 16, 2013 8:06 AM
> > > > To: KY Srinivasan
> > > > Cc: olaf@aepfle.de; gregkh@linuxfoundation.org; jasowang@redhat.com;
> > > > dmitry.torokhov@gmail.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
> > > >
> > > > On Mon, Sep 16, 2013 at 02:46:24PM +0000, KY Srinivasan wrote:
> > > > > > > +			case VM_PKT_DATA_INBAND:
> > > > > > > +				hv_kbd_on_receive(device, desc);
> > > > > >
> > > > > > This is the error handling I mentioned at the top.  hv_kbd_on_receive()
> > > > > > doesn't take into consideration the amount of data we recieved, it
> > > > > > trusts the offset we recieved from the user.  There is an out of bounds
> > > > > > read.
> > > > >
> > > > > What user are you referring to. The message is sent by the host - the user
> > > > keystroke
> > > > > is normalized into a fixed size packet by the host and sent to the  guest. We
> > will
> > > > parse this
> > > > > packet, based on the host specified layout here.
> > > > >
> > > >
> > > > The user means the hypervisor, yes.
> > > >
> > > > I don't want the hypervisor accessing outside of the buffer.  It is
> > > > robustness issue.  Just check the offset against "bytes_recvd".  It's
> > > > not complicated.
> > >
> > > At the outset, let me apologize for not understanding your concern.
> > > You say: " I don't want the hypervisor accessing outside of the buffer"
> > > Where did you see the hypervisor accessing anything outside the buffer?
> > > The buffer is allocated by this driver and a packet from vmbus is read into this
> > > buffer - this is the call to vmbus_recvpacket(). If the specified buffer is smaller
> > > than the packet that needs to be read, then nothing will be read. Once the read
> > > completes, we can be sure we have read a valid packet and can proceed to
> > parse it in
> > > this driver.
> > 
> > The concern is that number of bytes received and contents of a packet
> > are not in sync. Imagine if we were told that 16 butes was received but
> > in the packet offset is 78. Then we'll try reading well past the buffer
> > boundary that we allocated for the packets.
> 
> I am not sure how this would be the case. Following are the semantics of the function
> vmbus_recvpacket_raw():
> 
> If the packet to be read is larger than the buffer specified; nothing will be read and 
> appropriate error is returned. If a  packet is read, the complete packet is read and 
> so we can safely peek into this packet based on the information in the header.

No, you can not safely use contents of the packet because it has not
been vetted. The semantics you are talking about is provided by
vmbus_recvpacket(). That function does indeed look inside the packet end
ensures that offset specified in the packet is sane and would not exceed
the buffer. The vmbus_recvpacket_raw() does not do such validation.

Thanks.

-- 
Dmitry