| Message ID | 20190405122256.27840-1-saiprakash.ranjan@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | stm class: Fix out of bound access from bitmap allocation | expand |
From: Sai Prakash Ranjan > Sent: 05 April 2019 13:23 > > From: Mulu He <muluhe@codeaurora.org> > > 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 <muluhe@codeaurora.org> > Signed-off-by: Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org> > Cc: stable@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)
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
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.
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)
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.
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)
Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org> writes: > From: Mulu He <muluhe@codeaurora.org> > > 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 <alexander.shishkin@linux.intel.com> 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 <alexander.shishkin@linux.intel.com> Fixes: 7bd1d4093c2f ("stm class: Introduce an abstraction for System Trace Module devices") Reported-by: Mulu He <muluhe@codeaurora.org> Cc: stable@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;
On 4/16/2019 8:30 PM, Alexander Shishkin wrote: > Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org> writes: > >> From: Mulu He <muluhe@codeaurora.org> >> >> 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 <alexander.shishkin@linux.intel.com> > 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 <alexander.shishkin@linux.intel.com> > Fixes: 7bd1d4093c2f ("stm class: Introduce an abstraction for System Trace Module devices") > Reported-by: Mulu He <muluhe@codeaurora.org> > Cc: stable@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
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 <linux/configfs.h> +#define STM_MASTER_SZ sizeof(unsigned long) + struct stp_policy; struct stp_policy_node; struct stm_protocol_driver;