* [PATCH 1/3] tools: hv: fcopy_daemon: Check buffer limits
@ 2014-10-21 11:49 Matej Mužila
2014-10-21 12:13 ` One Thousand Gnomes
0 siblings, 1 reply; 6+ messages in thread
From: Matej Mužila @ 2014-10-21 11:49 UTC (permalink / raw)
To: kys, devel, linux-kernel
From: Matej Mužila <mmuzila@redhat.com>
Check if cpmsg->size is in limits of DATA_FRAGMENT
Signed-off-by: Matej Mužila <mmuzila@redhat.com>
Acked-by: K. Y. Srinivasan <kys@microsoft.com>
---
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);
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 1/3] tools: hv: fcopy_daemon: Check buffer limits
2014-10-21 11:49 [PATCH 1/3] tools: hv: fcopy_daemon: Check buffer limits Matej Mužila
@ 2014-10-21 12:13 ` One Thousand Gnomes
2014-10-21 12:59 ` Matej Mužila
0 siblings, 1 reply; 6+ messages in thread
From: One Thousand Gnomes @ 2014-10-21 12:13 UTC (permalink / raw)
To: Matej Mužila; +Cc: kys, devel, linux-kernel
On Tue, 21 Oct 2014 13:49:00 +0200
Matej Mužila <mmuzila@redhat.com> wrote:
> From: Matej Mužila <mmuzila@redhat.com>
>
> Check if cpmsg->size is in limits of DATA_FRAGMENT
>
> Signed-off-by: Matej Mužila <mmuzila@redhat.com>
> Acked-by: K. Y. Srinivasan <kys@microsoft.com>
> ---
> 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;
> +
- 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...
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 1/3] tools: hv: fcopy_daemon: Check buffer limits
2014-10-21 12:13 ` One Thousand Gnomes
@ 2014-10-21 12:59 ` Matej Mužila
2014-10-21 14:17 ` Dan Carpenter
2014-10-21 14:46 ` [PATCH v2 " Matej Mužila
0 siblings, 2 replies; 6+ messages in thread
From: Matej Mužila @ 2014-10-21 12:59 UTC (permalink / raw)
To: One Thousand Gnomes; +Cc: kys, devel, linux-kernel
> 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
> - C style comments for coding style
Fixed
> 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 <mmuzila@redhat.com>
Check if cpmsg->size is in limits of DATA_FRAGMENT
Signed-off-by: Matej Mužila <mmuzila@redhat.com>
---
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.
---
diff --git a/tools/hv/hv_fcopy_daemon.c b/tools/hv/hv_fcopy_daemon.c
index 6f27e2f..1fc2dc2 100644
--- a/tools/hv/hv_fcopy_daemon.c
+++ b/tools/hv/hv_fcopy_daemon.c
@@ -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);
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH 1/3] tools: hv: fcopy_daemon: Check buffer limits
2014-10-21 12:59 ` Matej Mužila
@ 2014-10-21 14:17 ` Dan Carpenter
2014-10-21 14:46 ` [PATCH v2 " Matej Mužila
1 sibling, 0 replies; 6+ messages in thread
From: Dan Carpenter @ 2014-10-21 14:17 UTC (permalink / raw)
To: Matej Mužila; +Cc: One Thousand Gnomes, devel, linux-kernel
On Tue, Oct 21, 2014 at 02:59:58PM +0200, Matej Mužila wrote:
> > 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
>
> > - C style comments for coding style
> Fixed
>
> > 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...
>
> 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
> ---
> From: Matej Mužila <mmuzila@redhat.com>
>
> Check if cpmsg->size is in limits of DATA_FRAGMENT
>
> Signed-off-by: Matej Mužila <mmuzila@redhat.com>
> ---
> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2 1/3] tools: hv: fcopy_daemon: Check buffer limits
2014-10-21 12:59 ` Matej Mužila
2014-10-21 14:17 ` Dan Carpenter
@ 2014-10-21 14:46 ` Matej Mužila
2014-11-07 18:19 ` Greg KH
1 sibling, 1 reply; 6+ messages in thread
From: Matej Mužila @ 2014-10-21 14:46 UTC (permalink / raw)
To: devel, linux-kernel, kys; +Cc: dan.carpenter, One Thousand Gnomes
From: Matej Mužila <mmuzila@redhat.com>
Check if cpmsg->size is in limits of DATA_FRAGMENT
Signed-off-by: Matej Mužila <mmuzila@redhat.com>
---
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
diff --git a/tools/hv/hv_fcopy_daemon.c b/tools/hv/hv_fcopy_daemon.c
index 6f27e2f..1fc2dc2 100644
--- a/tools/hv/hv_fcopy_daemon.c
+++ b/tools/hv/hv_fcopy_daemon.c
@@ -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);
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH v2 1/3] tools: hv: fcopy_daemon: Check buffer limits
2014-10-21 14:46 ` [PATCH v2 " Matej Mužila
@ 2014-11-07 18:19 ` Greg KH
0 siblings, 0 replies; 6+ messages in thread
From: Greg KH @ 2014-11-07 18:19 UTC (permalink / raw)
To: Matej Mužila
Cc: devel, linux-kernel, kys, One Thousand Gnomes, dan.carpenter
On Tue, Oct 21, 2014 at 04:46:58PM +0200, Matej Mužila wrote:
> From: Matej Mužila <mmuzila@redhat.com>
>
> Check if cpmsg->size is in limits of DATA_FRAGMENT
>
> Signed-off-by: Matej Mužila <mmuzila@redhat.com>
> ---
>
> 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
>
>
> diff --git a/tools/hv/hv_fcopy_daemon.c b/tools/hv/hv_fcopy_daemon.c
> index 6f27e2f..1fc2dc2 100644
> --- a/tools/hv/hv_fcopy_daemon.c
> +++ b/tools/hv/hv_fcopy_daemon.c
> @@ -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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-11-07 18:19 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-10-21 11:49 [PATCH 1/3] tools: hv: fcopy_daemon: Check buffer limits Matej Mužila
2014-10-21 12:13 ` One Thousand Gnomes
2014-10-21 12:59 ` Matej Mužila
2014-10-21 14:17 ` Dan Carpenter
2014-10-21 14:46 ` [PATCH v2 " Matej Mužila
2014-11-07 18:19 ` Greg KH
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox