linux-kernel - Re: [PATCH] fs-writeback: fix NULL pointer dereference in __mark_inode

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

Message-ID: <20110228162909.GB4675@swordfish.minsk.epam.com>
Date:	Mon, 28 Feb 2011 18:29:09 +0200
From:	Sergey Senozhatsky <sergey.senozhatsky@...il.com>
To:	Andreas Bießmann <andreas.devel@...glemail.com>
Cc:	linux-kernel@...r.kernel.org,
	Alexander Viro <viro@...iv.linux.org.uk>,
	linux-fsdevel@...r.kernel.org
Subject: Re: [PATCH] fs-writeback: fix NULL pointer dereference in
 __mark_inode_dirty

On (02/28/11 16:59), Andreas Bießmann wrote:
> Dear Sergey Senozhatsky,
> 
> Am 28.02.2011 16:43, schrieb Sergey Senozhatsky:
> > On (02/28/11 16:25), Andreas Bießmann wrote:
> 
> >> The reference to sb->s_bdi may be deleted from mmc_blk_remove() ->
> >> del_gendisk() -> unlink_gendisk() -> bdi_unregister() -> bdi_prune_sb() while
> >> another instance try to write some data to the device.
> >>
> >> Signed-off-by: Andreas Bießmann <biessmann@...science.de>
> >> ---
> >>  fs/fs-writeback.c |    3 +++
> >>  1 files changed, 3 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
> >> index cdbf7ac..96b4b25 100644
> >> --- a/fs/fs-writeback.c
> >> +++ b/fs/fs-writeback.c
> >> @@ -1047,6 +1047,9 @@ void __mark_inode_dirty(struct inode *inode, int flags)
> >>  		if (!was_dirty) {
> >>  			bdi = inode_to_bdi(inode);
> >>  
> >> +			if (!bdi)
> >> +				goto out;
> >> +
> >>  			if (bdi_cap_writeback_dirty(bdi)) {
> >>  				WARN(!test_bit(BDI_registered, &bdi->state),
> >>  				     "bdi-%s not registered\n", bdi->name);
> > 
> > Hello,
> > I had something very similar to this some time ago
> > https://lkml.org/lkml/2010/12/9/436
> 
> Sorry, I did not see that patch.
>
No problem :-)
 
> > However, I'm not sure that this check is sufficient.
> 
> Why are you think this is not sufficient?
> If an instance try to write that specific inode to an physical device
> which is not longer available how should we react then?
> 

I think the path which `kills' the device should take care of that.
Otherwise, IMHO, we have:
- ok, we're falling on line 42 -- let's fix line 42

ignoring the fact that there are reasons which led to faulty line 42,
which are, for example:
#0 
 604     spin_lock(&sb_lock);
 605     list_for_each_entry(sb, &super_blocks, s_list) {
 606         if (sb->s_bdi == bdi)
 607             sb->s_bdi = NULL;
 608     }
 609     spin_unlock(&sb_lock);

#1 bdi_prune_sb                                         
#2 bdi_unregister                
#3 del_gendisk                   

Of course, I may be wrong.


> Another solution could be to clean up all instances referring to that
> superblock in del_/unlink_gendisk(). But I think to check the return of
> inode_to_bdi() is needed in any case.
> 


	Sergey

Content of type "application/pgp-signature" skipped