linux-kernel - Re: [PATCH] jfs: Fix array-index-out-of-bounds in diFree

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] [thread-next>] [day] [month] [year] [list]

Date: Thu, 25 Apr 2024 09:24:43 -0500
From: Dave Kleikamp <dave.kleikamp@...cle.com>
To: Matthew Wilcox <willy@...radead.org>, Jeongjun Park <aha310510@...il.com>
Cc: brauner@...nel.org, jfs-discussion@...ts.sourceforge.net,
        jlayton@...nel.org, linux-fsdevel@...r.kernel.org,
        linux-kernel@...r.kernel.org, shaggy@...nel.org,
        syzbot+241c815bda521982cb49@...kaller.appspotmail.com,
        syzkaller-bugs@...glegroups.com
Subject: Re: [PATCH] jfs: Fix array-index-out-of-bounds in diFree

On 4/25/24 9:17AM, Matthew Wilcox wrote:
> On Thu, Apr 25, 2024 at 11:10:38PM +0900, Jeongjun Park wrote:
>> Matthew Wilcox wrote:
>>> If that's the problem then the correct place to detect & reject this is
>>> during mount, not at inode free time.
>>
>> I fixed the patch as you said. If you patch in this way, the
>> file system will not be affected by the vulnerability at all
>> due to the code structure.
> 
> It should be checked earlier than this.

Right. This is preferable.

>  There's this code in
> dbMount().  Why isn't this catching it?

db_agstart is not checked in this code. That may be the bad data in this 
case.

There are probably dozens more sanity checks that are missing when data 
is first read from the disk.

Shaggy

> 
>          bmp->db_agl2size = le32_to_cpu(dbmp_le->dn_agl2size);
>          if (bmp->db_agl2size > L2MAXL2SIZE - L2MAXAG ||
>              bmp->db_agl2size < 0) {
>                  err = -EINVAL;
>                  goto err_release_metapage;
>          }
> 
>          if (((bmp->db_mapsize - 1) >> bmp->db_agl2size) > MAXAG) {
>                  err = -EINVAL;
>                  goto err_release_metapage;
>          }
> 
> 
>> Thanks.
>>
>> ---
>>   fs/jfs/jfs_imap.c | 5 ++++-
>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/jfs/jfs_imap.c b/fs/jfs/jfs_imap.c
>> index 2ec35889ad24..ba0aa2f145cc 100644
>> --- a/fs/jfs/jfs_imap.c
>> +++ b/fs/jfs/jfs_imap.c
>> @@ -290,7 +290,7 @@ int diSync(struct inode *ipimap)
>>   int diRead(struct inode *ip)
>>   {
>>   	struct jfs_sb_info *sbi = JFS_SBI(ip->i_sb);
>> -	int iagno, ino, extno, rc;
>> +	int iagno, ino, extno, rc, agno;
>>   	struct inode *ipimap;
>>   	struct dinode *dp;
>>   	struct iag *iagp;
>> @@ -339,6 +339,9 @@ int diRead(struct inode *ip)
>>   
>>   	/* get the ag for the iag */
>>   	agstart = le64_to_cpu(iagp->agstart);
>> +	agno = BLKTOAG(agstart, JFS_SBI(ip->i_sb));
>> +	if(agno >= MAXAG || agno < 0)
>> +		return -EIO;
>>   
>>   	release_metapage(mp);
>>   
>> -- 
>> 2.34.1