In alloc_inode(), inode_init_always() could return -ENOMEM if
security_inode_alloc() fails. If this happens for nilfs2,
nilfs_free_inode() is called without initializing inode->i_private and
nilfs_free_inode() wrongly calls nilfs_mdt_destroy(), which frees
uninitialized inode->i_private and can trigger a crash.
Fix this bug by initializing inode->i_private in nilfs_alloc_inode().
Link: https://lkml.kernel.org/r/CAFcO6XOcf1Jj2SeGt=jJV59wmhESeSKpfR0omdFRq+J9nD1vfQ@mail.gmail.com
Link: https://lkml.kernel.org/r/[email protected]
Reported-by: butt3rflyh4ck <[email protected]>
Reported-by: Hao Sun <[email protected]>
Reported-by: Jiacheng Xu <[email protected]>
Reported-by: Mudong Liang <[email protected]>
Signed-off-by: Ryusuke Konishi <[email protected]>
Cc: Al Viro <[email protected]>
---
fs/nilfs2/super.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/fs/nilfs2/super.c b/fs/nilfs2/super.c
index ba108f915391..aca5614f1b44 100644
--- a/fs/nilfs2/super.c
+++ b/fs/nilfs2/super.c
@@ -159,6 +159,7 @@ struct inode *nilfs_alloc_inode(struct super_block *sb)
ii->i_cno = 0;
ii->i_assoc_inode = NULL;
ii->i_bmap = &ii->i_bmap_data;
+ ii->vfs_inode.i_private = NULL;
return &ii->vfs_inode;
}
--
2.34.1
On Tue, Aug 16, 2022 at 02:51:14AM +0900, Ryusuke Konishi wrote:
> In alloc_inode(), inode_init_always() could return -ENOMEM if
> security_inode_alloc() fails. If this happens for nilfs2,
> nilfs_free_inode() is called without initializing inode->i_private and
> nilfs_free_inode() wrongly calls nilfs_mdt_destroy(), which frees
> uninitialized inode->i_private and can trigger a crash.
>
> Fix this bug by initializing inode->i_private in nilfs_alloc_inode().
>
> Link: https://lkml.kernel.org/r/CAFcO6XOcf1Jj2SeGt=jJV59wmhESeSKpfR0omdFRq+J9nD1vfQ@mail.gmail.com
> Link: https://lkml.kernel.org/r/[email protected]
> Reported-by: butt3rflyh4ck <[email protected]>
> Reported-by: Hao Sun <[email protected]>
> Reported-by: Jiacheng Xu <[email protected]>
> Reported-by: Mudong Liang <[email protected]>
> Signed-off-by: Ryusuke Konishi <[email protected]>
> Cc: Al Viro <[email protected]>
> ---
> fs/nilfs2/super.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/fs/nilfs2/super.c b/fs/nilfs2/super.c
> index ba108f915391..aca5614f1b44 100644
> --- a/fs/nilfs2/super.c
> +++ b/fs/nilfs2/super.c
> @@ -159,6 +159,7 @@ struct inode *nilfs_alloc_inode(struct super_block *sb)
> ii->i_cno = 0;
> ii->i_assoc_inode = NULL;
> ii->i_bmap = &ii->i_bmap_data;
> + ii->vfs_inode.i_private = NULL;
> return &ii->vfs_inode;
> }
FWIW, I think it's better to deal with that in inode_init_always(), but
not just moving ->i_private initialization up - we ought to move
security_inode_alloc() to the very end. No sense playing whack-a-mole
with further possible bugs of that sort...
On Tue, Aug 16, 2022 at 3:27 AM Al Viro <[email protected]> wrote:
>
> On Tue, Aug 16, 2022 at 02:51:14AM +0900, Ryusuke Konishi wrote:
> > In alloc_inode(), inode_init_always() could return -ENOMEM if
> > security_inode_alloc() fails. If this happens for nilfs2,
> > nilfs_free_inode() is called without initializing inode->i_private and
> > nilfs_free_inode() wrongly calls nilfs_mdt_destroy(), which frees
> > uninitialized inode->i_private and can trigger a crash.
> >
> > Fix this bug by initializing inode->i_private in nilfs_alloc_inode().
> >
> > Link: https://lkml.kernel.org/r/CAFcO6XOcf1Jj2SeGt=jJV59wmhESeSKpfR0omdFRq+J9nD1vfQ@mail.gmail.com
> > Link: https://lkml.kernel.org/r/[email protected]
> > Reported-by: butt3rflyh4ck <[email protected]>
> > Reported-by: Hao Sun <[email protected]>
> > Reported-by: Jiacheng Xu <[email protected]>
> > Reported-by: Mudong Liang <[email protected]>
> > Signed-off-by: Ryusuke Konishi <[email protected]>
> > Cc: Al Viro <[email protected]>
> > ---
> > fs/nilfs2/super.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/fs/nilfs2/super.c b/fs/nilfs2/super.c
> > index ba108f915391..aca5614f1b44 100644
> > --- a/fs/nilfs2/super.c
> > +++ b/fs/nilfs2/super.c
> > @@ -159,6 +159,7 @@ struct inode *nilfs_alloc_inode(struct super_block *sb)
> > ii->i_cno = 0;
> > ii->i_assoc_inode = NULL;
> > ii->i_bmap = &ii->i_bmap_data;
> > + ii->vfs_inode.i_private = NULL;
> > return &ii->vfs_inode;
> > }
>
> FWIW, I think it's better to deal with that in inode_init_always(), but
> not just moving ->i_private initialization up - we ought to move
> security_inode_alloc() to the very end. No sense playing whack-a-mole
> with further possible bugs of that sort...
Yes, I agree it's better if security_inode_alloc() is moved to the end as
possible in the sense of avoiding similar issues.
But, would that vfs change be safe to backport to stable trees?
It looks like the error handling for security_inode_alloc() is in the
middle of inode_init_always() for a very long time..
If you want to see the impact of the vfs change, I think it's one way
to apply this one in advance. Or if you want to fix it in one step,
I think it's good too. How do you feel about this ?
Thanks,
Ryusuke Konishi
On Tue, Aug 16, 2022 at 05:34:12AM +0900, Ryusuke Konishi wrote:
> Yes, I agree it's better if security_inode_alloc() is moved to the end as
> possible in the sense of avoiding similar issues.
> But, would that vfs change be safe to backport to stable trees?
Yes.
> It looks like the error handling for security_inode_alloc() is in the
> middle of inode_init_always() for a very long time..
Look at the initializations done after it. The only thing with effects
outside of inode itself is (since 2010) an increment of nr_inodes.
> If you want to see the impact of the vfs change, I think it's one way
> to apply this one in advance. Or if you want to fix it in one step,
> I think it's good too. How do you feel about this ?
IMO that should go into inode_init_always(), with Cc:stable. If you
(or Dongliang Mu, or anybody else) would post such variant with
reasonable commit message, I'll pick it into vfs.git and feed to Linus
in the next window. E.g. into #work.inode, with that branch being
made never-rebased, so that you could pull it into your development
branch as soon as it's there...
On Tue, Aug 16, 2022 at 8:04 AM Al Viro wrote:
>
> On Tue, Aug 16, 2022 at 05:34:12AM +0900, Ryusuke Konishi wrote:
>
> > Yes, I agree it's better if security_inode_alloc() is moved to the end as
> > possible in the sense of avoiding similar issues.
> > But, would that vfs change be safe to backport to stable trees?
>
> Yes.
>
> > It looks like the error handling for security_inode_alloc() is in the
> > middle of inode_init_always() for a very long time..
>
> Look at the initializations done after it. The only thing with effects
> outside of inode itself is (since 2010) an increment of nr_inodes.
>
> > If you want to see the impact of the vfs change, I think it's one way
> > to apply this one in advance. Or if you want to fix it in one step,
> > I think it's good too. How do you feel about this ?
>
> IMO that should go into inode_init_always(), with Cc:stable. If you
> (or Dongliang Mu, or anybody else) would post such variant with
> reasonable commit message, I'll pick it into vfs.git and feed to Linus
> in the next window. E.g. into #work.inode, with that branch being
> made never-rebased, so that you could pull it into your development
> branch as soon as it's there...
I agree with your thoughts on the course of action.
Andrew, I withdraw this patch.
Dongliang (or Jiacheng?), would it be possible for you to post a revised patch
against inode_init_always() that moves the call of security_inode_alloc()
instead of i_private initialization (as Al Viro said in a nearby thread [1]) ?
If you have time, I would like to leave it to you since you wrote the
original patch for inode_init_always().
[1] https://lkml.kernel.org/r/CAO4S-mficMz1mQW06EuCF+o11+mRDiCpufqVfoHkcRbQbs8kVw@mail.gmail.com
Thanks,
Ryusuke Konishi
On Tue, Aug 16, 2022 at 11:11 AM Ryusuke Konishi
<[email protected]> wrote:
>
> On Tue, Aug 16, 2022 at 8:04 AM Al Viro wrote:
> >
> > On Tue, Aug 16, 2022 at 05:34:12AM +0900, Ryusuke Konishi wrote:
> >
> > > Yes, I agree it's better if security_inode_alloc() is moved to the end as
> > > possible in the sense of avoiding similar issues.
> > > But, would that vfs change be safe to backport to stable trees?
> >
> > Yes.
> >
> > > It looks like the error handling for security_inode_alloc() is in the
> > > middle of inode_init_always() for a very long time..
> >
> > Look at the initializations done after it. The only thing with effects
> > outside of inode itself is (since 2010) an increment of nr_inodes.
> >
> > > If you want to see the impact of the vfs change, I think it's one way
> > > to apply this one in advance. Or if you want to fix it in one step,
> > > I think it's good too. How do you feel about this ?
> >
> > IMO that should go into inode_init_always(), with Cc:stable. If you
> > (or Dongliang Mu, or anybody else) would post such variant with
> > reasonable commit message, I'll pick it into vfs.git and feed to Linus
> > in the next window. E.g. into #work.inode, with that branch being
> > made never-rebased, so that you could pull it into your development
> > branch as soon as it's there...
>
> I agree with your thoughts on the course of action.
> Andrew, I withdraw this patch.
>
> Dongliang (or Jiacheng?), would it be possible for you to post a revised patch
> against inode_init_always() that moves the call of security_inode_alloc()
> instead of i_private initialization (as Al Viro said in a nearby thread [1]) ?
> If you have time, I would like to leave it to you since you wrote the
> original patch for inode_init_always().
Sure, I will post a v2 patch that moves security_inode_alloc to the
location just prior to
this_cpu_inc(nr_inodes);
with proper commit message.
>
> [1] https://lkml.kernel.org/r/CAO4S-mficMz1mQW06EuCF+o11+mRDiCpufqVfoHkcRbQbs8kVw@mail.gmail.com
>
> Thanks,
> Ryusuke Konishi
On Tue, Aug 16, 2022 at 12:25 PM Dongliang Mu wrote:
> > Dongliang (or Jiacheng?), would it be possible for you to post a revised patch
> > against inode_init_always() that moves the call of security_inode_alloc()
> > instead of i_private initialization (as Al Viro said in a nearby thread [1]) ?
> > If you have time, I would like to leave it to you since you wrote the
> > original patch for inode_init_always().
>
> Sure, I will post a v2 patch that moves security_inode_alloc to the
> location just prior to
> this_cpu_inc(nr_inodes);
> with proper commit message.
>
I saw you already sent the v2 patch on linux-fsdevel, etc.
Just thank you for your quick follow.
Regards,
Ryusuke Konishi
> >
> > [1] https://lkml.kernel.org/r/CAO4S-mficMz1mQW06EuCF+o11+mRDiCpufqVfoHkcRbQbs8kVw@mail.gmail.com
> >
> > Thanks,
> > Ryusuke Konishi