Skip to contentSkip to conversation

tools: hv: fcopy_daemon: Check buffer limits

PATCH Superseded Matej Mužila 4 participants 6 messages Oct 21, 2014 – Nov 7, 2014 Hyper-V/Azure core and drivers

lore
Share & tools
lore

Matej Mužila

author
#0 lore raw

PATCH 1/3 tools: hv: fcopy_daemon: Check buffer limits

From: Matej Mužila <[email protected]>

Check if cpmsg->size is in limits of DATA_FRAGMENT

Signed-off-by Matej Mužila <[email protected]>
Acked-by K. Y. Srinivasan <[email protected]>

---
If corrupted data are read from /dev/vmbus/hv_fcopy, pwrite can read from memory outside of the buffer (defined at line 138). Added check.
---
@@ -104,6 +104,10 @@ static int hv_copy_data(struct hv_do_fcopy *cpmsg)
{
ssize_t bytes_written;

+ // Check if the cpmsg->size is in limits of DATA_FRAGMENT
+ if (cpmsg->size > DATA_FRAGMENT * sizeof(__u8))
+ return HV_E_FAIL;
+
bytes_written = pwrite(target_fd, cpmsg->data, cpmsg->size,
cpmsg->offset);

#1 lore raw
Quoted 12 lines · On Tue, 21 Oct 2014 13:49:00 +0200 Matej Mužila <[email protected]> wrote:

From: Matej Mužila <[email protected]>

Check if cpmsg->size is in limits of DATA_FRAGMENT

Signed-off-by Matej Mužila <[email protected]>
Acked-by K. Y. Srinivasan <[email protected]>

---
If corrupted data are read from /dev/vmbus/hv_fcopy, pwrite can read from memory outside of the buffer (defined at line 138). Added check.
---

Review static int hv_copy_data(struct hv_do_fcopy *cpmsg) · line 110

Quoted 7 lines
(quoted diff) +4 -0
@@ -104,6 +104,10 @@ static int hv_copy_data(struct hv_do_fcopy *cpmsg)
 {
 	ssize_t bytes_written;
 
+	// Check if the cpmsg->size is in limits of DATA_FRAGMENT
+	if (cpmsg->size > DATA_FRAGMENT * sizeof(__u8))
+		return HV_E_FAIL;
+

- C style comments for coding style, also sizeof(__u8) is by definition 1 so it's perhaps surplus ?

Also your patch block is devoid of a few thins like the file name...

author
#2 lore raw
Quoted 1 line

sizeof(__u8) is by definition 1 so it's perhaps surplus ?

Now the size is now determined from the structure definition in include/uapi/linux/hyperv.h

Quoted 1 line

- C style comments for coding style

Fixed

Quoted 1 line

Also your patch block is devoid of a few thins like the file name...

I'm sorry, the (missing) filename mistake occured in copy-paste process.

Here is the patch as it (I hope) should look like:
---
From: Matej Mužila <[email protected]>

Check if cpmsg->size is in limits of DATA_FRAGMENT

Signed-off-by Matej Mužila <[email protected]>

---
If corrupted data are read from /dev/vmbus/hv_fcopy, pwrite can read from memory outside of the buffer (defined at line 138). Added check.
---

tools/hv/hv_fcopy_daemon.c +4 -0
index 6f27e2f..1fc2dc2 100644
@@ -104,6 +104,10 @@ static int hv_copy_data(struct hv_do_fcopy *cpmsg)
 {
 	ssize_t bytes_written;
 
+	/* Check if the cpmsg->size is in limits of DATA_FRAGMENT */
+	if (cpmsg->size > sizeof(cpmsg->data)) 
+		return HV_E_FAIL;
+
 	bytes_written = pwrite(target_fd, cpmsg->data, cpmsg->size,
 				cpmsg->offset);
#3 lore raw
Quoted 10 lines · Matej Mužila
Quoted 1 line

sizeof(__u8) is by definition 1 so it's perhaps surplus ?

Now the size is now determined from the structure definition in include/uapi/linux/hyperv.h

Quoted 1 line

- C style comments for coding style

Fixed

Quoted 1 line

Also your patch block is devoid of a few thins like the file name...

I'm sorry, the (missing) filename mistake occured in copy-paste process.

Copy and paste is very error prone...

Quoted 2 lines

Here is the patch as it (I hope) should look like:

This patch looks good, but please resend it as a proper v2 patch.

https://www.google.com/search?q=how+to+send+a+v2+patch

Quoted 10 lines

---
From: Matej Mužila <[email protected]>

Check if cpmsg->size is in limits of DATA_FRAGMENT

Signed-off-by Matej Mužila <[email protected]>

---
If corrupted data are read from /dev/vmbus/hv_fcopy, pwrite can read from memory outside of the buffer (defined at line 138). Added check.

Put this information in the patch description and not beyond the cut off. That information is useful.

The cut off is meant for meta comentary to say what changed between v1 and v2 etc, which is nice to have but we don't want to preserve it.

regards,
dan carpenter

author
#4 lore raw

PATCH v2 1/3 tools: hv: fcopy_daemon: Check buffer limits

In reply to Matej Mužila

From: Matej Mužila <[email protected]>

Check if cpmsg->size is in limits of DATA_FRAGMENT

Signed-off-by Matej Mužila <[email protected]>

---

If corrupted data are read from /dev/vmbus/hv_fcopy, pwrite can read from memory outside of the buffer (defined at line 138). Added check.

Changes made since v1:
* max value of cmesg->size is now derived from structure definition in sources/include/uapi/linux/hyperv.h
* Fixed comments

tools/hv/hv_fcopy_daemon.c +4 -0
index 6f27e2f..1fc2dc2 100644
@@ -104,6 +104,10 @@ static int hv_copy_data(struct hv_do_fcopy *cpmsg)
 {
 	ssize_t bytes_written;
 
+	/* Check if the cpmsg->size is in limits of DATA_FRAGMENT */
+	if (cpmsg->size > sizeof(cpmsg->data)) 
+		return HV_E_FAIL;
+
 	bytes_written = pwrite(target_fd, cpmsg->data, cpmsg->size,
 				cpmsg->offset);
#5 lore raw
Quoted 22 lines · Matej Mužila

From: Matej Mužila <[email protected]>

Check if cpmsg->size is in limits of DATA_FRAGMENT

Signed-off-by Matej Mužila <[email protected]>

---

If corrupted data are read from /dev/vmbus/hv_fcopy, pwrite can read from memory outside of the buffer (defined at line 138). Added check.

Changes made since v1:
* max value of cmesg->size is now derived from structure definition in sources/include/uapi/linux/hyperv.h
* Fixed comments

tools/hv/hv_fcopy_daemon.c +0 -0
index 6f27e2f..1fc2dc2 100644

tools/hv/hv_fcopy_daemon.c static int hv_copy_data(struct hv_do_fcopy *cpmsg) · line 113

Quoted 10 lines
tools/hv/hv_fcopy_daemon.c +4 -0
@@ -104,6 +104,10 @@ static int hv_copy_data(struct hv_do_fcopy *cpmsg)
 {
 	ssize_t bytes_written;
 
+	/* Check if the cpmsg->size is in limits of DATA_FRAGMENT */
+	if (cpmsg->size > sizeof(cpmsg->data)) 
+		return HV_E_FAIL;
+
 	bytes_written = pwrite(target_fd, cpmsg->data, cpmsg->size,
 				cpmsg->offset);

ALWAYS run your patches through checkpatch before sending them, so you don't get grumpy emails from maintainers telling you to do the same thing...

Please fix this up and resend the whole series.

thanks,

greg k-h