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.
This patch is added for preventing deadloop in above scenario.
Signed-off-by: Chao Yu <[email protected]>
---
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
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.
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 <[email protected]>
> ---
> 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
Hi Jaegeuk,
> -----Original Message-----
> From: Jaegeuk Kim [mailto:[email protected]]
> Sent: Saturday, July 05, 2014 2:43 PM
> To: Chao Yu
> Cc: Changman Lee; [email protected]; [email protected];
> [email protected]
> 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.
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.
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 <[email protected]>
> > ---
> > 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
On Mon, Jul 07, 2014 at 09:24:05AM +0800, Chao Yu wrote:
> Hi Jaegeuk,
>
> > -----Original Message-----
> > From: Jaegeuk Kim [mailto:[email protected]]
> > Sent: Saturday, July 05, 2014 2:43 PM
> > To: Chao Yu
> > Cc: Changman Lee; [email protected]; [email protected];
> > [email protected]
> > 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.
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.
Why should we avoid this specific case only?
It seems that it is a kinda intentional user-made case.
Is it really normal?
> 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.
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 <[email protected]>
> > > ---
> > > 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
Hi,
> -----Original Message-----
> From: Jaegeuk Kim [mailto:[email protected]]
> Sent: Tuesday, July 08, 2014 1:56 PM
> To: Chao Yu
> Cc: 'Changman Lee'; [email protected]; [email protected];
> [email protected]
> 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:[email protected]]
> > > Sent: Saturday, July 05, 2014 2:43 PM
> > > To: Chao Yu
> > > Cc: Changman Lee; [email protected]; [email protected];
> > > [email protected]
> > > 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?
>
> 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 <[email protected]>
> > > > ---
> > > > 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
Hi Chao,
On Wed, Jul 09, 2014 at 10:57:43AM +0800, Chao Yu wrote:
> Hi,
>
> > -----Original Message-----
> > From: Jaegeuk Kim [mailto:[email protected]]
> > Sent: Tuesday, July 08, 2014 1:56 PM
> > To: Chao Yu
> > Cc: 'Changman Lee'; [email protected]; [email protected];
> > [email protected]
> > 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:[email protected]]
> > > > Sent: Saturday, July 05, 2014 2:43 PM
> > > > To: Chao Yu
> > > > Cc: Changman Lee; [email protected]; [email protected];
> > > > [email protected]
> > > > 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.
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.
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 <[email protected]>
> > > > > ---
> > > > > 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
Hi Jaegeuk,
> -----Original Message-----
> From: Jaegeuk Kim [mailto:[email protected]]
> Sent: Wednesday, July 09, 2014 9:48 PM
> To: Chao Yu
> Cc: 'Changman Lee'; [email protected]; [email protected];
> [email protected]
> 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:[email protected]]
> > > Sent: Tuesday, July 08, 2014 1:56 PM
> > > To: Chao Yu
> > > Cc: 'Changman Lee'; [email protected]; [email protected];
> > > [email protected]
> > > 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:[email protected]]
> > > > > Sent: Saturday, July 05, 2014 2:43 PM
> > > > > To: Chao Yu
> > > > > Cc: Changman Lee; [email protected];
> [email protected];
> > > > > [email protected]
> > > > > 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 <[email protected]>
> > > > > > ---
> > > > > > 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