On 16/07/13 14:53, Jiri Olsa wrote: > 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)' ?
'offset + size' can overflow and end up less than 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)) > OK >> + >> 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..? Currently the caller must ensure there is enough space. > > >> 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 OK > >> + * 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; >> -- 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/