On Tue, Jul 16, 2013 at 09:38:11AM +0300, Adrian Hunter wrote: > The size of data retrieved from a sample event must be > validated to ensure it does not go past the end of the > event. That was being done sporadically and without > considering integer overflows. > > Signed-off-by: Adrian Hunter <adrian.hun...@intel.com> > --- > tools/perf/util/evsel.c | 102 > ++++++++++++++++++++++++++++++------------------ > 1 file changed, 64 insertions(+), 38 deletions(-) > > diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c > index 724b75a..20e2ed9 100644 > --- a/tools/perf/util/evsel.c > +++ b/tools/perf/util/evsel.c > @@ -1114,24 +1114,38 @@ static int perf_evsel__parse_id_sample(const struct > perf_evsel *evsel, > return 0; > } > > -static bool sample_overlap(const union perf_event *event, > - const void *offset, u64 size) > +static inline bool overflow_one(const void *endp, const void *offset) > { > - const void *base = event; > - > - if (offset + size > base + event->header.size) > - return true; > + return offset + sizeof(u64) > endp; > +} > > - return false; > +static inline bool overflow(const void *endp, u16 max_size, const void > *offset, > + u64 size) > +{ > + return size > max_size || offset + size > endp;
hum, does '(size > max_size)' catch anything that would not be catched by '(offset + size > endp)' ? > } > > +#define OVERFLOW_CHECK_ONE(offset) \ > + do { \ > + if (overflow_one(endp, (offset))) \ > + return -EFAULT; \ > + } while (0) > + > +#define OVERFLOW_CHECK(offset, size) \ > + do { \ > + if (overflow(endp, max_size, (offset), (size))) \ > + return -EFAULT; \ > + } while (0) I think, it'd be better to have just one overflow check function, like: static inline bool overflow(const void *endp, const void *offset, u64 size) { return offset + size > endp; } #define OVERFLOW_CHECK(offset, size) \ do { \ if (overflow(endp, (offset), (size))) \ return -EFAULT; \ } while (0) #define OVERFLOW_CHECK_u64(offset) OVERFLOW_CHECK(offset, sizeof(u64)) > + > int perf_evsel__parse_sample(struct perf_evsel *evsel, union perf_event > *event, > struct perf_sample *data) > { Hum, I was thinking of making this check more general and cover also perf_event__synthesize_sample function.. but looks like it's not needed there..? > u64 type = evsel->attr.sample_type; > - u64 regs_user = evsel->attr.sample_regs_user; > bool swapped = evsel->needs_swap; > const u64 *array; > + u16 max_size = event->header.size; > + const void *endp = (void *)event + max_size; > + u64 sz; > > /* > * used for cross-endian analysis. See git commit 65014ab3 > @@ -1153,6 +1167,11 @@ int perf_evsel__parse_sample(struct perf_evsel *evsel, > union perf_event *event, > > array = event->sample.array; > > + /* > + * sample_size is based on PERF_SAMPLE_MASK which includes up to please prepend ^^^: The evsel's > + * PERF_SAMPLE_PERIOD. After that overflow_one() or overflow() must be > + * used to check the format does not go past the end of the event. > + */ > if (evsel->sample_size + sizeof(event->header) > event->header.size) > return -EFAULT; > thanks, jirka -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/