Re: [PATCH] btrfs: fix a out-of-boundary access for copy_compressed_data_to_page()

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

On 2021/11/12 12:17, Josef Bacik wrote:
> On Fri, Nov 12, 2021 at 10:22:53AM +0800, Qu Wenruo wrote:
>> [BUG]
>> The following script can cause btrfs to crash:
>>
>>   mount -o compress-force=lzo $DEV /mnt
>>   dd if=/dev/urandom of=/mnt/foo bs=4k count=1
>>   sync
>>
>> The calltrace looks like this:
>>
>>   general protection fault, probably for non-canonical address 0xe04b37fccce3b000: 0000 [#1] PREEMPT SMP NOPTI
>>   CPU: 5 PID: 164 Comm: kworker/u20:3 Not tainted 5.15.0-rc7-custom+ #4
>>   Workqueue: btrfs-delalloc btrfs_work_helper [btrfs]
>>   RIP: 0010:__memcpy+0x12/0x20
>>   Call Trace:
>>    lzo_compress_pages+0x236/0x540 [btrfs]
>>    btrfs_compress_pages+0xaa/0xf0 [btrfs]
>>    compress_file_range+0x431/0x8e0 [btrfs]
>>    async_cow_start+0x12/0x30 [btrfs]
>>    btrfs_work_helper+0xf6/0x3e0 [btrfs]
>>    process_one_work+0x294/0x5d0
>>    worker_thread+0x55/0x3c0
>>    kthread+0x140/0x170
>>    ret_from_fork+0x22/0x30
>>   ---[ end trace 63c3c0f131e61982 ]---
>>
>> [CAUSE]
>> In lzo_compress_pages(), parameter @out_pages is not only an output
>> parameter (for the compressed pages), but also an input parameter, for
>> the maximum amount of pages we can utilize.
>>
>> In commit d4088803f511 ("btrfs: subpage: make lzo_compress_pages()
>> compatible"), the refactor doesn't take @out_pages as an input, thus
>> completely ignoring the limit.
>>
>> And for compress-force case, we could hit incompressible data that
>> compressed size would go beyond the page limit, and cause above crash.
>>
>> [FIX]
>> Save @out_pages as @max_nr_page, and pass it to lzo_compress_pages(),
>> and check if we're beyond the limit before accessing the pages.
>>
>> Reported-by: Omar Sandoval <osandov@fb.com>
>> Fixes: d4088803f511 ("btrfs: subpage: make lzo_compress_pages() compatible")
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>>   fs/btrfs/lzo.c | 12 +++++++++++-
>>   1 file changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/btrfs/lzo.c b/fs/btrfs/lzo.c
>> index 00cffc183ec0..f410ceabcdbd 100644
>> --- a/fs/btrfs/lzo.c
>> +++ b/fs/btrfs/lzo.c
>> @@ -125,6 +125,7 @@ static inline size_t read_compress_length(const char *buf)
>>   static int copy_compressed_data_to_page(char *compressed_data,
>>   					size_t compressed_size,
>>   					struct page **out_pages,
>> +					unsigned long max_nr_page,
>
> If you want to do const down below you should use const here probably?  Thanks,

Right, max_nr_page should also be const.

Thanks for catching this,
Qu

>
> Josef
>