On Fri, Oct 13, 2017 at 01:39:13PM -0400, Nicolas Pitre wrote:
> On Fri, 13 Oct 2017, Al Viro wrote:
>
> > On Thu, Oct 12, 2017 at 02:16:10AM -0400, Nicolas Pitre wrote:
> >
> > > static void cramfs_kill_sb(struct super_block *sb)
> > > {
> > > struct cramfs_sb_info *sbi = CRAMFS_SB(sb);
> > >
> > > - kill_block_super(sb);
> > > + if (IS_ENABLED(CCONFIG_CRAMFS_MTD)) {
> > > + if (sbi->mtd_point_size)
> > > + mtd_unpoint(sb->s_mtd, 0, sbi->mtd_point_size);
> > > + if (sb->s_mtd)
> > > + kill_mtd_super(sb);
> >
> > ...
> >
> > > + mtd_unpoint(sb->s_mtd, 0, PAGE_SIZE);
> > > + err = mtd_point(sb->s_mtd, 0, sbi->size, &sbi->mtd_point_size,
> > > + &sbi->linear_virt_addr, &sbi->linear_phys_addr);
> > > + if (err || sbi->mtd_point_size != sbi->size) {
> >
> > What happens if that mtd_point() fails? Note that ->kill_sb() will be
> > called anyway and ->mtd_point_size is going to be non-zero here...
>
> mtd_point() always clears sbi->mtd_point_size first thing upon entry
> even before it has a chance to fail. So it it fails then
> sbi->mtd_point_size will be zero and ->kill_sb() will skip the unpoint
> call.
OK... I wonder if it should simply define stubs for kill_mtd_super(),
mtd_unpoint() and kill_block_super() in !CONFIG_MTD and !CONFIG_BLOCK
cases. mount_mtd() and mount_bdev() as well - e.g. mount_bdev()
returning ERR_PTR(-ENODEV) and kill_block_super() being simply BUG()
in !CONFIG_BLOCK case. Then cramfs_kill_sb() would be
if (sb->s_mtd) {
if (sbi->mtd_point_size)
mtd_unpoint(sb->s_mtd, 0, sbi->mtd_point_size);
kill_mtd_super(sb);
} else {
kill_block_super(sb);
}
kfree(sbi);
Wait. Looking at that code... what happens if you hit this failure
exit:
sbi = kzalloc(sizeof(struct cramfs_sb_info), GFP_KERNEL);
if (!sbi)
return -ENOMEM;
Current cramfs_kill_sb() will do kill_block_super() and kfree(NULL), which
works nicely, but you are dereferencing that sucker, not just passing it
to kfree(). IOW, that if (sbi->....) ought to be if (sbi && sbi->...)
