[PATCH 5/6] staging: ozwpan: Increase farewell report size.

[PATCH 5/6] staging: ozwpan: Increase farewell report size.

[Rupesh Gujare]

Farewell report size can be bigger than one byte, increase array
size to accomodate maximum 32 bytes of farewell report.

Signed-off-by: Rupesh Gujare <rupesh.gujare@atmel.com>
---
 drivers/staging/ozwpan/ozpd.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/ozwpan/ozpd.h b/drivers/staging/ozwpan/ozpd.h
index a281753..57e98c8 100644
--- a/drivers/staging/ozwpan/ozpd.h
+++ b/drivers/staging/ozwpan/ozpd.h
@@ -48,7 +48,7 @@ struct oz_farewell {
 	struct list_head link;
 	u8 ep_num;
 	u8 index;
-	u8 report[1];
+	u8 report[32];
 	u8 len;
 };
 
-- 
1.7.9.5

[PATCH 6/6] staging: ozwpan: Set farewell report length.

[Rupesh Gujare]

Fixes a bug where we were not setting length field causing wrong
report size to be copied.

Signed-off-by: Rupesh Gujare <rupesh.gujare@atmel.com>
---
 drivers/staging/ozwpan/ozproto.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/staging/ozwpan/ozproto.c b/drivers/staging/ozwpan/ozproto.c
index ec60286..084307a 100644
--- a/drivers/staging/ozwpan/ozproto.c
+++ b/drivers/staging/ozwpan/ozproto.c
@@ -296,6 +296,7 @@ static void oz_add_farewell(struct oz_pd *pd, u8 ep_num, u8 index,
 		return;
 	f->ep_num = ep_num;
 	f->index = index;
+	f->len = len;
 	memcpy(f->report, report, len);
 	oz_dbg(ON, "RX: Adding farewell report\n");
 	spin_lock(&g_polling_lock);
-- 
1.7.9.5

Re: [PATCH 5/6] staging: ozwpan: Increase farewell report size.

[Dan Carpenter]

On Thu, Aug 01, 2013 at 06:45:01PM +0100, Rupesh Gujare wrote:
> Farewell report size can be bigger than one byte, increase array
> size to accomodate maximum 32 bytes of farewell report.
> 

Gar...  No.  This is not right.

1) There is no check limiting the size to 32 and it could be up to
   253 bytes.

2) Use defines instead of magic numbers.

3) The oz_farewell struct is supposed to be a variable length struct
   but the variable part is put in the middle.  It doesn't make any
   sense to put the length of the variable size array after then end
   of the array because we can never find it again!  Put the
   variable size array at the end.  Make it a zero length array.
   u8 len;
   u8 report[0];

4) In oz_add_farewell() we do this:

	f = kmalloc(sizeof(struct oz_farewell) + len - 1, GFP_ATOMIC);

    The "- 1" refers to sizeof(f->report) but because it was a magic
    number then it was missed when the sizeof(f->report) changed.

5) In [patch 6/6] we set the ->len member.  But because it is at the
   end of a variable length array with no limit check the remote
   attacker can just rewrite it using the memcpy() on the next line.

regards,
dan carpenter

Re: [PATCH 5/6] staging: ozwpan: Increase farewell report size.

[Rupesh Gujare]

On 02/08/13 11:27, Dan Carpenter wrote:
> On Thu, Aug 01, 2013 at 06:45:01PM +0100, Rupesh Gujare wrote:
>> Farewell report size can be bigger than one byte, increase array
>> size to accomodate maximum 32 bytes of farewell report.
>>
> Gar...  No.  This is not right.
>
> 1) There is no check limiting the size to 32 and it could be up to
>     253 bytes.
>
> 2) Use defines instead of magic numbers.
>
> 3) The oz_farewell struct is supposed to be a variable length struct
>     but the variable part is put in the middle.  It doesn't make any
>     sense to put the length of the variable size array after then end
>     of the array because we can never find it again!  Put the
>     variable size array at the end.  Make it a zero length array.
>     u8 len;
>     u8 report[0];
>
> 4) In oz_add_farewell() we do this:
>
> 	f = kmalloc(sizeof(struct oz_farewell) + len - 1, GFP_ATOMIC);
>
>      The "- 1" refers to sizeof(f->report) but because it was a magic
>      number then it was missed when the sizeof(f->report) changed.
>
> 5) In [patch 6/6] we set the ->len member.  But because it is at the
>     end of a variable length array with no limit check the remote
>     attacker can just rewrite it using the memcpy() on the next line.
>
>
Thanks Dan.

A patch follows to fix above issues.

-- 
Regards,
Rupesh Gujare

[PATCH] staging: ozwpan: Fix farewell report.

[Rupesh Gujare]

