Re: [PATCH V6 05/12] perf tools: tidy up sample parsing overflow checking

[Jiri Olsa <jolsa@redhat.com>]

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.hunter@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