diff mbox series

[v5,1/4] btrfs: extent_io: do extra check for extent buffer read write functions

Message ID 20200819063550.62832-2-wqu@suse.com (mailing list archive)
State New, archived
Headers show
Series btrfs: Enhanced runtime defence against fuzzed images | expand

Commit Message

Qu Wenruo Aug. 19, 2020, 6:35 a.m. UTC 
Although we have start, len check for extent buffer reader/write (e.g.
read_extent_buffer()), those checks has its limitations:
- No overflow check
  Values like start = 1024 len = -1024 can still pass the basic
   (start + len) > eb->len check.

- Checks are not consistent
  For read_extent_buffer() we only check (start + len) against eb->len.
  While for memcmp_extent_buffer() we also check start against eb->len.

- Different error reporting mechanism
  We use WARN() in read_extent_buffer() but BUG() in
  memcpy_extent_buffer().

- Still modify memory if the request is obviously wrong
  In read_extent_buffer() even we find (start + len) > eb->len, we still
  call memset(dst, 0, len), which can eaisly cause memory access error
  if start + len overflows.

To address above problems, this patch creates a new common function to
check such access, check_eb_range().
- Add overflow check
  This function checks start, start + len against eb->len and overflow
  check.

- Unified checks

- Unified error reports
  Will call WARN() if CONFIG_BTRFS_DEBUG is configured.
  And also do btrfs_warn() message for non-debug build.

- Exit ASAP if check fails
  No more possible memory corruption.

- Add extra comment for @start @len used in those functions
  Even experienced developers sometimes get confused with the @start
  @len with logical address in those functions.
  I'm not sure what's the cause, maybe it's the extent_buffer::start
  naming.
  For now, just add some comment.

Link: https://bugzilla.kernel.org/show_bug.cgi?id=202817
[ Inspired by above report, the report itself is already addressed ]
Signed-off-by: Qu Wenruo <wqu@suse.com>
Reviewed-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/extent_io.c | 82 ++++++++++++++++++++++++--------------------
 1 file changed, 45 insertions(+), 37 deletions(-)

Comments

