linux-kernel - Re: [PATCH] f2fs: data: fix possible overflow in check_swap

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <5769c4fa-3112-3582-00a0-2b17757d8cab@omp.ru>
Date:   Mon, 11 Dec 2023 00:14:16 +0300
From:   Sergey Shtylyov <s.shtylyov@....ru>
To:     Chao Yu <chao@...nel.org>, Jaegeuk Kim <jaegeuk@...nel.org>,
        <linux-f2fs-devel@...ts.sourceforge.net>
CC:     <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] f2fs: data: fix possible overflow in
 check_swap_activate()

Hello!

   Sorry for replying a month later than I should -- I got distracted by the
other Svace reports... It took a significant part of the weekend to swap this
stuff back in... :-/

On 11/7/23 6:29 PM, Chao Yu wrote:
[...]
>> In check_swap_activate(), if the *while* loop exits early (0- or 1-page
>> long swap file), an overflow happens while calculating the value of the
>> span parameter as the lowest_pblock variable ends up being greater than
>> the highest_pblock variable. Let's set *span to 0 in this case...
> 
> What do you think of returning -EINVAL for such case? I assume this is a
> corner case.

   I don't know the code well enough but I got the impression that iff
we have a file containing a single page, we'd have one successful call
of add_swap_extent(). Am I missing something?

>> Found by Linux Verification Center (linuxtesting.org) with the SVACE static
>> analysis tool.
>>
>> Signed-off-by: Sergey Shtylyov <s.shtylyov@....ru>
>> ---
>> This patch is against the 'master' branch of Jaegeuk Kim's F2FS repo...
>>
>>   fs/f2fs/data.c | 5 ++++-
>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
>> index 916e317ac925..342cb0d5056d 100644
>> --- a/fs/f2fs/data.c
>> +++ b/fs/f2fs/data.c
>> @@ -4047,7 +4047,10 @@ static int check_swap_activate(struct swap_info_struct *sis,
>>           cur_lblock += nr_pblocks;
>>       }
>>       ret = nr_extents;
>> -    *span = 1 + highest_pblock - lowest_pblock;
>> +    if (lowest_pblock <= highest_pblock)
> 
> if (unlikely(higest_pblock < lowest_pblock))

   Well, Greg KH says we shouldn't use unlikely() unless we can prove
that it indeed improves things...

>     return -EINVAL;
> 
> *span = 1 + highest_pblock - lowest_pblock;
> 
> Thanks,
> 
>> +        *span = 1 + highest_pblock - lowest_pblock;
>> +    else
>> +        *span = 0;
>>       if (cur_lblock == 0)
>>           cur_lblock = 1;    /* force Empty message */
>>       sis->max = cur_lblock;

MBR, Sergey