[PATCH 1/3] f2fs: check filename length in recover_dentry

[PATCH 1/3] f2fs: check filename length in recover_dentry

[Chao Yu]

In current flow, we will get Null return value of f2fs_find_entry in
recover_dentry when name.len is bigger than F2FS_NAME_LEN, and then we
still add this inode into its dir entry.
To avoid this situation, we must check filename length before we use it.

Another point is that we could remove the code of checking filename length
In f2fs_find_entry, because f2fs_lookup will be called previously to ensure of
validity of filename length.

Signed-off-by: Chao Yu <chao2.yu@samsung.com>
---
 fs/f2fs/dir.c      |    3 ---
 fs/f2fs/recovery.c |    5 +++++
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
index 0cc26ba..3f3b661 100644
--- a/fs/f2fs/dir.c
+++ b/fs/f2fs/dir.c
@@ -190,9 +190,6 @@ struct f2fs_dir_entry *f2fs_find_entry(struct inode *dir,
 	unsigned int max_depth;
 	unsigned int level;
 
-	if (unlikely(namelen > F2FS_NAME_LEN))
-		return NULL;
-
 	if (npages == 0)
 		return NULL;
 
diff --git a/fs/f2fs/recovery.c b/fs/f2fs/recovery.c
index a3f4542..fdd175b 100644
--- a/fs/f2fs/recovery.c
+++ b/fs/f2fs/recovery.c
@@ -62,6 +62,11 @@ static int recover_dentry(struct page *ipage, struct inode *inode)
 
 	name.len = le32_to_cpu(raw_inode->i_namelen);
 	name.name = raw_inode->i_name;
+
+	if (unlikely(name.len > F2FS_NAME_LEN)) {
+		err = -ENAMETOOLONG;
+		goto out;
+	}
 retry:
 	de = f2fs_find_entry(dir, &name, &page);
 	if (de && inode->i_ino == le32_to_cpu(de->ino))
-- 
1.7.9.5


------------------------------------------------------------------------------
Rapidly troubleshoot problems before they affect your business. Most IT 
organizations don't have a clear picture of how application performance 
affects their revenue. With AppDynamics, you get 100% visibility into your 
Java,.NET, & PHP application. Start your 15-day FREE TRIAL of AppDynamics Pro!
http://pubads.g.doubleclick.net/gampad/clk?id=84349831&iu=/4140/ostg.clktrk

Re: [PATCH 1/3] f2fs: check filename length in recover_dentry

[Jaegeuk Kim]

2013-12-21 (토), 18:01 +0800, Chao Yu:
> In current flow, we will get Null return value of f2fs_find_entry in
> recover_dentry when name.len is bigger than F2FS_NAME_LEN, and then we
> still add this inode into its dir entry.
> To avoid this situation, we must check filename length before we use it.
> 
> Another point is that we could remove the code of checking filename length
> In f2fs_find_entry, because f2fs_lookup will be called previously to ensure of
> validity of filename length.
> 
> Signed-off-by: Chao Yu <chao2.yu@samsung.com>
> ---
>  fs/f2fs/dir.c      |    3 ---
>  fs/f2fs/recovery.c |    5 +++++
>  2 files changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
> index 0cc26ba..3f3b661 100644
> --- a/fs/f2fs/dir.c
> +++ b/fs/f2fs/dir.c
> @@ -190,9 +190,6 @@ struct f2fs_dir_entry *f2fs_find_entry(struct inode *dir,
>  	unsigned int max_depth;
>  	unsigned int level;
>  
> -	if (unlikely(namelen > F2FS_NAME_LEN))
> -		return NULL;
> -
>  	if (npages == 0)
>  		return NULL;
>  
> diff --git a/fs/f2fs/recovery.c b/fs/f2fs/recovery.c
> index a3f4542..fdd175b 100644
> --- a/fs/f2fs/recovery.c
> +++ b/fs/f2fs/recovery.c
> @@ -62,6 +62,11 @@ static int recover_dentry(struct page *ipage, struct inode *inode)
>  
>  	name.len = le32_to_cpu(raw_inode->i_namelen);
>  	name.name = raw_inode->i_name;
> +
> +	if (unlikely(name.len > F2FS_NAME_LEN)) {
> +		err = -ENAMETOOLONG;
> +		goto out;
> +	}

Have you seen this before?
This is a trivial bug case, so, if you have got this bug, we should fix
the bug first instead of adding any workaround patch.
Let's add WARN_ON() at least.
Thanks,

>  retry:
>  	de = f2fs_find_entry(dir, &name, &page);
>  	if (de && inode->i_ino == le32_to_cpu(de->ino))

-- 
Jaegeuk Kim
Samsung



------------------------------------------------------------------------------
Rapidly troubleshoot problems before they affect your business. Most IT 
organizations don't have a clear picture of how application performance 
affects their revenue. With AppDynamics, you get 100% visibility into your 
Java,.NET, & PHP application. Start your 15-day FREE TRIAL of AppDynamics Pro!
http://pubads.g.doubleclick.net/gampad/clk?id=84349831&iu=/4140/ostg.clktrk
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

RE: [f2fs-dev] [PATCH 1/3] f2fs: check filename length in recover_dentry

[Chao Yu]

Hi Kim,

> -----Original Message-----
> From: Jaegeuk Kim [mailto:jaegeuk.kim@samsung.com]
> Sent: Monday, December 23, 2013 9:26 AM
> To: Chao Yu
> Cc: linux-fsdevel@vger.kernel.org; linux-kernel@vger.kernel.org; linux-f2fs-devel@lists.sourceforge.net
> Subject: Re: [f2fs-dev] [PATCH 1/3] f2fs: check filename length in recover_dentry
> 
> 2013-12-21 (토), 18:01 +0800, Chao Yu:
> > In current flow, we will get Null return value of f2fs_find_entry in
> > recover_dentry when name.len is bigger than F2FS_NAME_LEN, and then we
> > still add this inode into its dir entry.
> > To avoid this situation, we must check filename length before we use it.
> >
> > Another point is that we could remove the code of checking filename length
> > In f2fs_find_entry, because f2fs_lookup will be called previously to ensure of
> > validity of filename length.
> >
> > Signed-off-by: Chao Yu <chao2.yu@samsung.com>
> > ---
> >  fs/f2fs/dir.c      |    3 ---
> >  fs/f2fs/recovery.c |    5 +++++
> >  2 files changed, 5 insertions(+), 3 deletions(-)
> >
> > diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
> > index 0cc26ba..3f3b661 100644
> > --- a/fs/f2fs/dir.c
> > +++ b/fs/f2fs/dir.c
> > @@ -190,9 +190,6 @@ struct f2fs_dir_entry *f2fs_find_entry(struct inode *dir,
> >  	unsigned int max_depth;
> >  	unsigned int level;
> >
> > -	if (unlikely(namelen > F2FS_NAME_LEN))
> > -		return NULL;
> > -
> >  	if (npages == 0)
> >  		return NULL;
> >
> > diff --git a/fs/f2fs/recovery.c b/fs/f2fs/recovery.c
> > index a3f4542..fdd175b 100644
> > --- a/fs/f2fs/recovery.c
> > +++ b/fs/f2fs/recovery.c
> > @@ -62,6 +62,11 @@ static int recover_dentry(struct page *ipage, struct inode *inode)
> >
> >  	name.len = le32_to_cpu(raw_inode->i_namelen);
> >  	name.name = raw_inode->i_name;
> > +
> > +	if (unlikely(name.len > F2FS_NAME_LEN)) {
> > +		err = -ENAMETOOLONG;
> > +		goto out;
> > +	}
> 
> Have you seen this before?

Not yet.

> This is a trivial bug case, so, if you have got this bug, we should fix
> the bug first instead of adding any workaround patch.

What I worry about is that not only f2fs bug lead to this trivial problem,
but also other program with operation in raw disk could do this.

> Let's add WARN_ON() at least.

Alright.
Thanks.

> Thanks,
> 
> >  retry:
> >  	de = f2fs_find_entry(dir, &name, &page);
> >  	if (de && inode->i_ino == le32_to_cpu(de->ino))
> 
> --
> Jaegeuk Kim
> Samsung