David Sterba Aug. 19, 2020, 5:11 p.m. UTC | #1 
On Wed, Aug 19, 2020 at 02:35:47PM +0800, Qu Wenruo wrote:
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -5620,6 +5620,34 @@ int read_extent_buffer_pages(struct extent_buffer *eb, int wait, int mirror_num)
>  	return ret;
>  }
>  
> +static void report_eb_range(const struct extent_buffer *eb, unsigned long start,
> +			    unsigned long len)
> +{
> +	btrfs_warn(eb->fs_info,
> +"btrfs: bad eb rw request, eb bytenr=%llu len=%lu rw start=%lu len=%lu\n",

No "btrfs:" prefix needed, no "\n" at the end of the string. The format
now uses the 'key=value' style, while we have the 'key value' in many
other places, this should be consistent.

> +		   eb->start, eb->len, start, len);
> +	WARN_ON(IS_ENABLED(CONFIG_BTRFS_DEBUG));
> +}
> +
> +/*
> + * Check if the [start, start + len) range is valid before reading/writing
> + * the eb.
> + * NOTE: @start and @len are offset *INSIDE* the eb, *NOT* logical address.
> + *
> + * Caller should not touch the dst/src memory if this function returns error.
> + */
> +static inline int check_eb_range(const struct extent_buffer *eb,
> +				 unsigned long start, unsigned long len)
> +{
> +	/* start, start + len should not go beyond eb->len nor overflow */
> +	if (unlikely(start > eb->len || start + len > eb->len ||
> +		     len > eb->len)) {

Can the number of condition be reduced? If 'start + len' overflows, then
we don't need to check 'start > eb->len', and for the case where
start = 1024 and len = -1024 the 'len > eb-len' would be enough.

> +		report_eb_range(eb, start, len);
> +		return -EINVAL;

This could be simply

		return report_eb_range(...);

It's not a big difference though, compiler produces the same code.
Qu Wenruo Aug. 19, 2020, 11:14 p.m. UTC | #2 
On 2020/8/20 上午1:11, David Sterba wrote:
> On Wed, Aug 19, 2020 at 02:35:47PM +0800, Qu Wenruo wrote:
>> --- a/fs/btrfs/extent_io.c
>> +++ b/fs/btrfs/extent_io.c
>> @@ -5620,6 +5620,34 @@ int read_extent_buffer_pages(struct extent_buffer *eb, int wait, int mirror_num)
>>  	return ret;
>>  }
>>  
>> +static void report_eb_range(const struct extent_buffer *eb, unsigned long start,
>> +			    unsigned long len)
>> +{
>> +	btrfs_warn(eb->fs_info,
>> +"btrfs: bad eb rw request, eb bytenr=%llu len=%lu rw start=%lu len=%lu\n",
> 
> No "btrfs:" prefix needed, no "\n" at the end of the string.

Oh sorry, should re-check the message.

> The format
> now uses the 'key=value' style, while we have the 'key value' in many
> other places, this should be consistent.

Indeed, I just checked tree-checker, which does use 'key value'.

> 
>> +		   eb->start, eb->len, start, len);
>> +	WARN_ON(IS_ENABLED(CONFIG_BTRFS_DEBUG));
>> +}
>> +
>> +/*
>> + * Check if the [start, start + len) range is valid before reading/writing
>> + * the eb.
>> + * NOTE: @start and @len are offset *INSIDE* the eb, *NOT* logical address.
>> + *
>> + * Caller should not touch the dst/src memory if this function returns error.
>> + */
>> +static inline int check_eb_range(const struct extent_buffer *eb,
>> +				 unsigned long start, unsigned long len)
>> +{
>> +	/* start, start + len should not go beyond eb->len nor overflow */
>> +	if (unlikely(start > eb->len || start + len > eb->len ||
>> +		     len > eb->len)) {
> 
> Can the number of condition be reduced? If 'start + len' overflows, then
> we don't need to check 'start > eb->len', and for the case where
> start = 1024 and len = -1024 the 'len > eb-len' would be enough.

I'm afraid not.
Although 'start > eb->len || len > eb->len' is enough to detect overflow
case, it no longer detects cases like 'start = 2k, len = 3k' while
eb->len == 4K case.

So we still need all 3 checks.

Thanks,
Qu

> 
>> +		report_eb_range(eb, start, len);
>> +		return -EINVAL;
> 
> This could be simply
> 
> 		return report_eb_range(...);
> 
> It's not a big difference though, compiler produces the same code.
>
David Sterba Aug. 20, 2020, 9:50 a.m. UTC | #3 
On Thu, Aug 20, 2020 at 07:14:13AM +0800, Qu Wenruo wrote:
> >> +static inline int check_eb_range(const struct extent_buffer *eb,
> >> +				 unsigned long start, unsigned long len)
> >> +{
> >> +	/* start, start + len should not go beyond eb->len nor overflow */
> >> +	if (unlikely(start > eb->len || start + len > eb->len ||
> >> +		     len > eb->len)) {
> > 
> > Can the number of condition be reduced? If 'start + len' overflows, then
> > we don't need to check 'start > eb->len', and for the case where
> > start = 1024 and len = -1024 the 'len > eb-len' would be enough.
> 
> I'm afraid not.
> Although 'start > eb->len || len > eb->len' is enough to detect overflow
> case, it no longer detects cases like 'start = 2k, len = 3k' while
> eb->len == 4K case.
> 
> So we still need all 3 checks.

I was suggesting 'start + len > eb->len', not 'start > eb-len'.

"start > eb->len" is implied by "start + len > eb->len".
Qu Wenruo Aug. 20, 2020, 9:58 a.m. UTC | #4 
On 2020/8/20 下午5:50, David Sterba wrote:
> On Thu, Aug 20, 2020 at 07:14:13AM +0800, Qu Wenruo wrote:
>>>> +static inline int check_eb_range(const struct extent_buffer *eb,
>>>> +				 unsigned long start, unsigned long len)
>>>> +{
>>>> +	/* start, start + len should not go beyond eb->len nor overflow */
>>>> +	if (unlikely(start > eb->len || start + len > eb->len ||
>>>> +		     len > eb->len)) {
>>>
>>> Can the number of condition be reduced? If 'start + len' overflows, then
>>> we don't need to check 'start > eb->len', and for the case where
>>> start = 1024 and len = -1024 the 'len > eb-len' would be enough.
>>
>> I'm afraid not.
>> Although 'start > eb->len || len > eb->len' is enough to detect overflow
>> case, it no longer detects cases like 'start = 2k, len = 3k' while
>> eb->len == 4K case.
>>
>> So we still need all 3 checks.
> 
> I was suggesting 'start + len > eb->len', not 'start > eb-len'.
> 
> "start > eb->len" is implied by "start + len > eb->len".
> 

start > eb->len is not implied if (start + len) over flows.

E.g. start = -2K, len = 2k, eb->len = 4K. We can still pass !(start +
len > eb->len || len > eb->len).

In short, if we want overflow check along with each one checked, we
really need 3 checks.

Thanks,
Qu
David Sterba Aug. 20, 2020, 2:46 p.m. UTC | #5 
On Thu, Aug 20, 2020 at 05:58:53PM +0800, Qu Wenruo wrote:
> 
> 
> On 2020/8/20 下午5:50, David Sterba wrote:
> > On Thu, Aug 20, 2020 at 07:14:13AM +0800, Qu Wenruo wrote:
> >>>> +static inline int check_eb_range(const struct extent_buffer *eb,
> >>>> +				 unsigned long start, unsigned long len)
> >>>> +{
> >>>> +	/* start, start + len should not go beyond eb->len nor overflow */
> >>>> +	if (unlikely(start > eb->len || start + len > eb->len ||
> >>>> +		     len > eb->len)) {
> >>>
> >>> Can the number of condition be reduced? If 'start + len' overflows, then
> >>> we don't need to check 'start > eb->len', and for the case where
> >>> start = 1024 and len = -1024 the 'len > eb-len' would be enough.
> >>
> >> I'm afraid not.
> >> Although 'start > eb->len || len > eb->len' is enough to detect overflow
> >> case, it no longer detects cases like 'start = 2k, len = 3k' while
> >> eb->len == 4K case.
> >>
> >> So we still need all 3 checks.
> > 
> > I was suggesting 'start + len > eb->len', not 'start > eb-len'.
> > 
> > "start > eb->len" is implied by "start + len > eb->len".
> 
> start > eb->len is not implied if (start + len) over flows.
> 
> E.g. start = -2K, len = 2k, eb->len = 4K. We can still pass !(start +
> len > eb->len || len > eb->len).
> 
> In short, if we want overflow check along with each one checked, we
> really need 3 checks.

So what if we add overflow check, that would catch negative start or
negative length, and then do start + len > eb->len?

The check_setget_bounds is different becasue the len is known at compile
time so the overflows can't happen in the same way as for the eb range,
so this this confused me first.

	check_add_overflow(start, len) || start + len > eb->len
David Sterba Aug. 20, 2020, 3:18 p.m. UTC | #6 
On Thu, Aug 20, 2020 at 04:46:47PM +0200, David Sterba wrote:
> On Thu, Aug 20, 2020 at 05:58:53PM +0800, Qu Wenruo wrote:
> 	check_add_overflow(start, len) || start + len > eb->len

--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -13,6 +13,7 @@
 #include <linux/pagevec.h>
 #include <linux/prefetch.h>
 #include <linux/cleancache.h>
+#include <linux/overflow.h>
 #include "extent_io.h"
 #include "extent-io-tree.h"
 #include "extent_map.h"
@@ -5641,9 +5642,10 @@ static void report_eb_range(const struct extent_buffer *eb, unsigned long start,
 static inline int check_eb_range(const struct extent_buffer *eb,
                                 unsigned long start, unsigned long len)
 {
+       unsigned long offset;
+
        /* start, start + len should not go beyond eb->len nor overflow */
-       if (unlikely(start > eb->len || start + len > eb->len ||
-                    len > eb->len)) {
+       if (unlikely(check_add_overflow(start, len, &offset) || offset > eb->len)) {
                report_eb_range(eb, start, len);
                return -EINVAL;
        }
---
Qu Wenruo Aug. 20, 2020, 11:39 p.m. UTC | #7 
On 2020/8/20 下午10:46, David Sterba wrote:
> On Thu, Aug 20, 2020 at 05:58:53PM +0800, Qu Wenruo wrote:
>>
>>
>> On 2020/8/20 下午5:50, David Sterba wrote:
>>> On Thu, Aug 20, 2020 at 07:14:13AM +0800, Qu Wenruo wrote:
>>>>>> +static inline int check_eb_range(const struct extent_buffer *eb,
>>>>>> +				 unsigned long start, unsigned long len)
>>>>>> +{
>>>>>> +	/* start, start + len should not go beyond eb->len nor overflow */
>>>>>> +	if (unlikely(start > eb->len || start + len > eb->len ||
>>>>>> +		     len > eb->len)) {
>>>>>
>>>>> Can the number of condition be reduced? If 'start + len' overflows, then
>>>>> we don't need to check 'start > eb->len', and for the case where
>>>>> start = 1024 and len = -1024 the 'len > eb-len' would be enough.
>>>>
>>>> I'm afraid not.
>>>> Although 'start > eb->len || len > eb->len' is enough to detect overflow
>>>> case, it no longer detects cases like 'start = 2k, len = 3k' while
>>>> eb->len == 4K case.
>>>>
>>>> So we still need all 3 checks.
>>>
>>> I was suggesting 'start + len > eb->len', not 'start > eb-len'.
>>>
>>> "start > eb->len" is implied by "start + len > eb->len".
>>
>> start > eb->len is not implied if (start + len) over flows.
>>
>> E.g. start = -2K, len = 2k, eb->len = 4K. We can still pass !(start +
>> len > eb->len || len > eb->len).
>>
>> In short, if we want overflow check along with each one checked, we
>> really need 3 checks.
> 
> So what if we add overflow check, that would catch negative start or
> negative length, and then do start + len > eb->len?
> 
> The check_setget_bounds is different becasue the len is known at compile
> time so the overflows can't happen in the same way as for the eb range,
> so this this confused me first.
> 
> 	check_add_overflow(start, len) || start + len > eb->len
> 
Then it should be more or less the same as the existing 3 checks.

In fact, the 3 checks are just the overflow safe check for (start + len
> eb->len).

The difference between check_setget_bounds() and check_eb_range() is,
the size for check_setget_bounds() are fixed size, thus it will never be
negative, thus it can skip the (size > eb->len) check.

Thanks,
Qu
diff mbox series

Patch

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 617ea38e6fd7..4eb35a1338c1 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -5620,6 +5620,34 @@  int read_extent_buffer_pages(struct extent_buffer *eb, int wait, int mirror_num)
 	return ret;
 }
 
+static void report_eb_range(const struct extent_buffer *eb, unsigned long start,
+			    unsigned long len)
+{
+	btrfs_warn(eb->fs_info,
+"btrfs: bad eb rw request, eb bytenr=%llu len=%lu rw start=%lu len=%lu\n",
+		   eb->start, eb->len, start, len);
+	WARN_ON(IS_ENABLED(CONFIG_BTRFS_DEBUG));
+}
+
+/*
+ * Check if the [start, start + len) range is valid before reading/writing
+ * the eb.
+ * NOTE: @start and @len are offset *INSIDE* the eb, *NOT* logical address.
+ *
+ * Caller should not touch the dst/src memory if this function returns error.
+ */
+static inline int check_eb_range(const struct extent_buffer *eb,
+				 unsigned long start, unsigned long len)
+{
+	/* start, start + len should not go beyond eb->len nor overflow */
+	if (unlikely(start > eb->len || start + len > eb->len ||
+		     len > eb->len)) {
+		report_eb_range(eb, start, len);
+		return -EINVAL;
+	}
+	return 0;
+}
+
 void read_extent_buffer(const struct extent_buffer *eb, void *dstv,
 			unsigned long start, unsigned long len)
 {
@@ -5630,12 +5658,8 @@  void read_extent_buffer(const struct extent_buffer *eb, void *dstv,
 	char *dst = (char *)dstv;
 	unsigned long i = start >> PAGE_SHIFT;
 
-	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);
+	if (check_eb_range(eb, start, len))
 		return;
-	}
 
 	offset = offset_in_page(start);
 
@@ -5700,8 +5724,8 @@  int memcmp_extent_buffer(const struct extent_buffer *eb, const void *ptrv,
 	unsigned long i = start >> PAGE_SHIFT;
 	int ret = 0;
 
-	WARN_ON(start > eb->len);
-	WARN_ON(start + len > eb->start + eb->len);
+	if (check_eb_range(eb, start, len))
+		return -EINVAL;
 
 	offset = offset_in_page(start);
 
@@ -5754,8 +5778,8 @@  void write_extent_buffer(const struct extent_buffer *eb, const void *srcv,
 	char *src = (char *)srcv;
 	unsigned long i = start >> PAGE_SHIFT;
 
-	WARN_ON(start > eb->len);
-	WARN_ON(start + len > eb->start + eb->len);
+	if (check_eb_range(eb, start, len))
+		return;
 
 	offset = offset_in_page(start);
 
@@ -5783,8 +5807,8 @@  void memzero_extent_buffer(const struct extent_buffer *eb, unsigned long start,
 	char *kaddr;
 	unsigned long i = start >> PAGE_SHIFT;
 
-	WARN_ON(start > eb->len);
-	WARN_ON(start + len > eb->start + eb->len);
+	if (check_eb_range(eb, start, len))
+		return;
 
 	offset = offset_in_page(start);
 
@@ -5828,6 +5852,10 @@  void copy_extent_buffer(const struct extent_buffer *dst,
 	char *kaddr;
 	unsigned long i = dst_offset >> PAGE_SHIFT;
 
+	if (check_eb_range(dst, dst_offset, len) ||
+	    check_eb_range(src, src_offset, len))
+		return;
+
 	WARN_ON(src->len != dst_len);
 
 	offset = offset_in_page(dst_offset);
@@ -6017,25 +6045,15 @@  void memcpy_extent_buffer(const struct extent_buffer *dst,
 			  unsigned long dst_offset, unsigned long src_offset,
 			  unsigned long len)
 {
-	struct btrfs_fs_info *fs_info = dst->fs_info;
 	size_t cur;
 	size_t dst_off_in_page;
 	size_t src_off_in_page;
 	unsigned long dst_i;
 	unsigned long src_i;
 
-	if (src_offset + len > dst->len) {
-		btrfs_err(fs_info,
-			"memmove bogus src_offset %lu move len %lu dst len %lu",
-			 src_offset, len, dst->len);
-		BUG();
-	}
-	if (dst_offset + len > dst->len) {
-		btrfs_err(fs_info,
-			"memmove bogus dst_offset %lu move len %lu dst len %lu",
-			 dst_offset, len, dst->len);
-		BUG();
-	}
+	if (check_eb_range(dst, dst_offset, len) ||
+	    check_eb_range(dst, src_offset, len))
+		return;
 
 	while (len > 0) {
 		dst_off_in_page = offset_in_page(dst_offset);
@@ -6062,7 +6080,6 @@  void memmove_extent_buffer(const struct extent_buffer *dst,
 			   unsigned long dst_offset, unsigned long src_offset,
 			   unsigned long len)
 {
-	struct btrfs_fs_info *fs_info = dst->fs_info;
 	size_t cur;
 	size_t dst_off_in_page;
 	size_t src_off_in_page;
@@ -6071,18 +6088,9 @@  void memmove_extent_buffer(const struct extent_buffer *dst,
 	unsigned long dst_i;
 	unsigned long src_i;
 
-	if (src_offset + len > dst->len) {
-		btrfs_err(fs_info,
-			  "memmove bogus src_offset %lu move len %lu len %lu",
-			  src_offset, len, dst->len);
-		BUG();
-	}
-	if (dst_offset + len > dst->len) {
-		btrfs_err(fs_info,
-			  "memmove bogus dst_offset %lu move len %lu len %lu",
-			  dst_offset, len, dst->len);
-		BUG();
-	}
+	if (check_eb_range(dst, dst_offset, len) ||
+	    check_eb_range(dst, src_offset, len))
+		return;
 	if (dst_offset < src_offset) {
 		memcpy_extent_buffer(dst, dst_offset, src_offset, len);
 		return;