This patch fixes issues reported by Dan here:-
http://driverdev.linuxdriverproject.org/pipermail/driverdev-devel/2013-August/040052.html

Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Rupesh Gujare <rupesh.gujare@atmel.com>
---
 drivers/staging/ozwpan/ozpd.h    |    2 +-
 drivers/staging/ozwpan/ozproto.c |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/ozwpan/ozpd.h b/drivers/staging/ozwpan/ozpd.h
index 57e98c8..996ef65 100644
--- a/drivers/staging/ozwpan/ozpd.h
+++ b/drivers/staging/ozwpan/ozpd.h
@@ -48,8 +48,8 @@ struct oz_farewell {
 	struct list_head link;
 	u8 ep_num;
 	u8 index;
-	u8 report[32];
 	u8 len;
+	u8 report[0];
 };
 
 /* Data structure that holds information on a specific peripheral device (PD).
diff --git a/drivers/staging/ozwpan/ozproto.c b/drivers/staging/ozwpan/ozproto.c
index 084307a..3d1a89f 100644
--- a/drivers/staging/ozwpan/ozproto.c
+++ b/drivers/staging/ozwpan/ozproto.c
@@ -291,7 +291,7 @@ static void oz_add_farewell(struct oz_pd *pd, u8 ep_num, u8 index,
 	struct oz_farewell *f;
 	struct oz_farewell *f2;
 	int found = 0;
-	f = kmalloc(sizeof(struct oz_farewell) + len - 1, GFP_ATOMIC);
+	f = kmalloc(sizeof(struct oz_farewell) + len, GFP_ATOMIC);
 	if (!f)
 		return;
 	f->ep_num = ep_num;
-- 
1.7.9.5

Re: [PATCH] staging: ozwpan: Fix farewell report.

[Greg KH]

On Fri, Aug 02, 2013 at 12:04:16PM +0100, Rupesh Gujare wrote:
> This patch fixes issues reported by Dan here:-
> http://driverdev.linuxdriverproject.org/pipermail/driverdev-devel/2013-August/040052.html

Please list the issues in the patch changelog itself, as sometimes you
don't have web access, and I sure wouldn't rely on that email archive to
always be around (hint, it's been down a bunch recently...)

Can you please resend this with a better changelog description?

thanks,

greg k-h

[PATCH v2] staging: ozwpan: Fix farewell report.

[Rupesh Gujare]

This patch fix following issues reported by Dan:-

1) There is no check limiting the size to 32 and it could be up to
   253 bytes.
2) Use defines instead of magic numbers.
3) The oz_farewell struct is supposed to be a variable length struct
   but the variable part is put in the middle.  It doesn't make any
   sense to put the length of the variable size array after then end
   of the array because we can never find it again!  Put the
   variable size array at the end.  Make it a zero length array.
   u8 len;
   u8 report[0];
4) In oz_add_farewell() we do this:

	f = kmalloc(sizeof(struct oz_farewell) + len - 1, GFP_ATOMIC);

    The "- 1" refers to sizeof(f->report) but because it was a magic
    number then it was missed when the sizeof(f->report) changed.
5) In [patch 6/6] we set the ->len member.  But because it is at the
   end of a variable length array with no limit check the remote
   attacker can just rewrite it using the memcpy() on the next line.

Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Rupesh Gujare <rupesh.gujare@atmel.com>
---
 drivers/staging/ozwpan/ozpd.h    |    2 +-
 drivers/staging/ozwpan/ozproto.c |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/ozwpan/ozpd.h b/drivers/staging/ozwpan/ozpd.h
index 57e98c8..996ef65 100644
--- a/drivers/staging/ozwpan/ozpd.h
+++ b/drivers/staging/ozwpan/ozpd.h
@@ -48,8 +48,8 @@ struct oz_farewell {
 	struct list_head link;
 	u8 ep_num;
 	u8 index;
-	u8 report[32];
 	u8 len;
+	u8 report[0];
 };
 
 /* Data structure that holds information on a specific peripheral device (PD).
diff --git a/drivers/staging/ozwpan/ozproto.c b/drivers/staging/ozwpan/ozproto.c
index 084307a..3d1a89f 100644
--- a/drivers/staging/ozwpan/ozproto.c
+++ b/drivers/staging/ozwpan/ozproto.c
@@ -291,7 +291,7 @@ static void oz_add_farewell(struct oz_pd *pd, u8 ep_num, u8 index,
 	struct oz_farewell *f;
 	struct oz_farewell *f2;
 	int found = 0;
-	f = kmalloc(sizeof(struct oz_farewell) + len - 1, GFP_ATOMIC);
+	f = kmalloc(sizeof(struct oz_farewell) + len, GFP_ATOMIC);
 	if (!f)
 		return;
 	f->ep_num = ep_num;
-- 
1.7.9.5