| Message ID | 20211201123334.679131-2-german.gomez@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Support register names from all architectures | expand |
On Wed, Dec 01, 2021 at 12:33:29PM +0000, German Gomez wrote: > The size of the cache of register values is arch-dependant > (PERF_REGS_MAX). This has the potential of causing an out-of-bounds > access in the function "perf_reg_value" if the local architecture > contains less registers than the one the perf.data file was recorded on. > > Since the maximum number of registers is bound by the bitmask "u64 > cache_mask", and the size of the cache when running under x86 systems is > 64 already, fix the size to 64 and add a range-check to the function > "perf_reg_value" to prevent out-of-bounds access. > > Signed-off-by: German Gomez <german.gomez@arm.com> Acked-by: Jiri Olsa <jolsa@redhat.com> thanks, jirka > --- > tools/perf/util/event.h | 5 ++++- > tools/perf/util/perf_regs.c | 3 +++ > 2 files changed, 7 insertions(+), 1 deletion(-) > > diff --git a/tools/perf/util/event.h b/tools/perf/util/event.h > index 95ffed663..c59331eea 100644 > --- a/tools/perf/util/event.h > +++ b/tools/perf/util/event.h > @@ -44,13 +44,16 @@ struct perf_event_attr; > /* perf sample has 16 bits size limit */ > #define PERF_SAMPLE_MAX_SIZE (1 << 16) > > +/* number of register is bound by the number of bits in regs_dump::mask (64) */ > +#define PERF_SAMPLE_REGS_CACHE_SIZE (8 * sizeof(u64)) > + > struct regs_dump { > u64 abi; > u64 mask; > u64 *regs; > > /* Cached values/mask filled by first register access. */ > - u64 cache_regs[PERF_REGS_MAX]; > + u64 cache_regs[PERF_SAMPLE_REGS_CACHE_SIZE]; > u64 cache_mask; > }; > > diff --git a/tools/perf/util/perf_regs.c b/tools/perf/util/perf_regs.c > index 5ee47ae15..06a7461ba 100644 > --- a/tools/perf/util/perf_regs.c > +++ b/tools/perf/util/perf_regs.c > @@ -25,6 +25,9 @@ int perf_reg_value(u64 *valp, struct regs_dump *regs, int id) > int i, idx = 0; > u64 mask = regs->mask; > > + if ((u64)id >= PERF_SAMPLE_REGS_CACHE_SIZE) > + return -EINVAL; > + > if (regs->cache_mask & (1ULL << id)) > goto out; > > -- > 2.25.1 >
On 12/1/21 6:03 PM, German Gomez wrote: > The size of the cache of register values is arch-dependant > (PERF_REGS_MAX). This has the potential of causing an out-of-bounds > access in the function "perf_reg_value" if the local architecture > contains less registers than the one the perf.data file was recorded on. > > Since the maximum number of registers is bound by the bitmask "u64 > cache_mask", and the size of the cache when running under x86 systems is > 64 already, fix the size to 64 and add a range-check to the function > "perf_reg_value" to prevent out-of-bounds access. > Patch looks good to me. Reviewed-by: Kajol Jain<kjain@linux.ibm.com> Thanks, Kajol Jain > Signed-off-by: German Gomez <german.gomez@arm.com> > --- > tools/perf/util/event.h | 5 ++++- > tools/perf/util/perf_regs.c | 3 +++ > 2 files changed, 7 insertions(+), 1 deletion(-) > > diff --git a/tools/perf/util/event.h b/tools/perf/util/event.h > index 95ffed663..c59331eea 100644 > --- a/tools/perf/util/event.h > +++ b/tools/perf/util/event.h > @@ -44,13 +44,16 @@ struct perf_event_attr; > /* perf sample has 16 bits size limit */ > #define PERF_SAMPLE_MAX_SIZE (1 << 16) > > +/* number of register is bound by the number of bits in regs_dump::mask (64) */ > +#define PERF_SAMPLE_REGS_CACHE_SIZE (8 * sizeof(u64)) > + > struct regs_dump { > u64 abi; > u64 mask; > u64 *regs; > > /* Cached values/mask filled by first register access. */ > - u64 cache_regs[PERF_REGS_MAX]; > + u64 cache_regs[PERF_SAMPLE_REGS_CACHE_SIZE]; > u64 cache_mask; > }; > > diff --git a/tools/perf/util/perf_regs.c b/tools/perf/util/perf_regs.c > index 5ee47ae15..06a7461ba 100644 > --- a/tools/perf/util/perf_regs.c > +++ b/tools/perf/util/perf_regs.c > @@ -25,6 +25,9 @@ int perf_reg_value(u64 *valp, struct regs_dump *regs, int id) > int i, idx = 0; > u64 mask = regs->mask; > > + if ((u64)id >= PERF_SAMPLE_REGS_CACHE_SIZE) > + return -EINVAL; > + > if (regs->cache_mask & (1ULL << id)) > goto out; > >
Em Fri, Dec 10, 2021 at 02:47:49PM +0530, kajoljain escreveu: > > > On 12/1/21 6:03 PM, German Gomez wrote: > > The size of the cache of register values is arch-dependant > > (PERF_REGS_MAX). This has the potential of causing an out-of-bounds > > access in the function "perf_reg_value" if the local architecture > > contains less registers than the one the perf.data file was recorded on. > > > > Since the maximum number of registers is bound by the bitmask "u64 > > cache_mask", and the size of the cache when running under x86 systems is > > 64 already, fix the size to 64 and add a range-check to the function > > "perf_reg_value" to prevent out-of-bounds access. > > > > Patch looks good to me. > > Reviewed-by: Kajol Jain<kjain@linux.ibm.com> Thanks, applied. - Arnaldo > Thanks, > Kajol Jain > > > Signed-off-by: German Gomez <german.gomez@arm.com> > > --- > > tools/perf/util/event.h | 5 ++++- > > tools/perf/util/perf_regs.c | 3 +++ > > 2 files changed, 7 insertions(+), 1 deletion(-) > > > > diff --git a/tools/perf/util/event.h b/tools/perf/util/event.h > > index 95ffed663..c59331eea 100644 > > --- a/tools/perf/util/event.h > > +++ b/tools/perf/util/event.h > > @@ -44,13 +44,16 @@ struct perf_event_attr; > > /* perf sample has 16 bits size limit */ > > #define PERF_SAMPLE_MAX_SIZE (1 << 16) > > > > +/* number of register is bound by the number of bits in regs_dump::mask (64) */ > > +#define PERF_SAMPLE_REGS_CACHE_SIZE (8 * sizeof(u64)) > > + > > struct regs_dump { > > u64 abi; > > u64 mask; > > u64 *regs; > > > > /* Cached values/mask filled by first register access. */ > > - u64 cache_regs[PERF_REGS_MAX]; > > + u64 cache_regs[PERF_SAMPLE_REGS_CACHE_SIZE]; > > u64 cache_mask; > > }; > > > > diff --git a/tools/perf/util/perf_regs.c b/tools/perf/util/perf_regs.c > > index 5ee47ae15..06a7461ba 100644 > > --- a/tools/perf/util/perf_regs.c > > +++ b/tools/perf/util/perf_regs.c > > @@ -25,6 +25,9 @@ int perf_reg_value(u64 *valp, struct regs_dump *regs, int id) > > int i, idx = 0; > > u64 mask = regs->mask; > > > > + if ((u64)id >= PERF_SAMPLE_REGS_CACHE_SIZE) > > + return -EINVAL; > > + > > if (regs->cache_mask & (1ULL << id)) > > goto out; > > > >
On 10/12/2021 13:38, Arnaldo Carvalho de Melo wrote: > Em Fri, Dec 10, 2021 at 02:47:49PM +0530, kajoljain escreveu: >> >> On 12/1/21 6:03 PM, German Gomez wrote: >>> The size of the cache of register values is arch-dependant >>> (PERF_REGS_MAX). This has the potential of causing an out-of-bounds >>> access in the function "perf_reg_value" if the local architecture >>> contains less registers than the one the perf.data file was recorded on. >>> >>> Since the maximum number of registers is bound by the bitmask "u64 >>> cache_mask", and the size of the cache when running under x86 systems is >>> 64 already, fix the size to 64 and add a range-check to the function >>> "perf_reg_value" to prevent out-of-bounds access. >>> >> Patch looks good to me. >> >> Reviewed-by: Kajol Jain<kjain@linux.ibm.com> > Thanks, applied. > > - Arnaldo Thanks Arnaldo, and the rest for the review. I did send a v2 of this patch afterwards. The only difference was to give credit to the reporter in the commit message with: Reported-by: Alexandre Truong <alexandre.truong@arm.com> Thanks, German > >> Thanks, >> Kajol Jain >> >>> Signed-off-by: German Gomez <german.gomez@arm.com> >>> --- >>> tools/perf/util/event.h | 5 ++++- >>> tools/perf/util/perf_regs.c | 3 +++ >>> 2 files changed, 7 insertions(+), 1 deletion(-) >>> >>> diff --git a/tools/perf/util/event.h b/tools/perf/util/event.h >>> index 95ffed663..c59331eea 100644 >>> --- a/tools/perf/util/event.h >>> +++ b/tools/perf/util/event.h >>> @@ -44,13 +44,16 @@ struct perf_event_attr; >>> /* perf sample has 16 bits size limit */ >>> #define PERF_SAMPLE_MAX_SIZE (1 << 16) >>> >>> +/* number of register is bound by the number of bits in regs_dump::mask (64) */ >>> +#define PERF_SAMPLE_REGS_CACHE_SIZE (8 * sizeof(u64)) >>> + >>> struct regs_dump { >>> u64 abi; >>> u64 mask; >>> u64 *regs; >>> >>> /* Cached values/mask filled by first register access. */ >>> - u64 cache_regs[PERF_REGS_MAX]; >>> + u64 cache_regs[PERF_SAMPLE_REGS_CACHE_SIZE]; >>> u64 cache_mask; >>> }; >>> >>> diff --git a/tools/perf/util/perf_regs.c b/tools/perf/util/perf_regs.c >>> index 5ee47ae15..06a7461ba 100644 >>> --- a/tools/perf/util/perf_regs.c >>> +++ b/tools/perf/util/perf_regs.c >>> @@ -25,6 +25,9 @@ int perf_reg_value(u64 *valp, struct regs_dump *regs, int id) >>> int i, idx = 0; >>> u64 mask = regs->mask; >>> >>> + if ((u64)id >= PERF_SAMPLE_REGS_CACHE_SIZE) >>> + return -EINVAL; >>> + >>> if (regs->cache_mask & (1ULL << id)) >>> goto out; >>> >>>
On December 10, 2021 12:28:56 PM GMT-03:00, German Gomez <german.gomez@arm.com> wrote: > >On 10/12/2021 13:38, Arnaldo Carvalho de Melo wrote: >> Em Fri, Dec 10, 2021 at 02:47:49PM +0530, kajoljain escreveu: >>> >>> On 12/1/21 6:03 PM, German Gomez wrote: >>>> The size of the cache of register values is arch-dependant >>>> (PERF_REGS_MAX). This has the potential of causing an out-of-bounds >>>> access in the function "perf_reg_value" if the local architecture >>>> contains less registers than the one the perf.data file was recorded on. >>>> >>>> Since the maximum number of registers is bound by the bitmask "u64 >>>> cache_mask", and the size of the cache when running under x86 systems is >>>> 64 already, fix the size to 64 and add a range-check to the function >>>> "perf_reg_value" to prevent out-of-bounds access. >>>> >>> Patch looks good to me. >>> >>> Reviewed-by: Kajol Jain<kjain@linux.ibm.com> >> Thanks, applied. >> >> - Arnaldo > >Thanks Arnaldo, and the rest for the review. > >I did send a v2 of this patch afterwards. The only difference was to >give credit to the reporter in the commit message with: > >Reported-by: Alexandre Truong <alexandre.truong@arm.com> I'll add it. - Arnaldo > >Thanks, >German > >> >>> Thanks, >>> Kajol Jain >>> >>>> Signed-off-by: German Gomez <german.gomez@arm.com> >>>> --- >>>> tools/perf/util/event.h | 5 ++++- >>>> tools/perf/util/perf_regs.c | 3 +++ >>>> 2 files changed, 7 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/tools/perf/util/event.h b/tools/perf/util/event.h >>>> index 95ffed663..c59331eea 100644 >>>> --- a/tools/perf/util/event.h >>>> +++ b/tools/perf/util/event.h >>>> @@ -44,13 +44,16 @@ struct perf_event_attr; >>>> /* perf sample has 16 bits size limit */ >>>> #define PERF_SAMPLE_MAX_SIZE (1 << 16) >>>> >>>> +/* number of register is bound by the number of bits in regs_dump::mask (64) */ >>>> +#define PERF_SAMPLE_REGS_CACHE_SIZE (8 * sizeof(u64)) >>>> + >>>> struct regs_dump { >>>> u64 abi; >>>> u64 mask; >>>> u64 *regs; >>>> >>>> /* Cached values/mask filled by first register access. */ >>>> - u64 cache_regs[PERF_REGS_MAX]; >>>> + u64 cache_regs[PERF_SAMPLE_REGS_CACHE_SIZE]; >>>> u64 cache_mask; >>>> }; >>>> >>>> diff --git a/tools/perf/util/perf_regs.c b/tools/perf/util/perf_regs.c >>>> index 5ee47ae15..06a7461ba 100644 >>>> --- a/tools/perf/util/perf_regs.c >>>> +++ b/tools/perf/util/perf_regs.c >>>> @@ -25,6 +25,9 @@ int perf_reg_value(u64 *valp, struct regs_dump *regs, int id) >>>> int i, idx = 0; >>>> u64 mask = regs->mask; >>>> >>>> + if ((u64)id >= PERF_SAMPLE_REGS_CACHE_SIZE) >>>> + return -EINVAL; >>>> + >>>> if (regs->cache_mask & (1ULL << id)) >>>> goto out; >>>> >>>>
diff --git a/tools/perf/util/event.h b/tools/perf/util/event.h index 95ffed663..c59331eea 100644 --- a/tools/perf/util/event.h +++ b/tools/perf/util/event.h @@ -44,13 +44,16 @@ struct perf_event_attr; /* perf sample has 16 bits size limit */ #define PERF_SAMPLE_MAX_SIZE (1 << 16) +/* number of register is bound by the number of bits in regs_dump::mask (64) */ +#define PERF_SAMPLE_REGS_CACHE_SIZE (8 * sizeof(u64)) + struct regs_dump { u64 abi; u64 mask; u64 *regs; /* Cached values/mask filled by first register access. */ - u64 cache_regs[PERF_REGS_MAX]; + u64 cache_regs[PERF_SAMPLE_REGS_CACHE_SIZE]; u64 cache_mask; }; diff --git a/tools/perf/util/perf_regs.c b/tools/perf/util/perf_regs.c index 5ee47ae15..06a7461ba 100644 --- a/tools/perf/util/perf_regs.c +++ b/tools/perf/util/perf_regs.c @@ -25,6 +25,9 @@ int perf_reg_value(u64 *valp, struct regs_dump *regs, int id) int i, idx = 0; u64 mask = regs->mask; + if ((u64)id >= PERF_SAMPLE_REGS_CACHE_SIZE) + return -EINVAL; + if (regs->cache_mask & (1ULL << id)) goto out;
The size of the cache of register values is arch-dependant (PERF_REGS_MAX). This has the potential of causing an out-of-bounds access in the function "perf_reg_value" if the local architecture contains less registers than the one the perf.data file was recorded on. Since the maximum number of registers is bound by the bitmask "u64 cache_mask", and the size of the cache when running under x86 systems is 64 already, fix the size to 64 and add a range-check to the function "perf_reg_value" to prevent out-of-bounds access. Signed-off-by: German Gomez <german.gomez@arm.com> --- tools/perf/util/event.h | 5 ++++- tools/perf/util/perf_regs.c | 3 +++ 2 files changed, 7 insertions(+), 1 deletion(-)