Re: Linux 3.18.111
On 2018년 08월 10일 19:11, Greg Kroah-Hartman wrote:
> On Fri, Aug 10, 2018 at 03:43:02PM +0900, Seung-Woo Kim wrote:
>> On 2018년 08월 08일 19:06, Seung-Woo Kim wrote:
>>> On 2018년 07월 05일 09:52, Al Viro wrote:
On Mon, Jul 02, 2018 at 10:01:25PM -0700, Linus Torvalds wrote:
> On Mon, Jul 2, 2018 at 9:43 PM Seung-Woo Kim
> wrote:
>>
>> I think the commit itself is required. Simple, but not reliable,
>> workaround fix is like below:
>>
>> diff --git a/fs/dcache.c b/fs/dcache.c
>> index a34d401..7c751f2 100644
>> --- a/fs/dcache.c
>> +++ b/fs/dcache.c
>> @@ -1879,6 +1879,8 @@ void d_instantiate_new(struct dentry *entry,
>> struct inode *inode)
>> BUG_ON(!hlist_unhashed(&entry->d_u.d_alias));
>> BUG_ON(!inode);
>> lockdep_annotate_inode_mutex_key(inode);
>> + /* WORKAROUND for calling security_d_instantiate() */
>> + entry->d_inode = inode;
>> security_d_instantiate(entry, inode);
>> spin_lock(&inode->i_lock);
>> __d_instantiate(entry, inode);
>
> Ugh. That looks horrible even if it might avoid the oops.
>
> I think a much better solution is to back-port commit b296821a7c42
> ("xattr_handler: pass dentry and inode as separate arguments of
> ->get()") to older kernels. Then the inode is passed down all the way,
> and you don't have people try to get it from the (not yet initialized)
> dentry.
>
> But there might be other parts missing too, and I didn't look at how
> easy/painful that backport would be.
>
> Al - comments? This is all because of commit 1e2e547a93a0 ("do
> d_instantiate/unlock_new_inode combinations safely") being marked for
> stable, and various cases of security_d_instantiate() calling down to
> getxattr. Which used to not get the inode at all, so those older
> kernels use d_inode(dentry), which doesn't work in this path since
> dentry->d_inode hasn't been instantiated yet..
You also want b96809173e94 and ce23e6401334 there...
>>>
>>> For above two commits, also b296821a7c42 is required. And after
>>> backport, smack still crashed because setxattr. To fix it, 5930122683df
>>> and 3767e255b390 are also required.
>>>
>>> By the way, does no one have met this kind getxattr crash issue with
>>> selinux from 3.18.y?
>>>
>>
>> I have checked with selinux, and it is confirmed that there is no crash
>> because selinux_d_instantiate() has null check for inode. So, it is only
>> security smack issue.
>
> So are the 5 patches you sent ok to apply to the 3.18-stable tree? Or
> do we need to do something else?
>
Those 5 patches are fine in my smack environment. I have not tested all
the file systems in run-time except ext2/4 and I only tested build for
those file systems.
Best Regards,
- Seung-Woo Kim
> thanks,
>
> greg k-h
>
>
Re: Linux 3.18.111
On Fri, Aug 10, 2018 at 03:43:02PM +0900, Seung-Woo Kim wrote:
> On 2018년 08월 08일 19:06, Seung-Woo Kim wrote:
> > On 2018년 07월 05일 09:52, Al Viro wrote:
> >> On Mon, Jul 02, 2018 at 10:01:25PM -0700, Linus Torvalds wrote:
> >>> On Mon, Jul 2, 2018 at 9:43 PM Seung-Woo Kim
> >>> wrote:
>
> I think the commit itself is required. Simple, but not reliable,
> workaround fix is like below:
>
> diff --git a/fs/dcache.c b/fs/dcache.c
> index a34d401..7c751f2 100644
> --- a/fs/dcache.c
> +++ b/fs/dcache.c
> @@ -1879,6 +1879,8 @@ void d_instantiate_new(struct dentry *entry,
> struct inode *inode)
> BUG_ON(!hlist_unhashed(&entry->d_u.d_alias));
> BUG_ON(!inode);
> lockdep_annotate_inode_mutex_key(inode);
> + /* WORKAROUND for calling security_d_instantiate() */
> + entry->d_inode = inode;
> security_d_instantiate(entry, inode);
> spin_lock(&inode->i_lock);
> __d_instantiate(entry, inode);
> >>>
> >>> Ugh. That looks horrible even if it might avoid the oops.
> >>>
> >>> I think a much better solution is to back-port commit b296821a7c42
> >>> ("xattr_handler: pass dentry and inode as separate arguments of
> >>> ->get()") to older kernels. Then the inode is passed down all the way,
> >>> and you don't have people try to get it from the (not yet initialized)
> >>> dentry.
> >>>
> >>> But there might be other parts missing too, and I didn't look at how
> >>> easy/painful that backport would be.
> >>>
> >>> Al - comments? This is all because of commit 1e2e547a93a0 ("do
> >>> d_instantiate/unlock_new_inode combinations safely") being marked for
> >>> stable, and various cases of security_d_instantiate() calling down to
> >>> getxattr. Which used to not get the inode at all, so those older
> >>> kernels use d_inode(dentry), which doesn't work in this path since
> >>> dentry->d_inode hasn't been instantiated yet..
> >>
> >> You also want b96809173e94 and ce23e6401334 there...
> >
> > For above two commits, also b296821a7c42 is required. And after
> > backport, smack still crashed because setxattr. To fix it, 5930122683df
> > and 3767e255b390 are also required.
> >
> > By the way, does no one have met this kind getxattr crash issue with
> > selinux from 3.18.y?
> >
>
> I have checked with selinux, and it is confirmed that there is no crash
> because selinux_d_instantiate() has null check for inode. So, it is only
> security smack issue.
So are the 5 patches you sent ok to apply to the 3.18-stable tree? Or
do we need to do something else?
thanks,
greg k-h
Re: Linux 3.18.111
On 2018년 08월 08일 19:06, Seung-Woo Kim wrote:
> On 2018년 07월 05일 09:52, Al Viro wrote:
>> On Mon, Jul 02, 2018 at 10:01:25PM -0700, Linus Torvalds wrote:
>>> On Mon, Jul 2, 2018 at 9:43 PM Seung-Woo Kim wrote:
I think the commit itself is required. Simple, but not reliable,
workaround fix is like below:
diff --git a/fs/dcache.c b/fs/dcache.c
index a34d401..7c751f2 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -1879,6 +1879,8 @@ void d_instantiate_new(struct dentry *entry,
struct inode *inode)
BUG_ON(!hlist_unhashed(&entry->d_u.d_alias));
BUG_ON(!inode);
lockdep_annotate_inode_mutex_key(inode);
+ /* WORKAROUND for calling security_d_instantiate() */
+ entry->d_inode = inode;
security_d_instantiate(entry, inode);
spin_lock(&inode->i_lock);
__d_instantiate(entry, inode);
>>>
>>> Ugh. That looks horrible even if it might avoid the oops.
>>>
>>> I think a much better solution is to back-port commit b296821a7c42
>>> ("xattr_handler: pass dentry and inode as separate arguments of
>>> ->get()") to older kernels. Then the inode is passed down all the way,
>>> and you don't have people try to get it from the (not yet initialized)
>>> dentry.
>>>
>>> But there might be other parts missing too, and I didn't look at how
>>> easy/painful that backport would be.
>>>
>>> Al - comments? This is all because of commit 1e2e547a93a0 ("do
>>> d_instantiate/unlock_new_inode combinations safely") being marked for
>>> stable, and various cases of security_d_instantiate() calling down to
>>> getxattr. Which used to not get the inode at all, so those older
>>> kernels use d_inode(dentry), which doesn't work in this path since
>>> dentry->d_inode hasn't been instantiated yet..
>>
>> You also want b96809173e94 and ce23e6401334 there...
>
> For above two commits, also b296821a7c42 is required. And after
> backport, smack still crashed because setxattr. To fix it, 5930122683df
> and 3767e255b390 are also required.
>
> By the way, does no one have met this kind getxattr crash issue with
> selinux from 3.18.y?
>
I have checked with selinux, and it is confirmed that there is no crash
because selinux_d_instantiate() has null check for inode. So, it is only
security smack issue.
--
Seung-Woo Kim
Samsung Research
--
Re: Linux 3.18.111
On 2018년 07월 05일 09:52, Al Viro wrote:
> On Mon, Jul 02, 2018 at 10:01:25PM -0700, Linus Torvalds wrote:
>> On Mon, Jul 2, 2018 at 9:43 PM Seung-Woo Kim wrote:
>>>
>>> I think the commit itself is required. Simple, but not reliable,
>>> workaround fix is like below:
>>>
>>> diff --git a/fs/dcache.c b/fs/dcache.c
>>> index a34d401..7c751f2 100644
>>> --- a/fs/dcache.c
>>> +++ b/fs/dcache.c
>>> @@ -1879,6 +1879,8 @@ void d_instantiate_new(struct dentry *entry,
>>> struct inode *inode)
>>> BUG_ON(!hlist_unhashed(&entry->d_u.d_alias));
>>> BUG_ON(!inode);
>>> lockdep_annotate_inode_mutex_key(inode);
>>> + /* WORKAROUND for calling security_d_instantiate() */
>>> + entry->d_inode = inode;
>>> security_d_instantiate(entry, inode);
>>> spin_lock(&inode->i_lock);
>>> __d_instantiate(entry, inode);
>>
>> Ugh. That looks horrible even if it might avoid the oops.
>>
>> I think a much better solution is to back-port commit b296821a7c42
>> ("xattr_handler: pass dentry and inode as separate arguments of
>> ->get()") to older kernels. Then the inode is passed down all the way,
>> and you don't have people try to get it from the (not yet initialized)
>> dentry.
>>
>> But there might be other parts missing too, and I didn't look at how
>> easy/painful that backport would be.
>>
>> Al - comments? This is all because of commit 1e2e547a93a0 ("do
>> d_instantiate/unlock_new_inode combinations safely") being marked for
>> stable, and various cases of security_d_instantiate() calling down to
>> getxattr. Which used to not get the inode at all, so those older
>> kernels use d_inode(dentry), which doesn't work in this path since
>> dentry->d_inode hasn't been instantiated yet..
>
> You also want b96809173e94 and ce23e6401334 there...
For above two commits, also b296821a7c42 is required. And after
backport, smack still crashed because setxattr. To fix it, 5930122683df
and 3767e255b390 are also required.
By the way, does no one have met this kind getxattr crash issue with
selinux from 3.18.y?
--
Seung-Woo Kim
Samsung Research
--
Re: Linux 3.18.111
On Mon, Jul 02, 2018 at 10:01:25PM -0700, Linus Torvalds wrote:
> On Mon, Jul 2, 2018 at 9:43 PM Seung-Woo Kim wrote:
> >
> > I think the commit itself is required. Simple, but not reliable,
> > workaround fix is like below:
> >
> > diff --git a/fs/dcache.c b/fs/dcache.c
> > index a34d401..7c751f2 100644
> > --- a/fs/dcache.c
> > +++ b/fs/dcache.c
> > @@ -1879,6 +1879,8 @@ void d_instantiate_new(struct dentry *entry,
> > struct inode *inode)
> > BUG_ON(!hlist_unhashed(&entry->d_u.d_alias));
> > BUG_ON(!inode);
> > lockdep_annotate_inode_mutex_key(inode);
> > + /* WORKAROUND for calling security_d_instantiate() */
> > + entry->d_inode = inode;
> > security_d_instantiate(entry, inode);
> > spin_lock(&inode->i_lock);
> > __d_instantiate(entry, inode);
>
> Ugh. That looks horrible even if it might avoid the oops.
>
> I think a much better solution is to back-port commit b296821a7c42
> ("xattr_handler: pass dentry and inode as separate arguments of
> ->get()") to older kernels. Then the inode is passed down all the way,
> and you don't have people try to get it from the (not yet initialized)
> dentry.
>
> But there might be other parts missing too, and I didn't look at how
> easy/painful that backport would be.
>
> Al - comments? This is all because of commit 1e2e547a93a0 ("do
> d_instantiate/unlock_new_inode combinations safely") being marked for
> stable, and various cases of security_d_instantiate() calling down to
> getxattr. Which used to not get the inode at all, so those older
> kernels use d_inode(dentry), which doesn't work in this path since
> dentry->d_inode hasn't been instantiated yet..
You also want b96809173e94 and ce23e6401334 there...
Re: Linux 3.18.111
On Mon, Jul 2, 2018 at 9:43 PM Seung-Woo Kim wrote:
>
> I think the commit itself is required. Simple, but not reliable,
> workaround fix is like below:
>
> diff --git a/fs/dcache.c b/fs/dcache.c
> index a34d401..7c751f2 100644
> --- a/fs/dcache.c
> +++ b/fs/dcache.c
> @@ -1879,6 +1879,8 @@ void d_instantiate_new(struct dentry *entry,
> struct inode *inode)
> BUG_ON(!hlist_unhashed(&entry->d_u.d_alias));
> BUG_ON(!inode);
> lockdep_annotate_inode_mutex_key(inode);
> + /* WORKAROUND for calling security_d_instantiate() */
> + entry->d_inode = inode;
> security_d_instantiate(entry, inode);
> spin_lock(&inode->i_lock);
> __d_instantiate(entry, inode);
Ugh. That looks horrible even if it might avoid the oops.
I think a much better solution is to back-port commit b296821a7c42
("xattr_handler: pass dentry and inode as separate arguments of
->get()") to older kernels. Then the inode is passed down all the way,
and you don't have people try to get it from the (not yet initialized)
dentry.
But there might be other parts missing too, and I didn't look at how
easy/painful that backport would be.
Al - comments? This is all because of commit 1e2e547a93a0 ("do
d_instantiate/unlock_new_inode combinations safely") being marked for
stable, and various cases of security_d_instantiate() calling down to
getxattr. Which used to not get the inode at all, so those older
kernels use d_inode(dentry), which doesn't work in this path since
dentry->d_inode hasn't been instantiated yet..
Linus
Re: Linux 3.18.111
On 2018년 07월 03일 13:36, Greg KH wrote: > On Tue, Jul 03, 2018 at 12:24:59PM +0900, Seung-Woo Kim wrote: >> Hello, >> >> On 2018년 05월 30일 16:32, Greg KH wrote: >>> I'm announcing the release of the 3.18.111 kernel. >>> >>> All users of the 3.18 kernel series must upgrade. >>> >>> The updated 3.18.y git tree can be found at: >>> git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git >>> linux-3.18.y >>> and can be browsed at the normal kernel.org git web browser: >>> >>> http://git.kernel.org/?p=linux/kernel/git/stable/linux-stable.git;a=summary >>> >>> thanks, >>> >>> greg k-h >>> >>> >> >> >> >>> do d_instantiate/unlock_new_inode combinations safely >> >> Recent my test in 3.18.113 kernel with security smack showed following >> crash during mkdir on ext4 fs. >> >> Unable to handle kernel paging request at virtual address ff98 >> pgd = ffc012411000 >> [ff98] *pgd=, *pud= >> [ cut here ] >> Kernel BUG at ffc0007d9430 [verbose debug info unavailable] >> Internal error: Oops - BUG: 9605 [#1] PREEMPT SMP >> CPU: 0 MPIDR: 8000 PID: 1237 Comm: mkdir Not tainted >> 3.18.113-00083-g1bfc02f-dirty #29-Tizen >> task: ffc02cbc2340 ti: ffc02b7fc000 task.ti: ffc02b7fc000 >> PC is at down_read+0x24/0x54 >> LR is at down_read+0x24/0x54 >> [...] >> Call trace: >> [] down_read+0x24/0x54 >> [] ext4_xattr_get+0x74/0x1f4 >> [] ext4_xattr_security_get+0x28/0x38 >> [] generic_getxattr+0x4c/0x60 >> [] smk_fetch.isra.6+0x8c/0xe0 >> [] smack_d_instantiate+0x194/0x324 >> [] security_d_instantiate+0x24/0x30 >> [] d_instantiate_new+0x34/0x94 >> [] ext4_mkdir+0x284/0x354 >> [] vfs_mkdir+0xc0/0x150 >> [] SyS_mkdirat+0x88/0xb8 >> [] SyS_mkdir+0x18/0x20 >> Code: aa0003f3 b00017c0 912e1000 97e38943 (c85f7e60) >> ---[ end trace b1ad797d63dae9c5 ]--- >> >> It is because d_instantiate_new() added from above commit calls >> security_d_instantiate() before calling __d_instantiate() and >> dentry->d_inode is not yet set and null. In 3.18.113 kernel, >> inode->i_op_getxattr() of ext4 is still generic_getxattr() and it only >> has dentry parameter without inode, so it tries to access dentry->d_inode. >> >> I did not test with selinux, but selinux also calls >> inode->i_op_getxattr() from selinux_d_instantiate(), so maybe there is >> also same issue. > > So should I revert something or do you have a proposed fix for this? I think the commit itself is required. Simple, but not reliable, workaround fix is like below: diff --git a/fs/dcache.c b/fs/dcache.c index a34d401..7c751f2 100644 --- a/fs/dcache.c +++ b/fs/dcache.c @@ -1879,6 +1879,8 @@ void d_instantiate_new(struct dentry *entry, struct inode *inode) BUG_ON(!hlist_unhashed(&entry->d_u.d_alias)); BUG_ON(!inode); lockdep_annotate_inode_mutex_key(inode); + /* WORKAROUND for calling security_d_instantiate() */ + entry->d_inode = inode; security_d_instantiate(entry, inode); spin_lock(&inode->i_lock); __d_instantiate(entry, inode); --- But I am not familiar with dentry/inode locking and there is no lock consideration at all. Thanks, - Seung-Woo Kim > > thanks, > > greg k-h > > > -- Seung-Woo Kim Samsung Research --
Re: Linux 3.18.111
On Tue, Jul 03, 2018 at 12:24:59PM +0900, Seung-Woo Kim wrote: > Hello, > > On 2018년 05월 30일 16:32, Greg KH wrote: > > I'm announcing the release of the 3.18.111 kernel. > > > > All users of the 3.18 kernel series must upgrade. > > > > The updated 3.18.y git tree can be found at: > > git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git > > linux-3.18.y > > and can be browsed at the normal kernel.org git web browser: > > > > http://git.kernel.org/?p=linux/kernel/git/stable/linux-stable.git;a=summary > > > > thanks, > > > > greg k-h > > > > > > > > > do d_instantiate/unlock_new_inode combinations safely > > Recent my test in 3.18.113 kernel with security smack showed following > crash during mkdir on ext4 fs. > > Unable to handle kernel paging request at virtual address ff98 > pgd = ffc012411000 > [ff98] *pgd=, *pud= > [ cut here ] > Kernel BUG at ffc0007d9430 [verbose debug info unavailable] > Internal error: Oops - BUG: 9605 [#1] PREEMPT SMP > CPU: 0 MPIDR: 8000 PID: 1237 Comm: mkdir Not tainted > 3.18.113-00083-g1bfc02f-dirty #29-Tizen > task: ffc02cbc2340 ti: ffc02b7fc000 task.ti: ffc02b7fc000 > PC is at down_read+0x24/0x54 > LR is at down_read+0x24/0x54 > [...] > Call trace: > [] down_read+0x24/0x54 > [] ext4_xattr_get+0x74/0x1f4 > [] ext4_xattr_security_get+0x28/0x38 > [] generic_getxattr+0x4c/0x60 > [] smk_fetch.isra.6+0x8c/0xe0 > [] smack_d_instantiate+0x194/0x324 > [] security_d_instantiate+0x24/0x30 > [] d_instantiate_new+0x34/0x94 > [] ext4_mkdir+0x284/0x354 > [] vfs_mkdir+0xc0/0x150 > [] SyS_mkdirat+0x88/0xb8 > [] SyS_mkdir+0x18/0x20 > Code: aa0003f3 b00017c0 912e1000 97e38943 (c85f7e60) > ---[ end trace b1ad797d63dae9c5 ]--- > > It is because d_instantiate_new() added from above commit calls > security_d_instantiate() before calling __d_instantiate() and > dentry->d_inode is not yet set and null. In 3.18.113 kernel, > inode->i_op_getxattr() of ext4 is still generic_getxattr() and it only > has dentry parameter without inode, so it tries to access dentry->d_inode. > > I did not test with selinux, but selinux also calls > inode->i_op_getxattr() from selinux_d_instantiate(), so maybe there is > also same issue. So should I revert something or do you have a proposed fix for this? thanks, greg k-h
Re: Linux 3.18.111
Hello, On 2018년 05월 30일 16:32, Greg KH wrote: > I'm announcing the release of the 3.18.111 kernel. > > All users of the 3.18 kernel series must upgrade. > > The updated 3.18.y git tree can be found at: > git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git > linux-3.18.y > and can be browsed at the normal kernel.org git web browser: > > http://git.kernel.org/?p=linux/kernel/git/stable/linux-stable.git;a=summary > > thanks, > > greg k-h > > > do d_instantiate/unlock_new_inode combinations safely Recent my test in 3.18.113 kernel with security smack showed following crash during mkdir on ext4 fs. Unable to handle kernel paging request at virtual address ff98 pgd = ffc012411000 [ff98] *pgd=, *pud= [ cut here ] Kernel BUG at ffc0007d9430 [verbose debug info unavailable] Internal error: Oops - BUG: 9605 [#1] PREEMPT SMP CPU: 0 MPIDR: 8000 PID: 1237 Comm: mkdir Not tainted 3.18.113-00083-g1bfc02f-dirty #29-Tizen task: ffc02cbc2340 ti: ffc02b7fc000 task.ti: ffc02b7fc000 PC is at down_read+0x24/0x54 LR is at down_read+0x24/0x54 [...] Call trace: [] down_read+0x24/0x54 [] ext4_xattr_get+0x74/0x1f4 [] ext4_xattr_security_get+0x28/0x38 [] generic_getxattr+0x4c/0x60 [] smk_fetch.isra.6+0x8c/0xe0 [] smack_d_instantiate+0x194/0x324 [] security_d_instantiate+0x24/0x30 [] d_instantiate_new+0x34/0x94 [] ext4_mkdir+0x284/0x354 [] vfs_mkdir+0xc0/0x150 [] SyS_mkdirat+0x88/0xb8 [] SyS_mkdir+0x18/0x20 Code: aa0003f3 b00017c0 912e1000 97e38943 (c85f7e60) ---[ end trace b1ad797d63dae9c5 ]--- It is because d_instantiate_new() added from above commit calls security_d_instantiate() before calling __d_instantiate() and dentry->d_inode is not yet set and null. In 3.18.113 kernel, inode->i_op_getxattr() of ext4 is still generic_getxattr() and it only has dentry parameter without inode, so it tries to access dentry->d_inode. I did not test with selinux, but selinux also calls inode->i_op_getxattr() from selinux_d_instantiate(), so maybe there is also same issue. Best Regards, - Seung-Woo Kim -- Seung-Woo Kim Samsung Research --
