cmnt: [X] [PATCH] fixes for LP: #1415636/CVE-2015-1350
khalid.elmously at canonical.com
Mon Aug 6 04:11:34 UTC 2018
On 2018-08-02 10:29:59 , Andy Whitcroft wrote:
> On Thu, Aug 02, 2018 at 12:00:13PM +0300, José Pekkarinen wrote:
> > On Thursday, 2 August 2018 08:44:30 EEST Khaled Elmously wrote:
> > [snip]
> > > > + struct dentry *dentry = inode->i_sb->s_root;
> > >
> > > I'm not sure I understand the above line. Can you please explain why it's
> > > true?
> > Seems to be the only way to find the dentry struct associated to an inode,
> > in newer kernels most of the functions modified in this patch already receives
> > a dentry instead of a raw inode, but for the remaining cases something needs
> > to be tailored. I failed to find any macro to help with this endeavour.
> That isn't 'the' dentry associated with an inode, that is the dentry
> associated with root of the filesystem. Surely you would have to
> iterate over the inode->i_dentry list looking for a valid one.
José, as Andy pointed out, this isn't the right dentry for the inode. With this change you'd be doing a permission check against a completely different inode.
I wasn't really sure if iterating over the dentry list is correct either, but I think it might actually be OK, since you're going to re-dereference the inode again as soon as you're in inode_change_ok() and from my understanding it should always lead you back to the same inode again.
For what it's worth, d_find_alias could be used instead of iterating manually.
Note you have a similar change made for fuse_do_truncate() - that would need to be fixed as well.
Once those are fixed, I think I would be OK with this change for fixing CVE-2015-1350.
More information about the kernel-team