Re: [PATCH] stm class: Fix out of bound access from bitmap allocation

2019-04-16 Thread Sai Prakash Ranjan 

On 4/16/2019 8:30 PM, Alexander Shishkin wrote:

Sai Prakash Ranjan  writes:


From: Mulu He 

Bitmap allocation works on array of unsigned longs and
for stm master allocation when the number of software
channels is 32, 4 bytes are allocated and there is a out of
bound access at the first 8 bytes access of bitmap region.


Does the below fix the problem for you?

 From fb22b9ab109b332e58d72df13563e270befbd0e3 Mon Sep 17 00:00:00 2001
From: Alexander Shishkin 
Date: Tue, 16 Apr 2019 17:47:02 +0300
Subject: [PATCH] stm class: Fix channel bitmap on 32-bit systems

Commit 7bd1d4093c2f ("stm class: Introduce an abstraction for System Trace
Module devices") naively calculates the channel bitmap size in 64-bit
chunks regardless of the size of underlying unsigned long, making the
bitmap half as big on a 32-bit system. This leads to an out of bounds
access with the upper half of the bitmap.

Fix this by using BITS_TO_LONGS. While at it, convert to using
struct_size() for the total size calculation of the master struct.

Signed-off-by: Alexander Shishkin 
Fixes: 7bd1d4093c2f ("stm class: Introduce an abstraction for System Trace Module 
devices")
Reported-by: Mulu He 
Cc: sta...@vger.kernel.org # v4.4+
---
  drivers/hwtracing/stm/core.c | 6 ++
  1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/hwtracing/stm/core.c b/drivers/hwtracing/stm/core.c
index 5b5807cbcf7c..8c45e79e47db 100644
--- a/drivers/hwtracing/stm/core.c
+++ b/drivers/hwtracing/stm/core.c
@@ -166,11 +166,9 @@ stm_master(struct stm_device *stm, unsigned int idx)
  static int stp_master_alloc(struct stm_device *stm, unsigned int idx)
  {
struct stp_master *master;
-   size_t size;
  
-	size = ALIGN(stm->data->sw_nchannels, 8) / 8;

-   size += sizeof(struct stp_master);
-   master = kzalloc(size, GFP_ATOMIC);
+   master = kzalloc(struct_size(master, chan_map, 
BITS_TO_LONGS(stm->data->sw_nchannels)),
+GFP_ATOMIC);
if (!master)
return -ENOMEM;
  



++ David

Yes it does fix the issue. Actually initial fix internally was using
BITS_TO_LONGS, don't no why they deferred from it.

Anyways thanks for the patch.

- Sai

--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH] stm class: Fix out of bound access from bitmap allocation

2019-04-16 Thread Alexander Shishkin 
Sai Prakash Ranjan  writes:

> From: Mulu He 
>
> Bitmap allocation works on array of unsigned longs and
> for stm master allocation when the number of software
> channels is 32, 4 bytes are allocated and there is a out of
> bound access at the first 8 bytes access of bitmap region.

Does the below fix the problem for you?

>From fb22b9ab109b332e58d72df13563e270befbd0e3 Mon Sep 17 00:00:00 2001
From: Alexander Shishkin 
Date: Tue, 16 Apr 2019 17:47:02 +0300
Subject: [PATCH] stm class: Fix channel bitmap on 32-bit systems

Commit 7bd1d4093c2f ("stm class: Introduce an abstraction for System Trace
Module devices") naively calculates the channel bitmap size in 64-bit
chunks regardless of the size of underlying unsigned long, making the
bitmap half as big on a 32-bit system. This leads to an out of bounds
access with the upper half of the bitmap.

Fix this by using BITS_TO_LONGS. While at it, convert to using
struct_size() for the total size calculation of the master struct.

Signed-off-by: Alexander Shishkin 
Fixes: 7bd1d4093c2f ("stm class: Introduce an abstraction for System Trace 
Module devices")
Reported-by: Mulu He 
Cc: sta...@vger.kernel.org # v4.4+
---
 drivers/hwtracing/stm/core.c | 6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/hwtracing/stm/core.c b/drivers/hwtracing/stm/core.c
index 5b5807cbcf7c..8c45e79e47db 100644
--- a/drivers/hwtracing/stm/core.c
+++ b/drivers/hwtracing/stm/core.c
@@ -166,11 +166,9 @@ stm_master(struct stm_device *stm, unsigned int idx)
 static int stp_master_alloc(struct stm_device *stm, unsigned int idx)
 {
struct stp_master *master;
-   size_t size;
 
-   size = ALIGN(stm->data->sw_nchannels, 8) / 8;
-   size += sizeof(struct stp_master);
-   master = kzalloc(size, GFP_ATOMIC);
+   master = kzalloc(struct_size(master, chan_map, 
BITS_TO_LONGS(stm->data->sw_nchannels)),
+GFP_ATOMIC);
if (!master)
return -ENOMEM;
 
-- 
2.20.1

RE: [PATCH] stm class: Fix out of bound access from bitmap allocation

2019-04-08 Thread David Laight 
From: Robin Murphy
> Sent: 08 April 2019 11:52
> On 08/04/2019 11:33, David Laight wrote:
> > From: Robin Murphy
> >> Sent: 08 April 2019 11:24
> >> On 07/04/2019 05:31, Sai Prakash Ranjan wrote:
> >>> On 4/5/2019 6:44 PM, David Laight wrote:
>  From: Sai Prakash Ranjan
> >
> > diff --git a/drivers/hwtracing/stm/core.c b/drivers/hwtracing/stm/core.c
> > index 93ce3aa740a9..21a5838f6e67 100644
> > --- a/drivers/hwtracing/stm/core.c
> > +++ b/drivers/hwtracing/stm/core.c
> > @@ -168,7 +168,7 @@ static int stp_master_alloc(struct stm_device
> > *stm, unsigned int idx)
> >    struct stp_master *master;
> >    size_t size;
> >
> > -    size = ALIGN(stm->data->sw_nchannels, 8) / 8;
> > +    size = ALIGN(stm->data->sw_nchannels, STM_MASTER_SZ) /
> > STM_MASTER_SZ;
> 
>  I'm not sure that using STP_MASTER_SZ improves readability at all.
> 
> >>>
> >>> I thought it was better to have a macro than directly specifying
> >>> sizeof(unsigned long), anyways I can change it.
> >>>
>  Is there something that gives the size of a bitmap for 'n' items?
> 
> >>>
> >>> Not sure if there is something.
> >>
> >> If you were to ask the question "how does the bitmap code itself know
> >> what the total size of a bitmap is?", that would quickly lead you
> >> towards BITS_TO_LONGS() ;)
> >>
> >> And given that stp_master::chan_map is already an appropriate type, that
> >> suggests simplifying the entire calculation down to something neat and
> >> tidy like:
> >>
> >> size = offsetof(struct stp_master, 
> >> chan_map[BITS_TO_LONGS(stm->data->sw_nchannels)]);
> >
> > Except that is invalid.
> > You can't use offsetof() with something that isn't a compile time constant.
> 
> Oh, I see the standard does actually say that, although there seem to be
> enough non-constant uses in the kernel to suggest that it still works in
> practice.

One of the compilers we use here complains about it - won't be a linux one.
It also fails to generate compile time constants (for static initialisers)
if you multiply or divide the result of anything that looks like offsetof()
applied to an array element (which is a bug).

I'd guess that the requirement in the standard was added because some
compilers were generating a result that wasn't a compile-time constant
and no one thought you'd want to pass a non-constant argument.

David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, 
UK
Registration No: 1397386 (Wales)

Re: [PATCH] stm class: Fix out of bound access from bitmap allocation

2019-04-08 Thread Robin Murphy 

On 08/04/2019 11:33, David Laight wrote:

From: Robin Murphy

Sent: 08 April 2019 11:24
On 07/04/2019 05:31, Sai Prakash Ranjan wrote:

On 4/5/2019 6:44 PM, David Laight wrote:

From: Sai Prakash Ranjan


diff --git a/drivers/hwtracing/stm/core.c b/drivers/hwtracing/stm/core.c
index 93ce3aa740a9..21a5838f6e67 100644
--- a/drivers/hwtracing/stm/core.c
+++ b/drivers/hwtracing/stm/core.c
@@ -168,7 +168,7 @@ static int stp_master_alloc(struct stm_device
*stm, unsigned int idx)
   struct stp_master *master;
   size_t size;

-    size = ALIGN(stm->data->sw_nchannels, 8) / 8;
+    size = ALIGN(stm->data->sw_nchannels, STM_MASTER_SZ) /
STM_MASTER_SZ;


I'm not sure that using STP_MASTER_SZ improves readability at all.



I thought it was better to have a macro than directly specifying
sizeof(unsigned long), anyways I can change it.


Is there something that gives the size of a bitmap for 'n' items?



Not sure if there is something.


If you were to ask the question "how does the bitmap code itself know
what the total size of a bitmap is?", that would quickly lead you
towards BITS_TO_LONGS() ;)

And given that stp_master::chan_map is already an appropriate type, that
suggests simplifying the entire calculation down to something neat and
tidy like:

size = offsetof(struct stp_master, 
chan_map[BITS_TO_LONGS(stm->data->sw_nchannels)]);


Except that is invalid.
You can't use offsetof() with something that isn't a compile time constant.


Oh, I see the standard does actually say that, although there seem to be 
enough non-constant uses in the kernel to suggest that it still works in 
practice.


However, while writing the above I was still trying to remember the 
other thing I'd seen for handling precisely this variable-sized-struct 
situation, which I've now found again, namely struct_size(). I guess now 
I understand why that isn't implemented in terms of offsetof(), thanks 
for the nudge :)


Robin.

RE: [PATCH] stm class: Fix out of bound access from bitmap allocation

2019-04-08 Thread David Laight 
From: Robin Murphy
> Sent: 08 April 2019 11:24
> On 07/04/2019 05:31, Sai Prakash Ranjan wrote:
> > On 4/5/2019 6:44 PM, David Laight wrote:
> >> From: Sai Prakash Ranjan
> >>>
> >>> diff --git a/drivers/hwtracing/stm/core.c b/drivers/hwtracing/stm/core.c
> >>> index 93ce3aa740a9..21a5838f6e67 100644
> >>> --- a/drivers/hwtracing/stm/core.c
> >>> +++ b/drivers/hwtracing/stm/core.c
> >>> @@ -168,7 +168,7 @@ static int stp_master_alloc(struct stm_device
> >>> *stm, unsigned int idx)
> >>>   struct stp_master *master;
> >>>   size_t size;
> >>>
> >>> -    size = ALIGN(stm->data->sw_nchannels, 8) / 8;
> >>> +    size = ALIGN(stm->data->sw_nchannels, STM_MASTER_SZ) /
> >>> STM_MASTER_SZ;
> >>
> >> I'm not sure that using STP_MASTER_SZ improves readability at all.
> >>
> >
> > I thought it was better to have a macro than directly specifying
> > sizeof(unsigned long), anyways I can change it.
> >
> >> Is there something that gives the size of a bitmap for 'n' items?
> >>
> >
> > Not sure if there is something.
> 
> If you were to ask the question "how does the bitmap code itself know
> what the total size of a bitmap is?", that would quickly lead you
> towards BITS_TO_LONGS() ;)
> 
> And given that stp_master::chan_map is already an appropriate type, that
> suggests simplifying the entire calculation down to something neat and
> tidy like:
> 
> size = offsetof(struct stp_master, 
> chan_map[BITS_TO_LONGS(stm->data->sw_nchannels)]);

Except that is invalid.
You can't use offsetof() with something that isn't a compile time constant.

David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, 
UK
Registration No: 1397386 (Wales)

Re: [PATCH] stm class: Fix out of bound access from bitmap allocation

2019-04-08 Thread Robin Murphy 

On 07/04/2019 05:31, Sai Prakash Ranjan wrote:

On 4/5/2019 6:44 PM, David Laight wrote:

From: Sai Prakash Ranjan


diff --git a/drivers/hwtracing/stm/core.c b/drivers/hwtracing/stm/core.c
index 93ce3aa740a9..21a5838f6e67 100644
--- a/drivers/hwtracing/stm/core.c
+++ b/drivers/hwtracing/stm/core.c
@@ -168,7 +168,7 @@ static int stp_master_alloc(struct stm_device 
*stm, unsigned int idx)

  struct stp_master *master;
  size_t size;

-    size = ALIGN(stm->data->sw_nchannels, 8) / 8;
+    size = ALIGN(stm->data->sw_nchannels, STM_MASTER_SZ) / 
STM_MASTER_SZ;


I'm not sure that using STP_MASTER_SZ improves readability at all.



I thought it was better to have a macro than directly specifying
sizeof(unsigned long), anyways I can change it.


Is there something that gives the size of a bitmap for 'n' items?



Not sure if there is something.


If you were to ask the question "how does the bitmap code itself know 
what the total size of a bitmap is?", that would quickly lead you 
towards BITS_TO_LONGS() ;)


