[PATCH] perf session: Remap buf if there is no space for event

[PATCH] perf session: Remap buf if there is no space for event

[Denis Nikitin]

If a perf event doesn't fit into remaining buffer space return NULL to
remap buf and fetch the event again.
Keep the logic to error out on inadequate input from fuzzing.

This fixes perf failing on ChromeOS (with 32b userspace):

  $ perf report -v -i perf.data
  ...
  prefetch_event: head=0x1fffff8 event->header_size=0x30, mmap_size=0x2000000: fuzzed or compressed perf.data?
  Error:
  failed to process sample

Fixes: 57fc032ad643 ("perf session: Avoid infinite loop when seeing invalid header.size")
Signed-off-by: Denis Nikitin <denik@chromium.org>
---
 tools/perf/util/session.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
index 3b8dfe603e50..45a30040ec8d 100644
--- a/tools/perf/util/session.c
+++ b/tools/perf/util/session.c
@@ -2095,6 +2095,7 @@ prefetch_event(char *buf, u64 head, size_t mmap_size,
 	       bool needs_swap, union perf_event *error)
 {
 	union perf_event *event;
+	u16 event_size;
 
 	/*
 	 * Ensure we have enough space remaining to read
@@ -2107,15 +2108,23 @@ prefetch_event(char *buf, u64 head, size_t mmap_size,
 	if (needs_swap)
 		perf_event_header__bswap(&event->header);
 
-	if (head + event->header.size <= mmap_size)
+	event_size = event->header.size;
+	if (head + event_size <= mmap_size)
 		return event;
 
 	/* We're not fetching the event so swap back again */
 	if (needs_swap)
 		perf_event_header__bswap(&event->header);
 
-	pr_debug("%s: head=%#" PRIx64 " event->header_size=%#x, mmap_size=%#zx:"
-		 " fuzzed or compressed perf.data?\n",__func__, head, event->header.size, mmap_size);
+	/* Check if the event fits into the next mmapped buf. */
+	if (event_size <= mmap_size - head % page_size) {
+		/* Remap buf and fetch again. */
+		return NULL;
+	}
+
+	/* Invalid input. Event size should never exceed mmap_size. */
+	pr_debug("%s: head=%#" PRIx64 " event->header.size=%#x, mmap_size=%#zx:"
+		 " fuzzed or compressed perf.data?\n", __func__, head, event_size, mmap_size);
 
 	return error;
 }
-- 
2.35.1.1021.g381101b075-goog

Re: [PATCH] perf session: Remap buf if there is no space for event

[Jiri Olsa]

On Tue, Mar 29, 2022 at 08:11:30PM -0700, Denis Nikitin wrote:
> If a perf event doesn't fit into remaining buffer space return NULL to
> remap buf and fetch the event again.
> Keep the logic to error out on inadequate input from fuzzing.
> 
> This fixes perf failing on ChromeOS (with 32b userspace):
> 
>   $ perf report -v -i perf.data
>   ...
>   prefetch_event: head=0x1fffff8 event->header_size=0x30, mmap_size=0x2000000: fuzzed or compressed perf.data?
>   Error:
>   failed to process sample
> 
> Fixes: 57fc032ad643 ("perf session: Avoid infinite loop when seeing invalid header.size")
> Signed-off-by: Denis Nikitin <denik@chromium.org>

Acked-by: Jiri Olsa <jolsa@kernel.org>

thanks,
jirka

> ---
>  tools/perf/util/session.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
> index 3b8dfe603e50..45a30040ec8d 100644
> --- a/tools/perf/util/session.c
> +++ b/tools/perf/util/session.c
> @@ -2095,6 +2095,7 @@ prefetch_event(char *buf, u64 head, size_t mmap_size,
>  	       bool needs_swap, union perf_event *error)
>  {
>  	union perf_event *event;
> +	u16 event_size;
>  
>  	/*
>  	 * Ensure we have enough space remaining to read
> @@ -2107,15 +2108,23 @@ prefetch_event(char *buf, u64 head, size_t mmap_size,
>  	if (needs_swap)
>  		perf_event_header__bswap(&event->header);
>  
> -	if (head + event->header.size <= mmap_size)
> +	event_size = event->header.size;
> +	if (head + event_size <= mmap_size)
>  		return event;
>  
>  	/* We're not fetching the event so swap back again */
>  	if (needs_swap)
>  		perf_event_header__bswap(&event->header);
>  
> -	pr_debug("%s: head=%#" PRIx64 " event->header_size=%#x, mmap_size=%#zx:"
> -		 " fuzzed or compressed perf.data?\n",__func__, head, event->header.size, mmap_size);
> +	/* Check if the event fits into the next mmapped buf. */
> +	if (event_size <= mmap_size - head % page_size) {
> +		/* Remap buf and fetch again. */
> +		return NULL;
> +	}
> +
> +	/* Invalid input. Event size should never exceed mmap_size. */
> +	pr_debug("%s: head=%#" PRIx64 " event->header.size=%#x, mmap_size=%#zx:"
> +		 " fuzzed or compressed perf.data?\n", __func__, head, event_size, mmap_size);
>  
>  	return error;
>  }
> -- 
> 2.35.1.1021.g381101b075-goog
> 

