diff mbox

[3/3] btrfs-progs: print-tree: Enhance btrfs_print_tree() check to avoid out-of-boundary memory access

Message ID 20180430031545.29891-3-wqu@suse.com (mailing list archive)
State New, archived
Headers show

Commit Message

Qu Wenruo April 30, 2018, 3:15 a.m. UTC
For btrfs_print_tree(), if nr_items is corrupted, it can easily go
beyond extent buffer boundary.

Add extra nr_item check, and only print as many valid slots as possible.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 print-tree.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

Comments

Su Yue April 30, 2018, 3:49 a.m. UTC | #1
On 04/30/2018 11:15 AM, Qu Wenruo wrote:
> For btrfs_print_tree(), if nr_items is corrupted, it can easily go
> beyond extent buffer boundary.
> 
> Add extra nr_item check, and only print as many valid slots as possible.
> 

Make sense.

> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>   print-tree.c | 11 ++++++++++-
>   1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/print-tree.c b/print-tree.c
> index 31a851ef4413..55db80bebb2a 100644
> --- a/print-tree.c
> +++ b/print-tree.c
> @@ -1376,6 +1376,11 @@ void btrfs_print_tree(struct extent_buffer *eb, int follow)
>   		btrfs_print_leaf(eb);
>   		return;
>   	}
> +	/* We are crossing eb boundary, this node must be corrupted */
> +	if (nr > BTRFS_NODEPTRS_PER_EXTENT_BUFFER(eb))
> +		warning(
> +		"node nr_items corrupted, has %u limit %u, continue print anyway",
> +			nr, BTRFS_NODEPTRS_PER_EXTENT_BUFFER(eb));
>   	printf("node %llu level %d items %d free %u generation %llu owner ",
>   	       (unsigned long long)eb->start,
>   	        btrfs_header_level(eb), nr,
> @@ -1386,7 +1391,11 @@ void btrfs_print_tree(struct extent_buffer *eb, int follow)
>   	print_uuids(eb);
>   	fflush(stdout);
>   		
> -		u64 blocknr = btrfs_node_blockptr(eb, i);
> +		u64 blocknr;
> +
> +		if (i > BTRFS_NODEPTRS_PER_EXTENT_BUFFER(eb))
> +			break;

Should it be i >= BTRFS_NODEPTRS_PER_EXTENT_BUFFER(eb)?

Here BTRFS_NODEPTRS_PER_EXTENT_BUFFER() is called during iterations.
The judement can be calculated in advance like:

	ptr_num = BTRFS_NODEPTRS_PER_EXTENT_BUFFER(eb);
	...
	for (i = 0; i < nr && i < ptr_num  ; i++) {

Thanks,
Su

> +		blocknr = btrfs_node_blockptr(eb, i);
>   		btrfs_node_key(eb, &disk_key, i);
>   		btrfs_disk_key_to_cpu(&key, &disk_key);
>   		printf("\t");
> 


--
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
Qu Wenruo April 30, 2018, 3:51 a.m. UTC | #2
On 2018年04月30日 11:49, Su Yue wrote:
> 
> 
> On 04/30/2018 11:15 AM, Qu Wenruo wrote:
>> For btrfs_print_tree(), if nr_items is corrupted, it can easily go
>> beyond extent buffer boundary.
>>
>> Add extra nr_item check, and only print as many valid slots as possible.
>>
> 
> Make sense.
> 
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>>   print-tree.c | 11 ++++++++++-
>>   1 file changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/print-tree.c b/print-tree.c
>> index 31a851ef4413..55db80bebb2a 100644
>> --- a/print-tree.c
>> +++ b/print-tree.c
>> @@ -1376,6 +1376,11 @@ void btrfs_print_tree(struct extent_buffer *eb,
>> int follow)
>>           btrfs_print_leaf(eb);
>>           return;
>>       }
>> +    /* We are crossing eb boundary, this node must be corrupted */
>> +    if (nr > BTRFS_NODEPTRS_PER_EXTENT_BUFFER(eb))
>> +        warning(
>> +        "node nr_items corrupted, has %u limit %u, continue print
>> anyway",
>> +            nr, BTRFS_NODEPTRS_PER_EXTENT_BUFFER(eb));
>>       printf("node %llu level %d items %d free %u generation %llu
>> owner ",
>>              (unsigned long long)eb->start,
>>               btrfs_header_level(eb), nr,
>> @@ -1386,7 +1391,11 @@ void btrfs_print_tree(struct extent_buffer *eb,
>> int follow)
>>       print_uuids(eb);
>>       fflush(stdout);
>>          
>> -        u64 blocknr = btrfs_node_blockptr(eb, i);
>> +        u64 blocknr;
>> +
>> +        if (i > BTRFS_NODEPTRS_PER_EXTENT_BUFFER(eb))
>> +            break;
> 
> Should it be i >= BTRFS_NODEPTRS_PER_EXTENT_BUFFER(eb)?

BTRFS_NODEPTRS_PER_EXTENT_BUFFER() provides the maximum valid number.
So it 's >=.

> 
> Here BTRFS_NODEPTRS_PER_EXTENT_BUFFER() is called during iterations.
> The judement can be calculated in advance like:
> 
>     ptr_num = BTRFS_NODEPTRS_PER_EXTENT_BUFFER(eb);
>     ...
>     for (i = 0; i < nr && i < ptr_num  ; i++) {

Indeed looks better.

Thanks,
Qu

> 
> Thanks,
> Su
> 
>> +        blocknr = btrfs_node_blockptr(eb, i);
>>           btrfs_node_key(eb, &disk_key, i);
>>           btrfs_disk_key_to_cpu(&key, &disk_key);
>>           printf("\t");
>>
> 
> 
> -- 
> 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
David Sterba May 9, 2018, 11:57 a.m. UTC | #3
On Mon, Apr 30, 2018 at 11:51:19AM +0800, Qu Wenruo wrote:
> >>           btrfs_print_leaf(eb);
> >>           return;
> >>       }
> >> +    /* We are crossing eb boundary, this node must be corrupted */
> >> +    if (nr > BTRFS_NODEPTRS_PER_EXTENT_BUFFER(eb))
> >> +        warning(
> >> +        "node nr_items corrupted, has %u limit %u, continue print
> >> anyway",
> >> +            nr, BTRFS_NODEPTRS_PER_EXTENT_BUFFER(eb));
> >>       printf("node %llu level %d items %d free %u generation %llu
> >> owner ",
> >>              (unsigned long long)eb->start,
> >>               btrfs_header_level(eb), nr,
> >> @@ -1386,7 +1391,11 @@ void btrfs_print_tree(struct extent_buffer *eb,
> >> int follow)
> >>       print_uuids(eb);
> >>       fflush(stdout);
> >>          
> >> -        u64 blocknr = btrfs_node_blockptr(eb, i);
> >> +        u64 blocknr;
> >> +
> >> +        if (i > BTRFS_NODEPTRS_PER_EXTENT_BUFFER(eb))
> >> +            break;
> > 
> > Should it be i >= BTRFS_NODEPTRS_PER_EXTENT_BUFFER(eb)?
> 
> BTRFS_NODEPTRS_PER_EXTENT_BUFFER() provides the maximum valid number.
> So it 's >=.
> 
> > 
> > Here BTRFS_NODEPTRS_PER_EXTENT_BUFFER() is called during iterations.
> > The judement can be calculated in advance like:
> > 
> >     ptr_num = BTRFS_NODEPTRS_PER_EXTENT_BUFFER(eb);
> >     ...
> >     for (i = 0; i < nr && i < ptr_num  ; i++) {
> 
> Indeed looks better.

Please resend this patch with the suggested updates, thanks.
--
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
diff mbox

Patch

diff --git a/print-tree.c b/print-tree.c
index 31a851ef4413..55db80bebb2a 100644
--- a/print-tree.c
+++ b/print-tree.c
@@ -1376,6 +1376,11 @@  void btrfs_print_tree(struct extent_buffer *eb, int follow)
 		btrfs_print_leaf(eb);
 		return;
 	}
+	/* We are crossing eb boundary, this node must be corrupted */
+	if (nr > BTRFS_NODEPTRS_PER_EXTENT_BUFFER(eb))
+		warning(
+		"node nr_items corrupted, has %u limit %u, continue print anyway",
+			nr, BTRFS_NODEPTRS_PER_EXTENT_BUFFER(eb));
 	printf("node %llu level %d items %d free %u generation %llu owner ",
 	       (unsigned long long)eb->start,
 	        btrfs_header_level(eb), nr,
@@ -1386,7 +1391,11 @@  void btrfs_print_tree(struct extent_buffer *eb, int follow)
 	print_uuids(eb);
 	fflush(stdout);
 	for (i = 0; i < nr; i++) {
-		u64 blocknr = btrfs_node_blockptr(eb, i);
+		u64 blocknr;
+
+		if (i > BTRFS_NODEPTRS_PER_EXTENT_BUFFER(eb))
+			break;
+		blocknr = btrfs_node_blockptr(eb, i);
 		btrfs_node_key(eb, &disk_key, i);
 		btrfs_disk_key_to_cpu(&key, &disk_key);
 		printf("\t");