RE: [f2fs-dev][PATCH 1/2] f2fs: check name_len of dir entry to prevent from deadloop

[Chao Yu]

Hi Jaegeuk,

> -----Original Message-----
> From: Jaegeuk Kim [mailto:jaegeuk@xxxxxxxxxx]
> Sent: Wednesday, July 09, 2014 9:48 PM
> To: Chao Yu
> Cc: 'Changman Lee'; linux-f2fs-devel@xxxxxxxxxxxxxxxxxxxxx; linux-fsdevel@xxxxxxxxxxxxxxx;
> linux-kernel@xxxxxxxxxxxxxxx
> Subject: Re: [f2fs-dev][PATCH 1/2] f2fs: check name_len of dir entry to prevent from deadloop
> 
> Hi Chao,
> 
> On Wed, Jul 09, 2014 at 10:57:43AM +0800, Chao Yu wrote:
> > Hi,
> >
> > > -----Original Message-----
> > > From: Jaegeuk Kim [mailto:jaegeuk@xxxxxxxxxx]
> > > Sent: Tuesday, July 08, 2014 1:56 PM
> > > To: Chao Yu
> > > Cc: 'Changman Lee'; linux-f2fs-devel@xxxxxxxxxxxxxxxxxxxxx; linux-fsdevel@xxxxxxxxxxxxxxx;
> > > linux-kernel@xxxxxxxxxxxxxxx
> > > Subject: Re: [f2fs-dev][PATCH 1/2] f2fs: check name_len of dir entry to prevent from deadloop
> > >
> > > On Mon, Jul 07, 2014 at 09:24:05AM +0800, Chao Yu wrote:
> > > > Hi Jaegeuk,
> > > >
> > > > > -----Original Message-----
> > > > > From: Jaegeuk Kim [mailto:jaegeuk@xxxxxxxxxx]
> > > > > Sent: Saturday, July 05, 2014 2:43 PM
> > > > > To: Chao Yu
> > > > > Cc: Changman Lee; linux-f2fs-devel@xxxxxxxxxxxxxxxxxxxxx;
> linux-fsdevel@xxxxxxxxxxxxxxx;
> > > > > linux-kernel@xxxxxxxxxxxxxxx
> > > > > Subject: Re: [f2fs-dev][PATCH 1/2] f2fs: check name_len of dir entry to prevent from
> deadloop
> > > > >
> > > > > Hi Chao,
> > > > >
> > > > > On Wed, Jul 02, 2014 at 01:23:47PM +0800, Chao Yu wrote:
> > > > > > We assume that modification of some special application could result in zeroed
> > > > > > name_len, or it is consciously made by somebody. We will deadloop in
> > > > > > find_in_block when name_len of dir entry is zero.
> > > > >
> > > > > Could you explain this a little bit more?
> > > > > I'm not sure how it can happen.
> > > >
> > > > IMO,
> > > > On one hand, programs like mkfs/fsck/img2simg and f2fs could directly operate
> > > > the raw device, so bugs of these software may be triggered to generate the
> > > > corrupt data such as zeroed name_len.
> > >
> > > Well...
> > > If such the programs try to corrupt the f2fs image crucially, the bug should be
> > > fixed inside the programs, not from the workaround through f2fs.
> >
> > IMO, software should have ability of self fault-tolerant, but not depend on correctness
> > of other related software. And I will hope there are no more other software which could
> > directly operate the raw device, to provide us such corrupted data as input.
> >
> > How about swifting to BUG_ON here?
> 
> Well, IMO, it would be good to add f2fs_bug_on() here with a specific comment.

Ok, I will modify this patch and resend it.

> 
> In the current phase of f2fs, it is more important to investigate the file
> system bugs, rather than workarounds for any corrupted images.
> And, definitely it needs to stop the kernel if any corrupted image was mounted,
> so that we can figure out where the bugs are occurred.

All right, I got it.
Thanks for the review and explanation in patience. :)

Regards,
Yu

> 
> Thanks,
> 
> >
> > >
> > > As I mentioned, even though f2fs avoids such the dead loop whatever at that
> > > moment, it will be operating with inconsistent file system status, resulting
> > > in system crash in the near furture finally.
> >
> > Agreed, we should avoid this. Previous solution with "break" is not suitable here.
> > Thanks for you reminder.
> >
> > >
> > > Why should we avoid this specific case only?
> >
> > Hmm... I just found this case could cause some issue when I review Gu's last patch.
> > As I check, several error cases of find_data_page in find_in_level could also cause
> > inconsistent status of fs as you said.
> >
> > > It seems that it is a kinda intentional user-made case.
> > > Is it really normal?
> >
> > It's really rare.
> >
> > >
> > > > On the other hand, it' could be treated as a potential security problem, because
> > > > user could crafted such a malicious image include zeroed name_len for hacking purpose.
> > >
> > > If user can try to do something like that, why do they write zeroed name_len only?
> > > To crash the system, they can do everything.
> >
> > If they have the whole right to access the system, certainly they do not have to do such
> > thing. I just assume that they only have the right to upload the crafted image, then use
> > social engineering in next "do mount" step. Or it's no need to worry about this?
> >
> > Thanks,
> > Yu
> >
> > >
> > > Thanks,
> > >
> > > > Once such special image is being mounted, deadloop could be triggered, finally will
> > > > result in effecting on linux system's running.
> > > >
> > > > Thanks,
> > > > Yu
> > > >
> > > > > I think the zeroed name_len would cause some problems in f2fs_add_link.
> > > > >
> > > > > Thanks,
> > > > >
> > > > > >
> > > > > > This patch is added for preventing deadloop in above scenario.
> > > > > >
> > > > > > Signed-off-by: Chao Yu <chao2.yu@xxxxxxxxxxx>
> > > > > > ---
> > > > > >  fs/f2fs/dir.c |   10 ++++++++++
> > > > > >  1 file changed, 10 insertions(+)
> > > > > >
> > > > > > diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
> > > > > > index be8c7af..4316ec5 100644
> > > > > > --- a/fs/f2fs/dir.c
> > > > > > +++ b/fs/f2fs/dir.c
> > > > > > @@ -121,6 +121,16 @@ static struct f2fs_dir_entry *find_in_block(struct page
> *dentry_page,
> > > > > >  			}
> > > > > >  		}
> > > > > >
> > > > > > +		/* check name_len to prevent from deadloop here */
> > > > > > +		if (unlikely(de->name_len == 0)) {
> > > > > > +			struct inode *inode = dentry_page->mapping->host;
> > > > > > +
> > > > > > +			f2fs_msg(inode->i_sb, KERN_ERR,
> > > > > > +				"zero-length dir entry, ino = %lu, name = %s",
> > > > > > +				(unsigned long)inode->i_ino, name->name);
> > > > > > +			break;
> > > > > > +		}
> > > > > > +
> > > > > >  		bit_start = bit_pos
> > > > > >  				+ GET_DENTRY_SLOTS(le16_to_cpu(de->name_len));
> > > > > >  	}
> > > > > > --
> > > > > > 1.7.9.5
> > > > >
> > > > > --
> > > > > Jaegeuk Kim
> > >
> > > --
> > > Jaegeuk Kim
> 
> --
> Jaegeuk Kim

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/