And given that stp_master::chan_map is already an appropriate type, that 
suggests simplifying the entire calculation down to something neat and 
tidy like:


size = offsetof(struct stp_master, 
chan_map[BITS_TO_LONGS(stm->data->sw_nchannels)]);



Robin.

Re: [PATCH] stm class: Fix out of bound access from bitmap allocation

2019-04-06 Thread Sai Prakash Ranjan 

On 4/5/2019 6:44 PM, David Laight wrote:

From: Sai Prakash Ranjan


diff --git a/drivers/hwtracing/stm/core.c b/drivers/hwtracing/stm/core.c
index 93ce3aa740a9..21a5838f6e67 100644
--- a/drivers/hwtracing/stm/core.c
+++ b/drivers/hwtracing/stm/core.c
@@ -168,7 +168,7 @@ static int stp_master_alloc(struct stm_device *stm, 
unsigned int idx)
struct stp_master *master;
size_t size;

-   size = ALIGN(stm->data->sw_nchannels, 8) / 8;
+   size = ALIGN(stm->data->sw_nchannels, STM_MASTER_SZ) / STM_MASTER_SZ;


I'm not sure that using STP_MASTER_SZ improves readability at all.



I thought it was better to have a macro than directly specifying
sizeof(unsigned long), anyways I can change it.


