Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:4 as permitted sender) client-ip=2620:137:e000::3:4;
Subject: Re: [PATCH] f2fs: data: fix possible overflow in
 check_swap_activate()
To:     Chao Yu <chao@kernel.org>, Jaegeuk Kim <jaegeuk@kernel.org>,
        <linux-f2fs-devel@lists.sourceforge.net>
CC:     <linux-kernel@vger.kernel.org>
References: <20231025202019.5228-1-s.shtylyov@omp.ru>
 <e2167506-2b0a-a0f5-c0e9-0e8b6139c69e@kernel.org>
From:   Sergey Shtylyov <s.shtylyov@omp.ru>
Organization: Open Mobile Platform
Message-ID: <5769c4fa-3112-3582-00a0-2b17757d8cab@omp.ru>
Date:   Mon, 11 Dec 2023 00:14:16 +0300
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.10.1
MIME-Version: 1.0
In-Reply-To: <e2167506-2b0a-a0f5-c0e9-0e8b6139c69e@kernel.org>
Content-Type: text/plain; charset="utf-8"
Content-Language: en-US
Content-Transfer-Encoding: 8bit
X-KSE-Attachment-Filter-Triggered-Filters: Clean
X-KSE-BulkMessagesFiltering-Scan-Result: InTheLimit
X-Spam-Status: No, score=-0.1 required=5.0 tests=HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,NICE_REPLY_A,RCVD_IN_SBL_CSS,SPF_HELO_NONE,SPF_PASS,
	T_SCC_BODY_TEXT_LINE autolearn=unavailable autolearn_force=no
	version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on howler.vger.email
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (howler.vger.email [0.0.0.0]); Sun, 10 Dec 2023 13:18:15 -0800 (PST)

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@omp.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