Re: [PATCH] perf session: Remap buf if there is no space for event

[Arnaldo Carvalho de Melo]

Em Thu, Mar 31, 2022 at 01:44:43PM +0200, Jiri Olsa escreveu:
> On Tue, Mar 29, 2022 at 08:11:30PM -0700, Denis Nikitin wrote:
> > If a perf event doesn't fit into remaining buffer space return NULL to
> > remap buf and fetch the event again.
> > Keep the logic to error out on inadequate input from fuzzing.
> > 
> > This fixes perf failing on ChromeOS (with 32b userspace):
> > 
> >   $ perf report -v -i perf.data
> >   ...
> >   prefetch_event: head=0x1fffff8 event->header_size=0x30, mmap_size=0x2000000: fuzzed or compressed perf.data?
> >   Error:
> >   failed to process sample
> > 
> > Fixes: 57fc032ad643 ("perf session: Avoid infinite loop when seeing invalid header.size")
> > Signed-off-by: Denis Nikitin <denik@chromium.org>
> 
> Acked-by: Jiri Olsa <jolsa@kernel.org>

Thanks, applied.

- Arnaldo

 
> thanks,
> jirka
> 
> > ---
> >  tools/perf/util/session.c | 15 ++++++++++++---
> >  1 file changed, 12 insertions(+), 3 deletions(-)
> > 
> > diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
> > index 3b8dfe603e50..45a30040ec8d 100644
> > --- a/tools/perf/util/session.c
> > +++ b/tools/perf/util/session.c
> > @@ -2095,6 +2095,7 @@ prefetch_event(char *buf, u64 head, size_t mmap_size,
> >  	       bool needs_swap, union perf_event *error)
> >  {
> >  	union perf_event *event;
> > +	u16 event_size;
> >  
> >  	/*
> >  	 * Ensure we have enough space remaining to read
> > @@ -2107,15 +2108,23 @@ prefetch_event(char *buf, u64 head, size_t mmap_size,
> >  	if (needs_swap)
> >  		perf_event_header__bswap(&event->header);
> >  
> > -	if (head + event->header.size <= mmap_size)
> > +	event_size = event->header.size;
> > +	if (head + event_size <= mmap_size)
> >  		return event;
> >  
> >  	/* We're not fetching the event so swap back again */
> >  	if (needs_swap)
> >  		perf_event_header__bswap(&event->header);
> >  
> > -	pr_debug("%s: head=%#" PRIx64 " event->header_size=%#x, mmap_size=%#zx:"
> > -		 " fuzzed or compressed perf.data?\n",__func__, head, event->header.size, mmap_size);
> > +	/* Check if the event fits into the next mmapped buf. */
> > +	if (event_size <= mmap_size - head % page_size) {
> > +		/* Remap buf and fetch again. */
> > +		return NULL;
> > +	}
> > +
> > +	/* Invalid input. Event size should never exceed mmap_size. */
> > +	pr_debug("%s: head=%#" PRIx64 " event->header.size=%#x, mmap_size=%#zx:"
> > +		 " fuzzed or compressed perf.data?\n", __func__, head, event_size, mmap_size);
> >  
> >  	return error;
> >  }
> > -- 
> > 2.35.1.1021.g381101b075-goog
> > 

-- 

- Arnaldo

Re: [PATCH] perf session: Remap buf if there is no space for event

[James Clark]

