| Message ID | 1560513109-2568-1-git-send-email-92siuyang@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: fix out of bounds array access while reading extent buffer | expand |
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); > >
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);
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(-)