Is there something that gives the size of a bitmap for 'n' items?



Not sure if there is something.

Thanks,
Sai

--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

RE: [PATCH] stm class: Fix out of bound access from bitmap allocation

2019-04-05 Thread David Laight 
From: Sai Prakash Ranjan
> Sent: 05 April 2019 13:23
> 
> From: Mulu He 
> 
> Bitmap allocation works on array of unsigned longs and
> for stm master allocation when the number of software
> channels is 32, 4 bytes are allocated and there is a out of
> bound access at the first 8 bytes access of bitmap region.
> 
> Fixes: 7bd1d4093c2f ("stm class: Introduce an abstraction for System Trace 
> Module devices")
> Signed-off-by: Mulu He 
> Signed-off-by: Sai Prakash Ranjan 
> Cc: sta...@vger.kernel.org
> ---
>  drivers/hwtracing/stm/core.c | 2 +-
>  drivers/hwtracing/stm/stm.h  | 2 ++
>  2 files changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/hwtracing/stm/core.c b/drivers/hwtracing/stm/core.c
> index 93ce3aa740a9..21a5838f6e67 100644
> --- a/drivers/hwtracing/stm/core.c
> +++ b/drivers/hwtracing/stm/core.c
> @@ -168,7 +168,7 @@ static int stp_master_alloc(struct stm_device *stm, 
> unsigned int idx)
>   struct stp_master *master;
>   size_t size;
> 
> - size = ALIGN(stm->data->sw_nchannels, 8) / 8;
> + size = ALIGN(stm->data->sw_nchannels, STM_MASTER_SZ) / STM_MASTER_SZ;

I'm not sure that using STP_MASTER_SZ improves readability at all.

Is there something that gives the size of a bitmap for 'n' items?

David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, 
UK
Registration No: 1397386 (Wales)

[PATCH] stm class: Fix out of bound access from bitmap allocation

2019-04-05 Thread Sai Prakash Ranjan 
From: Mulu He 

Bitmap allocation works on array of unsigned longs and
for stm master allocation when the number of software
channels is 32, 4 bytes are allocated and there is a out of
bound access at the first 8 bytes access of bitmap region.

Fixes: 7bd1d4093c2f ("stm class: Introduce an abstraction for System Trace 
Module devices")
Signed-off-by: Mulu He 
Signed-off-by: Sai Prakash Ranjan 
Cc: sta...@vger.kernel.org
---
 drivers/hwtracing/stm/core.c | 2 +-
 drivers/hwtracing/stm/stm.h  | 2 ++
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/hwtracing/stm/core.c b/drivers/hwtracing/stm/core.c
index 93ce3aa740a9..21a5838f6e67 100644
--- a/drivers/hwtracing/stm/core.c
+++ b/drivers/hwtracing/stm/core.c
@@ -168,7 +168,7 @@ static int stp_master_alloc(struct stm_device *stm, 
unsigned int idx)
struct stp_master *master;
size_t size;
 
-   size = ALIGN(stm->data->sw_nchannels, 8) / 8;
+   size = ALIGN(stm->data->sw_nchannels, STM_MASTER_SZ) / STM_MASTER_SZ;
size += sizeof(struct stp_master);
master = kzalloc(size, GFP_ATOMIC);
if (!master)
diff --git a/drivers/hwtracing/stm/stm.h b/drivers/hwtracing/stm/stm.h
index 3569439d53bb..10eac550c75f 100644
--- a/drivers/hwtracing/stm/stm.h
+++ b/drivers/hwtracing/stm/stm.h
@@ -12,6 +12,8 @@
 
 #include 
 
+#define STM_MASTER_SZ sizeof(unsigned long)
+
 struct stp_policy;
 struct stp_policy_node;
 struct stm_protocol_driver;
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation