[PATCH] Btrfs: fix out of bounds array access while reading extent buffer

[PATCH] Btrfs: fix out of bounds array access while reading extent buffer

[Liu Bo]

There is a cornel case that slip through the checkers in functions
reading extent buffer, ie.

if (start < eb->len) and (start + len > eb->len),
then

a) map_private_extent_buffer() returns immediately because
it's thinking the range spans across two pages,

b) and the checkers in read_extent_buffer(), WARN_ON(start > eb->len)
and WARN_ON(start + len > eb->start + eb->len), both are OK in this
corner case, but it'd actually try to access the eb->pages out of
bounds because of (start + len > eb->len).

The case is found by switching extent inline ref type from shared data
ref to non-shared data ref.

This is adding proper checks in order to avoid invalid memory access,
ie. 'general protection', before it's too late.

Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
---
 fs/btrfs/extent_io.c | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 0aff9b2..d198e87 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -5416,13 +5416,19 @@ void read_extent_buffer(struct extent_buffer *eb, void *dstv,
 	char *dst = (char *)dstv;
 	size_t start_offset = eb->start & ((u64)PAGE_SIZE - 1);
 	unsigned long i = (start_offset + start) >> PAGE_SHIFT;
+	unsigned long num_pages = num_extent_pages(eb->start, eb->len);
 
-	WARN_ON(start > eb->len);
-	WARN_ON(start + len > eb->start + eb->len);
+	if (start + len > eb->len) {
+		WARN(1, KERN_ERR "btrfs bad mapping eb start %llu len %lu, wanted %lu %lu\n",
+		     eb->start, eb->len, start, len);
+		memset(dst, 0, len);
+		return;
+	}
 
 	offset = (start_offset + start) & (PAGE_SIZE - 1);
 
 	while (len > 0) {
+		ASSERT(i < num_pages);
 		page = eb->pages[i];
 
 		cur = min(len, (PAGE_SIZE - offset));
@@ -5491,6 +5497,12 @@ int map_private_extent_buffer(struct extent_buffer *eb, unsigned long start,
 	unsigned long end_i = (start_offset + start + min_len - 1) >>
 		PAGE_SHIFT;
 
+	if (start + min_len > eb->len) {
+		WARN(1, KERN_ERR "btrfs bad mapping eb start %llu len %lu, wanted %lu %lu\n",
+		       eb->start, eb->len, start, min_len);
+		return -EINVAL;
+	}
+
 	if (i != end_i)
 		return 1;
 
@@ -5502,12 +5514,6 @@ int map_private_extent_buffer(struct extent_buffer *eb, unsigned long start,
 		*map_start = ((u64)i << PAGE_SHIFT) - start_offset;
 	}
 
-	if (start + min_len > eb->len) {
-		WARN(1, KERN_ERR "btrfs bad mapping eb start %llu len %lu, wanted %lu %lu\n",
-		       eb->start, eb->len, start, min_len);
-		return -EINVAL;
-	}
-
 	p = eb->pages[i];
 	kaddr = page_address(p);
 	*map = kaddr + offset;
-- 
2.9.4

Re: [PATCH] Btrfs: fix out of bounds array access while reading extent buffer

[Filipe Manana]

On Mon, Aug 7, 2017 at 8:39 PM, Liu Bo <bo.li.liu@oracle.com> wrote:
> There is a cornel case that slip through the checkers in functions
> reading extent buffer, ie.
>
> if (start < eb->len) and (start + len > eb->len),
> then
>
> a) map_private_extent_buffer() returns immediately because
> it's thinking the range spans across two pages,
>
> b) and the checkers in read_extent_buffer(), WARN_ON(start > eb->len)
> and WARN_ON(start + len > eb->start + eb->len), both are OK in this
> corner case, but it'd actually try to access the eb->pages out of
> bounds because of (start + len > eb->len).
>
> The case is found by switching extent inline ref type from shared data
> ref to non-shared data ref.
>
> This is adding proper checks in order to avoid invalid memory access,
> ie. 'general protection', before it's too late.

Hi Bo,

I don't understand these 2 last paragraphs.
How do you fix the invalid memory access? All the change does is make
sure that attempts to read from invalid regions of an extent buffer
result in a warning and returning an error code. Those paragraphs give
the idea that the problem is that some caller is passing a wrong
offset/length pair, however you aren't fixing any caller. can you
clarify?

Also when you say " The case is found by switching extent inline ref
type from shared data ref to non-shared data ref", it gives the idea
this is a deterministic problem that always happens when doing that
switch. If so, can we have a test case?

Also missing the word "fault" after 'general protection'.

Thanks.

>
> Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
> ---
>  fs/btrfs/extent_io.c | 22 ++++++++++++++--------
>  1 file changed, 14 insertions(+), 8 deletions(-)
>
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index 0aff9b2..d198e87 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -5416,13 +5416,19 @@ void read_extent_buffer(struct extent_buffer *eb, void *dstv,
>         char *dst = (char *)dstv;
>         size_t start_offset = eb->start & ((u64)PAGE_SIZE - 1);
>         unsigned long i = (start_offset + start) >> PAGE_SHIFT;
> +       unsigned long num_pages = num_extent_pages(eb->start, eb->len);
>
> -       WARN_ON(start > eb->len);
> -       WARN_ON(start + len > eb->start + eb->len);
> +       if (start + len > eb->len) {
> +               WARN(1, KERN_ERR "btrfs bad mapping eb start %llu len %lu, wanted %lu %lu\n",
> +                    eb->start, eb->len, start, len);
> +               memset(dst, 0, len);
> +               return;
> +       }
>
>         offset = (start_offset + start) & (PAGE_SIZE - 1);
>
>         while (len > 0) {
> +               ASSERT(i < num_pages);
>                 page = eb->pages[i];
>
>                 cur = min(len, (PAGE_SIZE - offset));
> @@ -5491,6 +5497,12 @@ int map_private_extent_buffer(struct extent_buffer *eb, unsigned long start,
>         unsigned long end_i = (start_offset + start + min_len - 1) >>
>                 PAGE_SHIFT;
>
> +       if (start + min_len > eb->len) {
> +               WARN(1, KERN_ERR "btrfs bad mapping eb start %llu len %lu, wanted %lu %lu\n",
> +                      eb->start, eb->len, start, min_len);
> +               return -EINVAL;
> +       }
> +
>         if (i != end_i)
>                 return 1;
>
> @@ -5502,12 +5514,6 @@ int map_private_extent_buffer(struct extent_buffer *eb, unsigned long start,
>                 *map_start = ((u64)i << PAGE_SHIFT) - start_offset;
>         }
>
> -       if (start + min_len > eb->len) {
> -               WARN(1, KERN_ERR "btrfs bad mapping eb start %llu len %lu, wanted %lu %lu\n",
> -                      eb->start, eb->len, start, min_len);
> -               return -EINVAL;
> -       }
> -
>         p = eb->pages[i];
>         kaddr = page_address(p);
>         *map = kaddr + offset;
> --
> 2.9.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Filipe David Manana,

“Whether you think you can, or you think you can't — you're right.”

Re: [PATCH] Btrfs: fix out of bounds array access while reading extent buffer

[Liu Bo]

Hi Filipe,

On Tue, Aug 08, 2017 at 09:47:21AM +0100, Filipe Manana wrote:
> On Mon, Aug 7, 2017 at 8:39 PM, Liu Bo <bo.li.liu@oracle.com> wrote:
> > There is a cornel case that slip through the checkers in functions
> > reading extent buffer, ie.
> >
> > if (start < eb->len) and (start + len > eb->len),
> > then
> >
> > a) map_private_extent_buffer() returns immediately because
> > it's thinking the range spans across two pages,
> >
> > b) and the checkers in read_extent_buffer(), WARN_ON(start > eb->len)
> > and WARN_ON(start + len > eb->start + eb->len), both are OK in this
> > corner case, but it'd actually try to access the eb->pages out of
> > bounds because of (start + len > eb->len).
> >
> > The case is found by switching extent inline ref type from shared data
> > ref to non-shared data ref.
> >
> > This is adding proper checks in order to avoid invalid memory access,
> > ie. 'general protection', before it's too late.
> 
> Hi Bo,
> 
> I don't understand these 2 last paragraphs.
> How do you fix the invalid memory access? All the change does is make
> sure that attempts to read from invalid regions of an extent buffer
> result in a warning and returning an error code. Those paragraphs give
> the idea that the problem is that some caller is passing a wrong
> offset/length pair, however you aren't fixing any caller. can you
> clarify?
>

I see your doubt, that wrong offset/length pair comes from one of
btrfs_setget helpers, eg. btrfs_extent_data_ref_root.

The invalid memory access happens when the pointer it's using to
access fields in "struct btrfs_extent_data_ref" is actually a "struct
btrfs_shared_data_ref", this is caused by switching those types.

So the offset/length pair is correct from btrfs_setget helper's point
of view, but it's just using the wrong helper due to a wrong ref type
(in v2 this'll be added to the commit log to clarity).

> Also when you say " The case is found by switching extent inline ref
> type from shared data ref to non-shared data ref", it gives the idea
> this is a deterministic problem that always happens when doing that
> switch. If so, can we have a test case?
>

It is deterministic, but it depends on patching btrfs-corrupt-block
and last time I posted a corrupt-block related case, I was told that
tool is gonna change a lot and maybe get deleted in the near future,
so it's not yet suitable for fstests cases.

> Also missing the word "fault" after 'general protection'.
>

Yeah, there is a 'fault', the full text is
"general protection fault: 0000 [#1] SMP KASAN",
will update.

Thanks,

-liubo

> Thanks.
> 
> >
> > Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
> > ---
> >  fs/btrfs/extent_io.c | 22 ++++++++++++++--------
> >  1 file changed, 14 insertions(+), 8 deletions(-)
> >
> > diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> > index 0aff9b2..d198e87 100644
> > --- a/fs/btrfs/extent_io.c
> > +++ b/fs/btrfs/extent_io.c
> > @@ -5416,13 +5416,19 @@ void read_extent_buffer(struct extent_buffer *eb, void *dstv,
> >         char *dst = (char *)dstv;
> >         size_t start_offset = eb->start & ((u64)PAGE_SIZE - 1);
> >         unsigned long i = (start_offset + start) >> PAGE_SHIFT;
> > +       unsigned long num_pages = num_extent_pages(eb->start, eb->len);
> >
> > -       WARN_ON(start > eb->len);
> > -       WARN_ON(start + len > eb->start + eb->len);
> > +       if (start + len > eb->len) {
> > +               WARN(1, KERN_ERR "btrfs bad mapping eb start %llu len %lu, wanted %lu %lu\n",
> > +                    eb->start, eb->len, start, len);
> > +               memset(dst, 0, len);
> > +               return;
> > +       }
> >
> >         offset = (start_offset + start) & (PAGE_SIZE - 1);
> >
> >         while (len > 0) {
> > +               ASSERT(i < num_pages);
> >                 page = eb->pages[i];
> >
> >                 cur = min(len, (PAGE_SIZE - offset));
> > @@ -5491,6 +5497,12 @@ int map_private_extent_buffer(struct extent_buffer *eb, unsigned long start,
> >         unsigned long end_i = (start_offset + start + min_len - 1) >>
> >                 PAGE_SHIFT;
> >
> > +       if (start + min_len > eb->len) {
> > +               WARN(1, KERN_ERR "btrfs bad mapping eb start %llu len %lu, wanted %lu %lu\n",
> > +                      eb->start, eb->len, start, min_len);
> > +               return -EINVAL;
> > +       }
> > +
> >         if (i != end_i)
> >                 return 1;
> >
> > @@ -5502,12 +5514,6 @@ int map_private_extent_buffer(struct extent_buffer *eb, unsigned long start,
> >                 *map_start = ((u64)i << PAGE_SHIFT) - start_offset;
> >         }
> >
> > -       if (start + min_len > eb->len) {
> > -               WARN(1, KERN_ERR "btrfs bad mapping eb start %llu len %lu, wanted %lu %lu\n",
> > -                      eb->start, eb->len, start, min_len);
> > -               return -EINVAL;
> > -       }
> > -
> >         p = eb->pages[i];
> >         kaddr = page_address(p);
> >         *map = kaddr + offset;
> > --
> > 2.9.4
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 
> 
> -- 
> Filipe David Manana,
> 
> “Whether you think you can, or you think you can't — you're right.”
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Btrfs: fix out of bounds array access while reading extent buffer

[Filipe Manana]

On Tue, Aug 8, 2017 at 6:05 PM, Liu Bo <bo.li.liu@oracle.com> wrote:
> Hi Filipe,
>
> On Tue, Aug 08, 2017 at 09:47:21AM +0100, Filipe Manana wrote:
>> On Mon, Aug 7, 2017 at 8:39 PM, Liu Bo <bo.li.liu@oracle.com> wrote:
>> > There is a cornel case that slip through the checkers in functions
>> > reading extent buffer, ie.
>> >
>> > if (start < eb->len) and (start + len > eb->len),
>> > then
>> >
>> > a) map_private_extent_buffer() returns immediately because
>> > it's thinking the range spans across two pages,
>> >
>> > b) and the checkers in read_extent_buffer(), WARN_ON(start > eb->len)
>> > and WARN_ON(start + len > eb->start + eb->len), both are OK in this
>> > corner case, but it'd actually try to access the eb->pages out of
>> > bounds because of (start + len > eb->len).
>> >
>> > The case is found by switching extent inline ref type from shared data
>> > ref to non-shared data ref.
>> >
>> > This is adding proper checks in order to avoid invalid memory access,
>> > ie. 'general protection', before it's too late.
>>
>> Hi Bo,
>>
>> I don't understand these 2 last paragraphs.
>> How do you fix the invalid memory access? All the change does is make
>> sure that attempts to read from invalid regions of an extent buffer
>> result in a warning and returning an error code. Those paragraphs give
>> the idea that the problem is that some caller is passing a wrong
>> offset/length pair, however you aren't fixing any caller. can you
>> clarify?
>>
>
> I see your doubt, that wrong offset/length pair comes from one of
> btrfs_setget helpers, eg. btrfs_extent_data_ref_root.
>
> The invalid memory access happens when the pointer it's using to
> access fields in "struct btrfs_extent_data_ref" is actually a "struct
> btrfs_shared_data_ref", this is caused by switching those types.
>
> So the offset/length pair is correct from btrfs_setget helper's point
> of view, but it's just using the wrong helper due to a wrong ref type
> (in v2 this'll be added to the commit log to clarity).
>
>> Also when you say " The case is found by switching extent inline ref
>> type from shared data ref to non-shared data ref", it gives the idea
>> this is a deterministic problem that always happens when doing that
>> switch. If so, can we have a test case?
>>
>
> It is deterministic, but it depends on patching btrfs-corrupt-block
> and last time I posted a corrupt-block related case, I was told that
> tool is gonna change a lot and maybe get deleted in the near future,
> so it's not yet suitable for fstests cases.

Ok, so this only happens if the metadata is corrupted. It wasn't clear
for me from the change log.

Thanks.

>
>> Also missing the word "fault" after 'general protection'.
>>
>
> Yeah, there is a 'fault', the full text is
> "general protection fault: 0000 [#1] SMP KASAN",
> will update.
>
> Thanks,
>
> -liubo
>
>> Thanks.
>>
>> >
>> > Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
>> > ---
>> >  fs/btrfs/extent_io.c | 22 ++++++++++++++--------
>> >  1 file changed, 14 insertions(+), 8 deletions(-)
>> >
>> > diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
>> > index 0aff9b2..d198e87 100644
>> > --- a/fs/btrfs/extent_io.c
>> > +++ b/fs/btrfs/extent_io.c
>> > @@ -5416,13 +5416,19 @@ void read_extent_buffer(struct extent_buffer *eb, void *dstv,
>> >         char *dst = (char *)dstv;
>> >         size_t start_offset = eb->start & ((u64)PAGE_SIZE - 1);
>> >         unsigned long i = (start_offset + start) >> PAGE_SHIFT;
>> > +       unsigned long num_pages = num_extent_pages(eb->start, eb->len);
>> >
>> > -       WARN_ON(start > eb->len);
>> > -       WARN_ON(start + len > eb->start + eb->len);
>> > +       if (start + len > eb->len) {
>> > +               WARN(1, KERN_ERR "btrfs bad mapping eb start %llu len %lu, wanted %lu %lu\n",
>> > +                    eb->start, eb->len, start, len);
>> > +               memset(dst, 0, len);
>> > +               return;
>> > +       }
>> >
>> >         offset = (start_offset + start) & (PAGE_SIZE - 1);
>> >
>> >         while (len > 0) {
>> > +               ASSERT(i < num_pages);
>> >                 page = eb->pages[i];
>> >
>> >                 cur = min(len, (PAGE_SIZE - offset));
>> > @@ -5491,6 +5497,12 @@ int map_private_extent_buffer(struct extent_buffer *eb, unsigned long start,
>> >         unsigned long end_i = (start_offset + start + min_len - 1) >>
>> >                 PAGE_SHIFT;
>> >
>> > +       if (start + min_len > eb->len) {
>> > +               WARN(1, KERN_ERR "btrfs bad mapping eb start %llu len %lu, wanted %lu %lu\n",
>> > +                      eb->start, eb->len, start, min_len);
>> > +               return -EINVAL;
>> > +       }
>> > +
>> >         if (i != end_i)
>> >                 return 1;
>> >
>> > @@ -5502,12 +5514,6 @@ int map_private_extent_buffer(struct extent_buffer *eb, unsigned long start,
>> >                 *map_start = ((u64)i << PAGE_SHIFT) - start_offset;
>> >         }
>> >
>> > -       if (start + min_len > eb->len) {
>> > -               WARN(1, KERN_ERR "btrfs bad mapping eb start %llu len %lu, wanted %lu %lu\n",
>> > -                      eb->start, eb->len, start, min_len);
>> > -               return -EINVAL;
>> > -       }
>> > -
>> >         p = eb->pages[i];
>> >         kaddr = page_address(p);
>> >         *map = kaddr + offset;
>> > --
>> > 2.9.4
>> >
>> > --
>> > To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
>> > the body of a message to majordomo@vger.kernel.org
>> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>>
>>
>> --
>> Filipe David Manana,
>>
>> “Whether you think you can, or you think you can't — you're right.”
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Filipe David Manana,

“Whether you think you can, or you think you can't — you're right.”

[PATCH v2] Btrfs: fix out of bounds array access while reading extent buffer

[Liu Bo]

There is a cornel case that slip through the checkers in functions
reading extent buffer, ie.

if (start < eb->len) and (start + len > eb->len),
then

a) map_private_extent_buffer() returns immediately because
it's thinking the range spans across two pages,

b) and the checkers in read_extent_buffer(), WARN_ON(start > eb->len)
and WARN_ON(start + len > eb->start + eb->len), both are OK in this
corner case, but it'd actually try to access the eb->pages out of
bounds because of (start + len > eb->len).

The case is found by switching extent inline ref type from shared data
ref to non-shared data ref, which is a kind of metadata corruption.

It'd use the wrong helper to access the eb,
eg. btrfs_extent_data_ref_root(eb, ref) is used but the %ref passing
here is "struct btrfs_shared_data_ref".  And if the extent item
happens to be the first item in the eb, then offset/length will get
over eb->len which ends up an invalid memory access.

This is adding proper checks in order to avoid invalid memory access,
ie. 'general protection fault', before it's too late.

Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
---

v2: Improve the commit log to clarify that this can only happen if
metadata is corrupted.

 fs/btrfs/extent_io.c | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 0aff9b2..d198e87 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -5416,13 +5416,19 @@ void read_extent_buffer(struct extent_buffer *eb, void *dstv,
 	char *dst = (char *)dstv;
 	size_t start_offset = eb->start & ((u64)PAGE_SIZE - 1);
 	unsigned long i = (start_offset + start) >> PAGE_SHIFT;
+	unsigned long num_pages = num_extent_pages(eb->start, eb->len);
 
-	WARN_ON(start > eb->len);
-	WARN_ON(start + len > eb->start + eb->len);
+	if (start + len > eb->len) {
+		WARN(1, KERN_ERR "btrfs bad mapping eb start %llu len %lu, wanted %lu %lu\n",
+		     eb->start, eb->len, start, len);
+		memset(dst, 0, len);
+		return;
+	}
 
 	offset = (start_offset + start) & (PAGE_SIZE - 1);
 
 	while (len > 0) {
+		ASSERT(i < num_pages);
 		page = eb->pages[i];
 
 		cur = min(len, (PAGE_SIZE - offset));
@@ -5491,6 +5497,12 @@ int map_private_extent_buffer(struct extent_buffer *eb, unsigned long start,
 	unsigned long end_i = (start_offset + start + min_len - 1) >>
 		PAGE_SHIFT;
 
+	if (start + min_len > eb->len) {
+		WARN(1, KERN_ERR "btrfs bad mapping eb start %llu len %lu, wanted %lu %lu\n",
+		       eb->start, eb->len, start, min_len);
+		return -EINVAL;
+	}
+
 	if (i != end_i)
 		return 1;
 
@@ -5502,12 +5514,6 @@ int map_private_extent_buffer(struct extent_buffer *eb, unsigned long start,
 		*map_start = ((u64)i << PAGE_SHIFT) - start_offset;
 	}
 
-	if (start + min_len > eb->len) {
-		WARN(1, KERN_ERR "btrfs bad mapping eb start %llu len %lu, wanted %lu %lu\n",
-		       eb->start, eb->len, start, min_len);
-		return -EINVAL;
-	}
-
 	p = eb->pages[i];
 	kaddr = page_address(p);
 	*map = kaddr + offset;
-- 
2.9.4

Re: [PATCH v2] Btrfs: fix out of bounds array access while reading extent buffer

[Filipe Manana]

On Wed, Aug 9, 2017 at 5:31 PM, Liu Bo <bo.li.liu@oracle.com> wrote:
> There is a cornel case that slip through the checkers in functions
> reading extent buffer, ie.
>
> if (start < eb->len) and (start + len > eb->len),
> then
>
> a) map_private_extent_buffer() returns immediately because
> it's thinking the range spans across two pages,
>
> b) and the checkers in read_extent_buffer(), WARN_ON(start > eb->len)
> and WARN_ON(start + len > eb->start + eb->len), both are OK in this
> corner case, but it'd actually try to access the eb->pages out of
> bounds because of (start + len > eb->len).
>
> The case is found by switching extent inline ref type from shared data
> ref to non-shared data ref, which is a kind of metadata corruption.
>
> It'd use the wrong helper to access the eb,
> eg. btrfs_extent_data_ref_root(eb, ref) is used but the %ref passing
> here is "struct btrfs_shared_data_ref".  And if the extent item
> happens to be the first item in the eb, then offset/length will get
> over eb->len which ends up an invalid memory access.
>
> This is adding proper checks in order to avoid invalid memory access,
> ie. 'general protection fault', before it's too late.
>
> Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
Reviewed-by: Filipe Manana <fdmanana@suse.com>

> ---
>
> v2: Improve the commit log to clarify that this can only happen if
> metadata is corrupted.

Thanks for adding the clarification!

>
>  fs/btrfs/extent_io.c | 22 ++++++++++++++--------
>  1 file changed, 14 insertions(+), 8 deletions(-)
>
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index 0aff9b2..d198e87 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -5416,13 +5416,19 @@ void read_extent_buffer(struct extent_buffer *eb, void *dstv,
>         char *dst = (char *)dstv;
>         size_t start_offset = eb->start & ((u64)PAGE_SIZE - 1);
>         unsigned long i = (start_offset + start) >> PAGE_SHIFT;
> +       unsigned long num_pages = num_extent_pages(eb->start, eb->len);
>
> -       WARN_ON(start > eb->len);
> -       WARN_ON(start + len > eb->start + eb->len);
> +       if (start + len > eb->len) {
> +               WARN(1, KERN_ERR "btrfs bad mapping eb start %llu len %lu, wanted %lu %lu\n",
> +                    eb->start, eb->len, start, len);
> +               memset(dst, 0, len);
> +               return;
> +       }
>
>         offset = (start_offset + start) & (PAGE_SIZE - 1);
>
>         while (len > 0) {
> +               ASSERT(i < num_pages);
>                 page = eb->pages[i];
>
>                 cur = min(len, (PAGE_SIZE - offset));
> @@ -5491,6 +5497,12 @@ int map_private_extent_buffer(struct extent_buffer *eb, unsigned long start,
>         unsigned long end_i = (start_offset + start + min_len - 1) >>
>                 PAGE_SHIFT;
>
> +       if (start + min_len > eb->len) {
> +               WARN(1, KERN_ERR "btrfs bad mapping eb start %llu len %lu, wanted %lu %lu\n",
> +                      eb->start, eb->len, start, min_len);
> +               return -EINVAL;
> +       }
> +
>         if (i != end_i)
>                 return 1;
>
> @@ -5502,12 +5514,6 @@ int map_private_extent_buffer(struct extent_buffer *eb, unsigned long start,
>                 *map_start = ((u64)i << PAGE_SHIFT) - start_offset;
>         }
>
> -       if (start + min_len > eb->len) {
> -               WARN(1, KERN_ERR "btrfs bad mapping eb start %llu len %lu, wanted %lu %lu\n",
> -                      eb->start, eb->len, start, min_len);
> -               return -EINVAL;
> -       }
> -
>         p = eb->pages[i];
>         kaddr = page_address(p);
>         *map = kaddr + offset;
> --
> 2.9.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Filipe David Manana,

“Whether you think you can, or you think you can't — you're right.”

Re: [PATCH v2] Btrfs: fix out of bounds array access while reading extent buffer

[Filipe Manana]

On Wed, Aug 9, 2017 at 5:31 PM, Liu Bo <bo.li.liu@oracle.com> wrote:
> There is a cornel case that slip through the checkers in functions
> reading extent buffer, ie.
>
> if (start < eb->len) and (start + len > eb->len),
> then
>
> a) map_private_extent_buffer() returns immediately because
> it's thinking the range spans across two pages,
>
> b) and the checkers in read_extent_buffer(), WARN_ON(start > eb->len)
> and WARN_ON(start + len > eb->start + eb->len), both are OK in this
> corner case, but it'd actually try to access the eb->pages out of
> bounds because of (start + len > eb->len).
>
> The case is found by switching extent inline ref type from shared data
> ref to non-shared data ref, which is a kind of metadata corruption.
>
> It'd use the wrong helper to access the eb,
> eg. btrfs_extent_data_ref_root(eb, ref) is used but the %ref passing
> here is "struct btrfs_shared_data_ref".  And if the extent item
> happens to be the first item in the eb, then offset/length will get
> over eb->len which ends up an invalid memory access.
>
> This is adding proper checks in order to avoid invalid memory access,
> ie. 'general protection fault', before it's too late.
>
> Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
> ---
>
> v2: Improve the commit log to clarify that this can only happen if
> metadata is corrupted.
>
>  fs/btrfs/extent_io.c | 22 ++++++++++++++--------
>  1 file changed, 14 insertions(+), 8 deletions(-)
>
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index 0aff9b2..d198e87 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -5416,13 +5416,19 @@ void read_extent_buffer(struct extent_buffer *eb, void *dstv,
>         char *dst = (char *)dstv;
>         size_t start_offset = eb->start & ((u64)PAGE_SIZE - 1);
>         unsigned long i = (start_offset + start) >> PAGE_SHIFT;
> +       unsigned long num_pages = num_extent_pages(eb->start, eb->len);

Forgot to note that if CONFIG_BTRFS_ASSERT is not set, this variable
is unused and the compiler and static checkers will emit a warning.
Other than that all looks fine. Thanks.

>
> -       WARN_ON(start > eb->len);
> -       WARN_ON(start + len > eb->start + eb->len);
> +       if (start + len > eb->len) {
> +               WARN(1, KERN_ERR "btrfs bad mapping eb start %llu len %lu, wanted %lu %lu\n",
> +                    eb->start, eb->len, start, len);
> +               memset(dst, 0, len);
> +               return;
> +       }
>
>         offset = (start_offset + start) & (PAGE_SIZE - 1);
>
>         while (len > 0) {
> +               ASSERT(i < num_pages);
>                 page = eb->pages[i];
>
>                 cur = min(len, (PAGE_SIZE - offset));
> @@ -5491,6 +5497,12 @@ int map_private_extent_buffer(struct extent_buffer *eb, unsigned long start,
>         unsigned long end_i = (start_offset + start + min_len - 1) >>
>                 PAGE_SHIFT;
>
> +       if (start + min_len > eb->len) {
> +               WARN(1, KERN_ERR "btrfs bad mapping eb start %llu len %lu, wanted %lu %lu\n",
> +                      eb->start, eb->len, start, min_len);
> +               return -EINVAL;
> +       }
> +
>         if (i != end_i)
>                 return 1;
>
> @@ -5502,12 +5514,6 @@ int map_private_extent_buffer(struct extent_buffer *eb, unsigned long start,
>                 *map_start = ((u64)i << PAGE_SHIFT) - start_offset;
>         }
>
> -       if (start + min_len > eb->len) {
> -               WARN(1, KERN_ERR "btrfs bad mapping eb start %llu len %lu, wanted %lu %lu\n",
> -                      eb->start, eb->len, start, min_len);
> -               return -EINVAL;
> -       }
> -
>         p = eb->pages[i];
>         kaddr = page_address(p);
>         *map = kaddr + offset;
> --
> 2.9.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Filipe David Manana,

“Whether you think you can, or you think you can't — you're right.”

Re: [PATCH v2] Btrfs: fix out of bounds array access while reading extent buffer

[Liu Bo]

On Wed, Aug 09, 2017 at 06:40:04PM +0100, Filipe Manana wrote:
> On Wed, Aug 9, 2017 at 5:31 PM, Liu Bo <bo.li.liu@oracle.com> wrote:
> > There is a cornel case that slip through the checkers in functions
> > reading extent buffer, ie.
> >
> > if (start < eb->len) and (start + len > eb->len),
> > then
> >
> > a) map_private_extent_buffer() returns immediately because
> > it's thinking the range spans across two pages,
> >
> > b) and the checkers in read_extent_buffer(), WARN_ON(start > eb->len)
> > and WARN_ON(start + len > eb->start + eb->len), both are OK in this
> > corner case, but it'd actually try to access the eb->pages out of
> > bounds because of (start + len > eb->len).
> >
> > The case is found by switching extent inline ref type from shared data
> > ref to non-shared data ref, which is a kind of metadata corruption.
> >
> > It'd use the wrong helper to access the eb,
> > eg. btrfs_extent_data_ref_root(eb, ref) is used but the %ref passing
> > here is "struct btrfs_shared_data_ref".  And if the extent item
> > happens to be the first item in the eb, then offset/length will get
> > over eb->len which ends up an invalid memory access.
> >
> > This is adding proper checks in order to avoid invalid memory access,
> > ie. 'general protection fault', before it's too late.
> >
> > Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
> > ---
> >
> > v2: Improve the commit log to clarify that this can only happen if
> > metadata is corrupted.
> >
> >  fs/btrfs/extent_io.c | 22 ++++++++++++++--------
> >  1 file changed, 14 insertions(+), 8 deletions(-)
> >
> > diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> > index 0aff9b2..d198e87 100644
> > --- a/fs/btrfs/extent_io.c
> > +++ b/fs/btrfs/extent_io.c
> > @@ -5416,13 +5416,19 @@ void read_extent_buffer(struct extent_buffer *eb, void *dstv,
> >         char *dst = (char *)dstv;
> >         size_t start_offset = eb->start & ((u64)PAGE_SIZE - 1);
> >         unsigned long i = (start_offset + start) >> PAGE_SHIFT;
> > +       unsigned long num_pages = num_extent_pages(eb->start, eb->len);
> 
> Forgot to note that if CONFIG_BTRFS_ASSERT is not set, this variable
> is unused and the compiler and static checkers will emit a warning.
> Other than that all looks fine. Thanks.
>

True, then lets remove it, I don't think we could reach that ASSERT if
(start + len > eb->len).

thanks,

-liubo

> >
> > -       WARN_ON(start > eb->len);
> > -       WARN_ON(start + len > eb->start + eb->len);
> > +       if (start + len > eb->len) {
> > +               WARN(1, KERN_ERR "btrfs bad mapping eb start %llu len %lu, wanted %lu %lu\n",
> > +                    eb->start, eb->len, start, len);
> > +               memset(dst, 0, len);
> > +               return;
> > +       }
> >
> >         offset = (start_offset + start) & (PAGE_SIZE - 1);
> >
> >         while (len > 0) {
> > +               ASSERT(i < num_pages);
> >                 page = eb->pages[i];
> >
> >                 cur = min(len, (PAGE_SIZE - offset));
> > @@ -5491,6 +5497,12 @@ int map_private_extent_buffer(struct extent_buffer *eb, unsigned long start,
> >         unsigned long end_i = (start_offset + start + min_len - 1) >>
> >                 PAGE_SHIFT;
> >
> > +       if (start + min_len > eb->len) {
> > +               WARN(1, KERN_ERR "btrfs bad mapping eb start %llu len %lu, wanted %lu %lu\n",
> > +                      eb->start, eb->len, start, min_len);
> > +               return -EINVAL;
> > +       }
> > +
> >         if (i != end_i)
> >                 return 1;
> >
> > @@ -5502,12 +5514,6 @@ int map_private_extent_buffer(struct extent_buffer *eb, unsigned long start,
> >                 *map_start = ((u64)i << PAGE_SHIFT) - start_offset;
> >         }
> >
> > -       if (start + min_len > eb->len) {
> > -               WARN(1, KERN_ERR "btrfs bad mapping eb start %llu len %lu, wanted %lu %lu\n",
> > -                      eb->start, eb->len, start, min_len);
> > -               return -EINVAL;
> > -       }
> > -
> >         p = eb->pages[i];
> >         kaddr = page_address(p);
> >         *map = kaddr + offset;
> > --
> > 2.9.4
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 
> 
> -- 
> Filipe David Manana,
> 
> “Whether you think you can, or you think you can't — you're right.”
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3] Btrfs: fix out of bounds array access while reading extent buffer

[Liu Bo]

There is a cornel case that slip through the checkers in functions
reading extent buffer, ie.

if (start < eb->len) and (start + len > eb->len),
then

a) map_private_extent_buffer() returns immediately because
it's thinking the range spans across two pages,

b) and the checkers in read_extent_buffer(), WARN_ON(start > eb->len)
and WARN_ON(start + len > eb->start + eb->len), both are OK in this
corner case, but it'd actually try to access the eb->pages out of
bounds because of (start + len > eb->len).

The case is found by switching extent inline ref type from shared data
ref to non-shared data ref, which is a kind of metadata corruption.

It'd use the wrong helper to access the eb,
eg. btrfs_extent_data_ref_root(eb, ref) is used but the %ref passing
here is "struct btrfs_shared_data_ref".  And if the extent item
happens to be the first item in the eb, then offset/length will get
over eb->len which ends up an invalid memory access.

This is adding proper checks in order to avoid invalid memory access,
ie. 'general protection fault', before it's too late.

Reviewed-by: Filipe Manana <fdmanana@suse.com>
Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
---

v3: Remove the unnecessary ASSERT and num_pages.

v2: Improve the commit log to clarify that this can only happen if
metadata is corrupted.

 fs/btrfs/extent_io.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 0aff9b2..e6c6853 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -5417,8 +5417,12 @@ void read_extent_buffer(struct extent_buffer *eb, void *dstv,
 	size_t start_offset = eb->start & ((u64)PAGE_SIZE - 1);
 	unsigned long i = (start_offset + start) >> PAGE_SHIFT;
 
-	WARN_ON(start > eb->len);
-	WARN_ON(start + len > eb->start + eb->len);
+	if (start + len > eb->len) {
+		WARN(1, KERN_ERR "btrfs bad mapping eb start %llu len %lu, wanted %lu %lu\n",
+		     eb->start, eb->len, start, len);
+		memset(dst, 0, len);
+		return;
+	}
 
 	offset = (start_offset + start) & (PAGE_SIZE - 1);
 
@@ -5491,6 +5495,12 @@ int map_private_extent_buffer(struct extent_buffer *eb, unsigned long start,
 	unsigned long end_i = (start_offset + start + min_len - 1) >>
 		PAGE_SHIFT;
 
+	if (start + min_len > eb->len) {
+		WARN(1, KERN_ERR "btrfs bad mapping eb start %llu len %lu, wanted %lu %lu\n",
+		       eb->start, eb->len, start, min_len);
+		return -EINVAL;
+	}
+
 	if (i != end_i)
 		return 1;
 
@@ -5502,12 +5512,6 @@ int map_private_extent_buffer(struct extent_buffer *eb, unsigned long start,
 		*map_start = ((u64)i << PAGE_SHIFT) - start_offset;
 	}
 
-	if (start + min_len > eb->len) {
-		WARN(1, KERN_ERR "btrfs bad mapping eb start %llu len %lu, wanted %lu %lu\n",
-		       eb->start, eb->len, start, min_len);
-		return -EINVAL;
-	}
-
 	p = eb->pages[i];
 	kaddr = page_address(p);
 	*map = kaddr + offset;
-- 
2.9.4

Re: [PATCH v3] Btrfs: fix out of bounds array access while reading extent buffer

[Hugo Mills]

[-- Attachment #1: Type: text/plain, Size: 3435 bytes --]

On Wed, Aug 09, 2017 at 11:10:16AM -0600, Liu Bo wrote:
> There is a cornel case that slip through the checkers in functions
             ^^^^^^ corner

   Sorry, that's been bugging me every time it goes past. A cornel is
a kind of tree, apparently.

   Hugo.

> reading extent buffer, ie.
> 
> if (start < eb->len) and (start + len > eb->len),
> then
> 
> a) map_private_extent_buffer() returns immediately because
> it's thinking the range spans across two pages,
> 
> b) and the checkers in read_extent_buffer(), WARN_ON(start > eb->len)
> and WARN_ON(start + len > eb->start + eb->len), both are OK in this
> corner case, but it'd actually try to access the eb->pages out of
> bounds because of (start + len > eb->len).
> 
> The case is found by switching extent inline ref type from shared data
> ref to non-shared data ref, which is a kind of metadata corruption.
> 
> It'd use the wrong helper to access the eb,
> eg. btrfs_extent_data_ref_root(eb, ref) is used but the %ref passing
> here is "struct btrfs_shared_data_ref".  And if the extent item
> happens to be the first item in the eb, then offset/length will get
> over eb->len which ends up an invalid memory access.
> 
> This is adding proper checks in order to avoid invalid memory access,
> ie. 'general protection fault', before it's too late.
> 
> Reviewed-by: Filipe Manana <fdmanana@suse.com>
> Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
> ---
> 
> v3: Remove the unnecessary ASSERT and num_pages.
> 
> v2: Improve the commit log to clarify that this can only happen if
> metadata is corrupted.
> 
>  fs/btrfs/extent_io.c | 20 ++++++++++++--------
>  1 file changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index 0aff9b2..e6c6853 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -5417,8 +5417,12 @@ void read_extent_buffer(struct extent_buffer *eb, void *dstv,
>  	size_t start_offset = eb->start & ((u64)PAGE_SIZE - 1);
>  	unsigned long i = (start_offset + start) >> PAGE_SHIFT;
>  
> -	WARN_ON(start > eb->len);
> -	WARN_ON(start + len > eb->start + eb->len);
> +	if (start + len > eb->len) {
> +		WARN(1, KERN_ERR "btrfs bad mapping eb start %llu len %lu, wanted %lu %lu\n",
> +		     eb->start, eb->len, start, len);
> +		memset(dst, 0, len);
> +		return;
> +	}
>  
>  	offset = (start_offset + start) & (PAGE_SIZE - 1);
>  
> @@ -5491,6 +5495,12 @@ int map_private_extent_buffer(struct extent_buffer *eb, unsigned long start,
>  	unsigned long end_i = (start_offset + start + min_len - 1) >>
>  		PAGE_SHIFT;
>  
> +	if (start + min_len > eb->len) {
> +		WARN(1, KERN_ERR "btrfs bad mapping eb start %llu len %lu, wanted %lu %lu\n",
> +		       eb->start, eb->len, start, min_len);
> +		return -EINVAL;
> +	}
> +
>  	if (i != end_i)
>  		return 1;
>  
> @@ -5502,12 +5512,6 @@ int map_private_extent_buffer(struct extent_buffer *eb, unsigned long start,
>  		*map_start = ((u64)i << PAGE_SHIFT) - start_offset;
>  	}
>  
> -	if (start + min_len > eb->len) {
> -		WARN(1, KERN_ERR "btrfs bad mapping eb start %llu len %lu, wanted %lu %lu\n",
> -		       eb->start, eb->len, start, min_len);
> -		return -EINVAL;
> -	}
> -
>  	p = eb->pages[i];
>  	kaddr = page_address(p);
>  	*map = kaddr + offset;

-- 
Hugo Mills             | Jazz is the sort of music where no-one plays
hugo@... carfax.org.uk | anything the same way once.
http://carfax.org.uk/  |
PGP: E2AB1DE4          |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v3] Btrfs: fix out of bounds array access while reading extent buffer

[Duncan]

Hugo Mills posted on Wed, 09 Aug 2017 18:15:29 +0000 as excerpted:

> On Wed, Aug 09, 2017 at 11:10:16AM -0600, Liu Bo wrote:
>> There is a cornel case that slip through the checkers in functions
>              ^^^^^^ corner
> 
>    Sorry, that's been bugging me every time it goes past. A cornel is
> a kind of tree, apparently.

LOL!  Me too but likely more so because I actually live in Cornell 
Condominiums so seeing "cornel" in this context is a rather confusing 
context switch, to say the least!  I let it slip the first time as I 
never expected it to make it to a v3, but...

-- 
Duncan - List replies preferred.   No HTML msgs.
"Every nonfree program has a lord, a master --
and if you use the program, he is your master."  Richard Stallman

Re: [PATCH] Btrfs: fix out of bounds array access while reading extent buffer

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2045 bytes --]

Hi Liu,

[auto build test WARNING on v4.13-rc4]
[also build test WARNING on next-20170811]
[cannot apply to btrfs/next]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Liu-Bo/Btrfs-fix-out-of-bounds-array-access-while-reading-extent-buffer/20170810-235607
config: x86_64-randconfig-a0-08120433 (attached as .config)
compiler: gcc-4.4 (Debian 4.4.7-8) 4.4.7
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All warnings (new ones prefixed by >>):

   fs//btrfs/extent_io.c: In function 'read_extent_buffer':
>> fs//btrfs/extent_io.c:5419: warning: unused variable 'num_pages'

vim +/num_pages +5419 fs//btrfs/extent_io.c

  5407	
  5408	void read_extent_buffer(struct extent_buffer *eb, void *dstv,
  5409				unsigned long start,
  5410				unsigned long len)
  5411	{
  5412		size_t cur;
  5413		size_t offset;
  5414		struct page *page;
  5415		char *kaddr;
  5416		char *dst = (char *)dstv;
  5417		size_t start_offset = eb->start & ((u64)PAGE_SIZE - 1);
  5418		unsigned long i = (start_offset + start) >> PAGE_SHIFT;
> 5419		unsigned long num_pages = num_extent_pages(eb->start, eb->len);
  5420	
  5421		if (start + len > eb->len) {
  5422			WARN(1, KERN_ERR "btrfs bad mapping eb start %llu len %lu, wanted %lu %lu\n",
  5423			     eb->start, eb->len, start, len);
  5424			memset(dst, 0, len);
  5425			return;
  5426		}
  5427	
  5428		offset = (start_offset + start) & (PAGE_SIZE - 1);
  5429	
  5430		while (len > 0) {
  5431			ASSERT(i < num_pages);
  5432			page = eb->pages[i];
  5433	
  5434			cur = min(len, (PAGE_SIZE - offset));
  5435			kaddr = page_address(page);
  5436			memcpy(dst, kaddr + offset, cur);
  5437	
  5438			dst += cur;
  5439			len -= cur;
  5440			offset = 0;
  5441			i++;
  5442		}
  5443	}
  5444	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 29010 bytes --]

[PATCH] btrfs: fix out of bounds array access while reading extent buffer

[Young Xiao]

There is a corner case that slips through the checkers in functions
reading extent buffer, ie.

if (start < eb->len) and (start + len > eb->len), then:
the checkers in read_extent_buffer_to_user(), and memcmp_extent_buffer()
WARN_ON(start > eb->len) and WARN_ON(start + len > eb->start + eb->len),
both are OK in this corner case, but it'd actually try to access the eb->pages
out of bounds because of (start + len > eb->len).

This is adding proper checks in order to avoid invalid memory access,
ie. 'general protection fault', before it's too late.

See commit f716abd55d1e ("Btrfs: fix out of bounds array access while
reading extent buffer") for details.

Signed-off-by: Young Xiao <92siuyang@gmail.com>
---
 fs/btrfs/extent_io.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index db337e5..dcf3b2e 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -5476,8 +5476,12 @@ int read_extent_buffer_to_user(const struct extent_buffer *eb,
 	unsigned long i = (start_offset + start) >> PAGE_SHIFT;
 	int ret = 0;
 
-	WARN_ON(start > eb->len);
-	WARN_ON(start + len > eb->start + eb->len);
+	if (start + len > eb->len) {
+		WARN(1, KERN_ERR "btrfs bad mapping eb start %llu len %lu, wanted %lu %lu\n",
+		     eb->start, eb->len, start, len);
+		memset(dst, 0, len);
+		return;
+	}
 
 	offset = offset_in_page(start_offset + start);
 
@@ -5554,8 +5558,12 @@ int memcmp_extent_buffer(const struct extent_buffer *eb, const void *ptrv,
 	unsigned long i = (start_offset + start) >> PAGE_SHIFT;
 	int ret = 0;
 
-	WARN_ON(start > eb->len);
-	WARN_ON(start + len > eb->start + eb->len);
+	if (start + len > eb->len) {
+		WARN(1, KERN_ERR "btrfs bad mapping eb start %llu len %lu, wanted %lu %lu\n",
+		     eb->start, eb->len, start, len);
+		memset(ptr, 0, len);
+		return;
+	}
 
 	offset = offset_in_page(start_offset + start);
 
-- 
2.7.4

Re: [PATCH] btrfs: fix out of bounds array access while reading extent buffer

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 2840 bytes --]



On 2019/6/14 下午7:51, Young Xiao wrote:
> There is a corner case that slips through the checkers in functions
> reading extent buffer, ie.
> 
> if (start < eb->len) and (start + len > eb->len), then:
> the checkers in read_extent_buffer_to_user(), and memcmp_extent_buffer()
> WARN_ON(start > eb->len) and WARN_ON(start + len > eb->start + eb->len),
> both are OK in this corner case, but it'd actually try to access the eb->pages
> out of bounds because of (start + len > eb->len).
> 
> This is adding proper checks in order to avoid invalid memory access,
> ie. 'general protection fault', before it's too late.
> 
> See commit f716abd55d1e ("Btrfs: fix out of bounds array access while
> reading extent buffer") for details.
> 
> Signed-off-by: Young Xiao <92siuyang@gmail.com>
> ---
>  fs/btrfs/extent_io.c | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index db337e5..dcf3b2e 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -5476,8 +5476,12 @@ int read_extent_buffer_to_user(const struct extent_buffer *eb,
>  	unsigned long i = (start_offset + start) >> PAGE_SHIFT;
>  	int ret = 0;
>  
> -	WARN_ON(start > eb->len);
> -	WARN_ON(start + len > eb->start + eb->len);
> +	if (start + len > eb->len) {
The original (start + len > eb->start + eb->len) check is so wrong from
the very beginning. eb->start makes no sense in the context.
So your patch makes sense.

But it's not 100% fixed.

If @start and @len overflow u64, e.g @start = 1 << 63 + 8k, @len = 1<<
63 + 8K. it can still skip the check.

So, we still need to check @start against eb->len, then @start + @len
against eb->len.

Also, shouldn't we include the equal case for @start? (although start +
len == eb->len should be OK)

> +		WARN(1, KERN_ERR "btrfs bad mapping eb start %llu len %lu, wanted %lu %lu\n",
> +		     eb->start, eb->len, start, len);
> +		memset(dst, 0, len);

I'd prefer not to do the memset, as @start and @len is already wrong, I
doubt the @dst could be completely some wild pointer, and set them could
easily screw up the whole kernel.

Thanks,
Qu

> +		return;
> +	}
>  
>  	offset = offset_in_page(start_offset + start);
>  
> @@ -5554,8 +5558,12 @@ int memcmp_extent_buffer(const struct extent_buffer *eb, const void *ptrv,
>  	unsigned long i = (start_offset + start) >> PAGE_SHIFT;
>  	int ret = 0;
>  
> -	WARN_ON(start > eb->len);
> -	WARN_ON(start + len > eb->start + eb->len);
> +	if (start + len > eb->len) {
> +		WARN(1, KERN_ERR "btrfs bad mapping eb start %llu len %lu, wanted %lu %lu\n",
> +		     eb->start, eb->len, start, len);
> +		memset(ptr, 0, len);
> +		return;
> +	}
>  
>  	offset = offset_in_page(start_offset + start);
>  
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]