On 30/03/2022 04:11, Denis Nikitin wrote:
> If a perf event doesn't fit into remaining buffer space return NULL to
> remap buf and fetch the event again.
> Keep the logic to error out on inadequate input from fuzzing.
> 
> This fixes perf failing on ChromeOS (with 32b userspace):
> 
>   $ perf report -v -i perf.data
>   ...
>   prefetch_event: head=0x1fffff8 event->header_size=0x30, mmap_size=0x2000000: fuzzed or compressed perf.data?
>   Error:
>   failed to process sample
> 
> Fixes: 57fc032ad643 ("perf session: Avoid infinite loop when seeing invalid header.size")
> Signed-off-by: Denis Nikitin <denik@chromium.org>
> ---
>  tools/perf/util/session.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
> index 3b8dfe603e50..45a30040ec8d 100644
> --- a/tools/perf/util/session.c
> +++ b/tools/perf/util/session.c
> @@ -2095,6 +2095,7 @@ prefetch_event(char *buf, u64 head, size_t mmap_size,
>  	       bool needs_swap, union perf_event *error)
>  {
>  	union perf_event *event;
> +	u16 event_size;
>  
>  	/*
>  	 * Ensure we have enough space remaining to read
> @@ -2107,15 +2108,23 @@ prefetch_event(char *buf, u64 head, size_t mmap_size,
>  	if (needs_swap)
>  		perf_event_header__bswap(&event->header);
>  
> -	if (head + event->header.size <= mmap_size)
> +	event_size = event->header.size;
> +	if (head + event_size <= mmap_size)
>  		return event;
>  
>  	/* We're not fetching the event so swap back again */
>  	if (needs_swap)
>  		perf_event_header__bswap(&event->header);
>  
> -	pr_debug("%s: head=%#" PRIx64 " event->header_size=%#x, mmap_size=%#zx:"
> -		 " fuzzed or compressed perf.data?\n",__func__, head, event->header.size, mmap_size);
> +	/* Check if the event fits into the next mmapped buf. */
> +	if (event_size <= mmap_size - head % page_size) {
> +		/* Remap buf and fetch again. */
> +		return NULL;

Hi Denis,

I tested this and it does fix the issue with a 32bit build. One concern is that the calculation to
see if it will fit in the next map is dependent on the implementation of reader__mmap(). I think it
would be possible for that to change slightly and then you could still get an infinite loop.

But I can't really see a better way to do it, and it's unlikely for reader__mmap() to be modified
to map data in a way to waste part of the buffer so it's probably fine.

Maybe you could extract a function to calculate where the new offset would be in the buffer and share
it between here and reader__mmap(). That would also make it more obvious what the 'head % page_size'
bit is for.

Either way:

Reviewed-by: James Clark <james.clark@arm.com>

> +	}
> +
> +	/* Invalid input. Event size should never exceed mmap_size. */
> +	pr_debug("%s: head=%#" PRIx64 " event->header.size=%#x, mmap_size=%#zx:"
> +		 " fuzzed or compressed perf.data?\n", __func__, head, event_size, mmap_size);
>  
>  	return error;
>  }

Re: [PATCH] perf session: Remap buf if there is no space for event

[Denis Nikitin]

Hi James,

Thanks for your review.

On Thu, Mar 31, 2022 at 7:20 AM James Clark <james.clark@arm.com> wrote:
>
>
>
> On 30/03/2022 04:11, Denis Nikitin wrote:
> > If a perf event doesn't fit into remaining buffer space return NULL to
> > remap buf and fetch the event again.
> > Keep the logic to error out on inadequate input from fuzzing.
> >
> > This fixes perf failing on ChromeOS (with 32b userspace):
> >
> >   $ perf report -v -i perf.data
> >   ...
> >   prefetch_event: head=0x1fffff8 event->header_size=0x30, mmap_size=0x2000000: fuzzed or compressed perf.data?
> >   Error:
> >   failed to process sample
> >
> > Fixes: 57fc032ad643 ("perf session: Avoid infinite loop when seeing invalid header.size")
> > Signed-off-by: Denis Nikitin <denik@chromium.org>
>
> Hi Denis,
>
> I tested this and it does fix the issue with a 32bit build. One concern is that the calculation to
> see if it will fit in the next map is dependent on the implementation of reader__mmap(). I think it
> would be possible for that to change slightly and then you could still get an infinite loop.
>
> But I can't really see a better way to do it, and it's unlikely for reader__mmap() to be modified
> to map data in a way to waste part of the buffer so it's probably fine.
>
> Maybe you could extract a function to calculate where the new offset would be in the buffer and share
> it between here and reader__mmap(). That would also make it more obvious what the 'head % page_size'
> bit is for.

Good point. I will send a separate patch to handle this.

Thanks,
Denis

>
> Either way:
>
> Reviewed-by: James Clark <james.clark@arm.com>
>