Joseph Qi
2022-Apr-09 13:30 UTC
[Ocfs2-devel] [PATCH v1 3/5] ocfs2: ocfs2_initialize_super does cleanup job before return error
On 4/8/22 6:30 PM, Heming Zhao wrote:> After this patch, when error, ocfs2_fill_super doesn't take care to > release resources which are allocated in ocfs2_initialize_super. > > Signed-off-by: Heming Zhao <heming.zhao at suse.com> > --- > fs/ocfs2/super.c | 58 +++++++++++++++++++++++++++++++++--------------- > 1 file changed, 40 insertions(+), 18 deletions(-) > > diff --git a/fs/ocfs2/super.c b/fs/ocfs2/super.c > index f91c5510bc7e..8443ba031dec 100644 > --- a/fs/ocfs2/super.c > +++ b/fs/ocfs2/super.c > @@ -2023,7 +2023,7 @@ static int ocfs2_initialize_super(struct super_block *sb, > if (!osb) { > status = -ENOMEM; > mlog_errno(status); > - goto bail; > + goto out; > } > > sb->s_fs_info = osb; > @@ -2084,7 +2084,7 @@ static int ocfs2_initialize_super(struct super_block *sb, > mlog(ML_ERROR, "Invalid number of node slots (%u)\n", > osb->max_slots); > status = -EINVAL; > - goto bail; > + goto out; > } > > ocfs2_orphan_scan_init(osb); > @@ -2093,7 +2093,7 @@ static int ocfs2_initialize_super(struct super_block *sb, > if (status) { > mlog(ML_ERROR, "Unable to initialize recovery state\n"); > mlog_errno(status); > - goto bail; > + goto out; > } > > init_waitqueue_head(&osb->checkpoint_event); > @@ -2117,7 +2117,7 @@ static int ocfs2_initialize_super(struct super_block *sb, > if (!osb->vol_label) { > mlog(ML_ERROR, "unable to alloc vol label\n"); > status = -ENOMEM; > - goto bail; > + goto out_recovery_map; > } > > osb->slot_recovery_generations > @@ -2126,7 +2126,7 @@ static int ocfs2_initialize_super(struct super_block *sb, > if (!osb->slot_recovery_generations) { > status = -ENOMEM; > mlog_errno(status); > - goto bail; > + goto out_vol_label; > } > > init_waitqueue_head(&osb->osb_wipe_event); > @@ -2136,7 +2136,7 @@ static int ocfs2_initialize_super(struct super_block *sb, > if (!osb->osb_orphan_wipes) { > status = -ENOMEM; > mlog_errno(status); > - goto bail; > + goto out_slot_recovery_gen; > } > > osb->osb_rf_lock_tree = RB_ROOT; > @@ -2152,13 +2152,13 @@ static int ocfs2_initialize_super(struct super_block *sb, > mlog(ML_ERROR, "couldn't mount because of unsupported " > "optional features (%x).\n", i); > status = -EINVAL; > - goto bail; > + goto out_orphan_wipes; > } > if (!sb_rdonly(osb->sb) && (i = OCFS2_HAS_RO_COMPAT_FEATURE(osb->sb, ~OCFS2_FEATURE_RO_COMPAT_SUPP))) { > mlog(ML_ERROR, "couldn't mount RDWR because of " > "unsupported optional features (%x).\n", i); > status = -EINVAL; > - goto bail; > + goto out_orphan_wipes; > } > > if (ocfs2_clusterinfo_valid(osb)) { > @@ -2179,7 +2179,7 @@ static int ocfs2_initialize_super(struct super_block *sb, > "cluster stack label (%s) \n", > osb->osb_cluster_stack); > status = -EINVAL; > - goto bail; > + goto out_orphan_wipes; > } > memcpy(osb->osb_cluster_name, > OCFS2_RAW_SB(di)->s_cluster_info.ci_cluster, > @@ -2204,7 +2204,7 @@ static int ocfs2_initialize_super(struct super_block *sb, > if (!journal) { > mlog(ML_ERROR, "unable to alloc journal\n"); > status = -ENOMEM; > - goto bail; > + goto out_orphan_wipes; > } > osb->journal = journal; > journal->j_osb = osb; > @@ -2231,7 +2231,7 @@ static int ocfs2_initialize_super(struct super_block *sb, > mlog(ML_ERROR, "Volume has invalid cluster size (%d)\n", > osb->s_clustersize); > status = -EINVAL; > - goto bail; > + goto out_journal; > } > > total_blocks = ocfs2_clusters_to_blocks(osb->sb, > @@ -2243,14 +2243,14 @@ static int ocfs2_initialize_super(struct super_block *sb, > mlog(ML_ERROR, "Volume too large " > "to mount safely on this system"); > status = -EFBIG; > - goto bail; > + goto out_journal; > } > > if (ocfs2_setup_osb_uuid(osb, di->id2.i_super.s_uuid, > sizeof(di->id2.i_super.s_uuid))) { > mlog(ML_ERROR, "Out of memory trying to setup our uuid.\n"); > status = -ENOMEM; > - goto bail; > + goto out_journal; > } > > strlcpy(osb->vol_label, di->id2.i_super.s_label, > @@ -2270,7 +2270,7 @@ static int ocfs2_initialize_super(struct super_block *sb, > if (!osb->osb_dlm_debug) { > status = -ENOMEM; > mlog_errno(status); > - goto bail; > + goto out_uuid_str; > } > > atomic_set(&osb->vol_state, VOLUME_INIT); > @@ -2279,7 +2279,7 @@ static int ocfs2_initialize_super(struct super_block *sb, > status = ocfs2_init_global_system_inodes(osb); > if (status < 0) { > mlog_errno(status); > - goto bail; > + goto out_dlm_out; > } > > /* > @@ -2290,7 +2290,7 @@ static int ocfs2_initialize_super(struct super_block *sb, > if (!inode) { > status = -EINVAL; > mlog_errno(status); > - goto bail; > + goto out_system_inodes; > } > > osb->bitmap_blkno = OCFS2_I(inode)->ip_blkno; > @@ -2303,16 +2303,38 @@ static int ocfs2_initialize_super(struct super_block *sb, > status = ocfs2_init_slot_info(osb); > if (status < 0) { > mlog_errno(status); > - goto bail; > + goto out_system_inodes; > } > > osb->ocfs2_wq = alloc_ordered_workqueue("ocfs2_wq", WQ_MEM_RECLAIM); > if (!osb->ocfs2_wq) { > status = -ENOMEM; > mlog_errno(status); > + goto out_slot_info; > } > > -bail: > + return status; > + > +out_slot_info: > + ocfs2_free_slot_info(osb); > +out_system_inodes: > + ocfs2_release_system_inodes(osb); > +out_dlm_out: > + ocfs2_put_dlm_debug(osb->osb_dlm_debug); > +out_uuid_str: > + kfree(osb->uuid_str); > +out_journal: > + kfree(osb->journal); > +out_orphan_wipes: > + kfree(osb->osb_orphan_wipes); > +out_slot_recovery_gen: > + kfree(osb->slot_recovery_generations); > +out_vol_label: > + kfree(osb->vol_label); > +out_recovery_map: > + kfree(osb->recovery_map); > +out: > + kfree(osb);Should set osb to NULL here to prevent UAF in ocfs2_fill_super(). Thanks, Joseph> return status; > } >
heming.zhao at suse.com
2022-Apr-09 16:47 UTC
[Ocfs2-devel] [PATCH v1 3/5] ocfs2: ocfs2_initialize_super does cleanup job before return error
On 4/9/22 21:30, Joseph Qi wrote:> > > On 4/8/22 6:30 PM, Heming Zhao wrote: >> After this patch, when error, ocfs2_fill_super doesn't take care to >> release resources which are allocated in ocfs2_initialize_super. >> >> Signed-off-by: Heming Zhao <heming.zhao at suse.com> >> --- >> fs/ocfs2/super.c | 58 +++++++++++++++++++++++++++++++++--------------- >> 1 file changed, 40 insertions(+), 18 deletions(-) >> >> diff --git a/fs/ocfs2/super.c b/fs/ocfs2/super.c >> index f91c5510bc7e..8443ba031dec 100644 >> --- a/fs/ocfs2/super.c >> +++ b/fs/ocfs2/super.c >> @@ -2023,7 +2023,7 @@ static int ocfs2_initialize_super(struct super_block *sb, >> if (!osb) { >> status = -ENOMEM; >> mlog_errno(status); >> - goto bail; >> + goto out; >> } >> ... ... >> >> -bail: >> + return status; >> + >> +out_slot_info: >> + ocfs2_free_slot_info(osb); >> +out_system_inodes: >> + ocfs2_release_system_inodes(osb); >> +out_dlm_out: >> + ocfs2_put_dlm_debug(osb->osb_dlm_debug); >> +out_uuid_str: >> + kfree(osb->uuid_str); >> +out_journal: >> + kfree(osb->journal); >> +out_orphan_wipes: >> + kfree(osb->osb_orphan_wipes); >> +out_slot_recovery_gen: >> + kfree(osb->slot_recovery_generations); >> +out_vol_label: >> + kfree(osb->vol_label); >> +out_recovery_map: >> + kfree(osb->recovery_map); >> +out: >> + kfree(osb); > > Should set osb to NULL here to prevent UAF in ocfs2_fill_super(). >Your concern only valid with patch 1/5+2/5+3/5, but after 5/5, the UAF won't be triggered. ocfs2_initialize_super() failure will directly jump (by "goto out") to return. Thanks, Heming> >> return status; >> } >> >