Messages in this thread | | | From | Dongliang Mu <> | Date | Fri, 14 Oct 2022 16:45:02 +0800 | Subject | Re: [PATCH] fs: jfs: fix shift-out-of-bounds in dbAllocAG |
| |
On Fri, Oct 14, 2022 at 4:55 AM Dave Kleikamp <dave.kleikamp@oracle.com> wrote: > > On 9/29/22 12:44AM, Dongliang Mu wrote: > > From: Dongliang Mu <mudongliangabcd@gmail.com> > > > > Syzbot found a crash : UBSAN: shift-out-of-bounds in dbAllocAG. The > > underlying bug is the missing check of bmp->db_agl2size. The field can > > be greater than 32 and trigger the shift-out-of-bounds. > > The shift is done to a 64-byte value, not 32. > > The volume can contain up to 2^43 blocks, which are divided up into 128 > (2^7) aggregate groups, so each AG can have 2^36 blocks, which makes 36 > a valid value for db_agl2size.
Thanks for your explanation. Will send a v2 patch after testing in Syzbot.
> > > > > Fix this bug by adding a check of bmp->db_agl2size in dbMount since this > > field is used in many following functions. Note that, for maintainance, > > I reorganzie the error handling code of dbMount. > > > > Reported-by: syzbot+15342c1aa6a00fb7a438@syzkaller.appspotmail.com > > Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com> > > --- > > fs/jfs/jfs_dmap.c | 21 +++++++++++++++------ > > 1 file changed, 15 insertions(+), 6 deletions(-) > > > > diff --git a/fs/jfs/jfs_dmap.c b/fs/jfs/jfs_dmap.c > > index 6b838d3ae7c2..4c717f245920 100644 > > --- a/fs/jfs/jfs_dmap.c > > +++ b/fs/jfs/jfs_dmap.c > > @@ -155,7 +155,7 @@ int dbMount(struct inode *ipbmap) > > struct bmap *bmp; > > struct dbmap_disk *dbmp_le; > > struct metapage *mp; > > - int i; > > + int i, err; > > > > /* > > * allocate/initialize the in-memory bmap descriptor > > @@ -170,8 +170,8 @@ int dbMount(struct inode *ipbmap) > > BMAPBLKNO << JFS_SBI(ipbmap->i_sb)->l2nbperpage, > > PSIZE, 0); > > if (mp == NULL) { > > - kfree(bmp); > > - return -EIO; > > + err = -EIO; > > + goto err_kfree_bmp; > > } > > > > /* copy the on-disk bmap descriptor to its in-memory version. */ > > @@ -181,9 +181,8 @@ int dbMount(struct inode *ipbmap) > > bmp->db_l2nbperpage = le32_to_cpu(dbmp_le->dn_l2nbperpage); > > bmp->db_numag = le32_to_cpu(dbmp_le->dn_numag); > > if (!bmp->db_numag) { > > - release_metapage(mp); > > - kfree(bmp); > > - return -EINVAL; > > + err = -EINVAL; > > + goto err_release_metapage; > > } > > > > bmp->db_maxlevel = le32_to_cpu(dbmp_le->dn_maxlevel); > > @@ -194,6 +193,10 @@ int dbMount(struct inode *ipbmap) > > bmp->db_agwidth = le32_to_cpu(dbmp_le->dn_agwidth); > > bmp->db_agstart = le32_to_cpu(dbmp_le->dn_agstart); > > bmp->db_agl2size = le32_to_cpu(dbmp_le->dn_agl2size); > > + if (bmp->db_agl2size >= 32) { > > I think the right number here is MAXMAPSIZE - L2MAXAG, so > > if (bmp->db_agl2size > (MAXMAPSIZE - L2MAXAG)) { > > > + err = -EINVAL; > > + goto err_release_metapage; > > + } > > for (i = 0; i < MAXAG; i++) > > bmp->db_agfree[i] = le64_to_cpu(dbmp_le->dn_agfree[i]); > > bmp->db_agsize = le64_to_cpu(dbmp_le->dn_agsize); > > @@ -214,6 +217,12 @@ int dbMount(struct inode *ipbmap) > > BMAP_LOCK_INIT(bmp); > > > > return (0); > > + > > +err_release_metapage: > > + release_metapage(mp); > > +err_kfree_bmp: > > + kfree(bmp); > > + return err; > > } > > > >
| |