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

From: David Laight <David.Laight@ACULAB.COM>
To: 'Robin Murphy' <robin.murphy@arm.com>,
	Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org>,
	Alexander Shishkin <alexander.shishkin@linux.intel.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Mulu He <muluhe@codeaurora.org>,
	Tingwei Zhang <tingwei@codeaurora.org>,
	Maxime Coquelin <mcoquelin.stm32@gmail.com>,
	Alexandre Torgue <alexandre.torgue@st.com>,
	"linux-stm32@st-md-mailman.stormreply.com"
	<linux-stm32@st-md-mailman.stormreply.com>,
	Mathieu Poirier <mathieu.poirier@linaro.org>,
	Suzuki K Poulose <suzuki.poulose@arm.com>,
	"Mike Leach" <mike.leach@linaro.org>,
	Leo Yan <leo.yan@linaro.org>
Cc: Rajendra Nayak <rnayak@codeaurora.org>,
	"linux-arm-msm@vger.kernel.org" <linux-arm-msm@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"stable@vger.kernel.org" <stable@vger.kernel.org>,
	Sibi Sankar <sibis@codeaurora.org>,
	Vivek Gautam <vivek.gautam@codeaurora.org>,
	"linux-arm-kernel@lists.infradead.org"
	<linux-arm-kernel@lists.infradead.org>
Subject: RE: [PATCH] stm class: Fix out of bound access from bitmap allocation
Date: Mon, 8 Apr 2019 11:13:33 +0000	[thread overview]
Message-ID: <fcc635f7984a4fe786e37f4c04a6cd86@AcuMS.aculab.com> (raw)
In-Reply-To: <f3a699e5-a6c4-83f4-fff3-20515e8caa06@arm.com>

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)
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel