inode.i_flags might be altered without proper
synchronisation when the inode belongs to devtmpfs.
blkdev_write_iter() starts writing via __generic_file_write_iter()
which sets S_NOSEC bit without any synchronisation.
The following stacktrace shows how to get there:
13: entry_SYSENTER_32:460
12: do_fast_syscall_32:410
11: _static_cpu_has:146
10: do_syscall_32_irqs_on:322
09: SyS_pwrite64:636
08: SYSC_pwrite64:650
07: fdput:38
06: vfs_write:560
05: __vfs_write:512
04: new_sync_write:500
03: blkdev_write_iter:1977
02: __generic_file_write_iter:2897
01: file_remove_privs:1818
00: inode_has_no_xattr:3163
If S_NOSEC is *not* set, i_rwsem is acquired around
__generic_file_write_iter().
Found by LockDoc (Alexander Lochmann, Horst Schirmeier and Olaf
Spinczyk)
Signed-off-by: Alexander Lochmann <[email protected]>
Signed-off-by: Horst Schirmeier <[email protected]>
---
fs/block_dev.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/fs/block_dev.c b/fs/block_dev.c
index a80b4f0ee7c4..b4ece62e3c05 100644
--- a/fs/block_dev.c
+++ b/fs/block_dev.c
@@ -1894,8 +1894,10 @@ ssize_t blkdev_write_iter(struct kiocb *iocb,
struct iov_iter *from)
{
struct file *file = iocb->ki_filp;
struct inode *bd_inode = bdev_file_inode(file);
+ struct inode *inode = file_inode(file);
loff_t size = i_size_read(bd_inode);
struct blk_plug plug;
+ int locked = 0;
ssize_t ret;
if (bdev_read_only(I_BDEV(bd_inode)))
@@ -1913,9 +1915,19 @@ ssize_t blkdev_write_iter(struct kiocb *iocb,
struct iov_iter *from)
iov_iter_truncate(from, size - iocb->ki_pos);
blk_start_plug(&plug);
+ /*
+ * Ensure excl. access to i_flags in __generic_file_write_iter().
+ * Otherwise, it would race with chmod adding SUID bit.
+ */
+ if (!IS_NOSEC(inode)) {
+ inode_lock(inode);
+ locked = 1;
+ }
ret = __generic_file_write_iter(iocb, from);
if (ret > 0)
ret = generic_write_sync(iocb, ret);
+ if (locked)
+ inode_unlock(inode);
blk_finish_plug(&plug);
return ret;
}
--
2.19.2
On Fri, Dec 07, 2018 at 05:10:15PM +0100, Alexander Lochmann wrote:
>
> inode.i_flags might be altered without proper
> synchronisation when the inode belongs to devtmpfs.
> blkdev_write_iter() starts writing via __generic_file_write_iter()
> which sets S_NOSEC bit without any synchronisation.
> The following stacktrace shows how to get there:
> 13: entry_SYSENTER_32:460
> 12: do_fast_syscall_32:410
> 11: _static_cpu_has:146
> 10: do_syscall_32_irqs_on:322
> 09: SyS_pwrite64:636
> 08: SYSC_pwrite64:650
> 07: fdput:38
> 06: vfs_write:560
> 05: __vfs_write:512
> 04: new_sync_write:500
> 03: blkdev_write_iter:1977
> 02: __generic_file_write_iter:2897
> 01: file_remove_privs:1818
> 00: inode_has_no_xattr:3163
> If S_NOSEC is *not* set, i_rwsem is acquired around
> __generic_file_write_iter().
> + * Ensure excl. access to i_flags in __generic_file_write_iter().
> + * Otherwise, it would race with chmod adding SUID bit.
> + */
_What_ SUID bit? We are talking about a write to block device, for fsck sake...
On 07.12.18 18:58, Al Viro wrote:
> On Fri, Dec 07, 2018 at 05:10:15PM +0100, Alexander Lochmann wrote:
>>
>> inode.i_flags might be altered without proper
>> synchronisation when the inode belongs to devtmpfs.
>> blkdev_write_iter() starts writing via __generic_file_write_iter()
>> which sets S_NOSEC bit without any synchronisation.
>> The following stacktrace shows how to get there:
>> 13: entry_SYSENTER_32:460
>> 12: do_fast_syscall_32:410
>> 11: _static_cpu_has:146
>> 10: do_syscall_32_irqs_on:322
>> 09: SyS_pwrite64:636
>> 08: SYSC_pwrite64:650
>> 07: fdput:38
>> 06: vfs_write:560
>> 05: __vfs_write:512
>> 04: new_sync_write:500
>> 03: blkdev_write_iter:1977
>> 02: __generic_file_write_iter:2897
>> 01: file_remove_privs:1818
>> 00: inode_has_no_xattr:3163
>> If S_NOSEC is *not* set, i_rwsem is acquired around
>> __generic_file_write_iter().
>
>> + * Ensure excl. access to i_flags in __generic_file_write_iter().
>> + * Otherwise, it would race with chmod adding SUID bit.
>> + */
>
> _What_ SUID bit? We are talking about a write to block device, for fsck sake...
>
That's the way I understood Jan's explanation:
"
Thinking more about this I'm not sure if this is actually the right
solution. Because for example the write(2) can set S_NOSEC flag wrongly
when it would race with chmod adding SUID bit. So probably we rather need
to acquire i_rwsem in blkdev_write_iter() if file does not have S_NOSEC set
(we don't want to acquire it unconditionally as that would heavily impact
scalability of block device writes).
Honza"
If I'm mistaking, pls help me with fixing it.
- Alex
--
Technische Universität Dortmund
Alexander Lochmann PGP key: 0xBC3EF6FD
Otto-Hahn-Str. 16 phone: +49.231.7556141
D-44227 Dortmund fax: +49.231.7556116
http://ess.cs.tu-dortmund.de/Staff/al
On Fri, Dec 07, 2018 at 08:49:16PM +0100, Alexander Lochmann wrote:
> > _What_ SUID bit? We are talking about a write to block device, for fsck sake...
> >
> That's the way I understood Jan's explanation:
> "
> Thinking more about this I'm not sure if this is actually the right
> solution. Because for example the write(2) can set S_NOSEC flag wrongly
> when it would race with chmod adding SUID bit. So probably we rather need
> to acquire i_rwsem in blkdev_write_iter() if file does not have S_NOSEC set
> (we don't want to acquire it unconditionally as that would heavily impact
> scalability of block device writes).
IDGI. We are talking about a block device here. What business could
file_remove_privs() have doing _anything_ to it? should_remove_suid() returns
to return 0 for those; what case do you have in mind? Somebody setting
security.capabilities on a block device inode?
IMO the bug here is file_remove_privs() not buggering off immediately
after having observed that we are dealing with a block device. It really
has nothing useful to do.
On Sat 08-12-18 00:49:44, Al Viro wrote:
> On Fri, Dec 07, 2018 at 08:49:16PM +0100, Alexander Lochmann wrote:
>
> > > _What_ SUID bit? We are talking about a write to block device, for fsck sake...
> > >
> > That's the way I understood Jan's explanation:
> > "
> > Thinking more about this I'm not sure if this is actually the right
> > solution. Because for example the write(2) can set S_NOSEC flag wrongly
> > when it would race with chmod adding SUID bit. So probably we rather need
> > to acquire i_rwsem in blkdev_write_iter() if file does not have S_NOSEC set
> > (we don't want to acquire it unconditionally as that would heavily impact
> > scalability of block device writes).
>
> IDGI. We are talking about a block device here. What business could
> file_remove_privs() have doing _anything_ to it? should_remove_suid() returns
> to return 0 for those; what case do you have in mind? Somebody setting
> security.capabilities on a block device inode?
>
> IMO the bug here is file_remove_privs() not buggering off immediately
> after having observed that we are dealing with a block device. It really
> has nothing useful to do.
I didn't notice that S_ISREG() check in should_remove_suid(). My bad. And I
wasn't quite sure whether some security module does not rely on
inode_need_killpriv security hook. But now when I grep I see that
cap_inode_need_killpriv() is really the only user and security.capabilities
probably don't make sense for it since block devices cannot be executed
anyway.
So yes, the easiest fix is to just bail from file_remove_privs(). Probably
for anything that is not a regular file, right? Directories cannot be
written anyway and for pipes and character devices same logic applies as
for block devices.
Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR
file_remove_privs() might be called for non-regular files, e.g.
blkdev inode. There is no reason to do its job on things
like blkdev inodes, pipes, or cdevs. Hence, abort if
file does not refer to a regular inode.
The following stacktrace shows how to get there:
13: entry_SYSENTER_32:460
12: do_fast_syscall_32:410
11: _static_cpu_has:146
10: do_syscall_32_irqs_on:322
09: SyS_pwrite64:636
08: SYSC_pwrite64:650
07: fdput:38
06: vfs_write:560
05: __vfs_write:512
04: new_sync_write:500
03: blkdev_write_iter:1977
02: __generic_file_write_iter:2897
01: file_remove_privs:1818
00: inode_has_no_xattr:3163
Found by LockDoc (Alexander Lochmann, Horst Schirmeier and Olaf
Spinczyk)
Signed-off-by: Alexander Lochmann <[email protected]>
Signed-off-by: Horst Schirmeier <[email protected]>
---
fs/inode.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/fs/inode.c b/fs/inode.c
index 35d2108d567c..682088190413 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -1820,8 +1820,13 @@ int file_remove_privs(struct file *file)
int kill;
int error = 0;
- /* Fast path for nothing security related */
- if (IS_NOSEC(inode))
+ /*
+ * Fast path for nothing security related.
+ * As well for non-regular files, e.g. blkdev inodes.
+ * For example, blkdev_write_iter() might get here
+ * trying to remove privs which it is not allowed to.
+ */
+ if (IS_NOSEC(inode) || !S_ISREG(inode->i_mode))
return 0;
kill = dentry_needs_remove_privs(dentry);
--
2.19.2
On Fri 14-12-18 11:55:52, Alexander Lochmann wrote:
>
> file_remove_privs() might be called for non-regular files, e.g.
> blkdev inode. There is no reason to do its job on things
> like blkdev inodes, pipes, or cdevs. Hence, abort if
> file does not refer to a regular inode.
> The following stacktrace shows how to get there:
> 13: entry_SYSENTER_32:460
> 12: do_fast_syscall_32:410
> 11: _static_cpu_has:146
> 10: do_syscall_32_irqs_on:322
> 09: SyS_pwrite64:636
> 08: SYSC_pwrite64:650
> 07: fdput:38
> 06: vfs_write:560
> 05: __vfs_write:512
> 04: new_sync_write:500
> 03: blkdev_write_iter:1977
> 02: __generic_file_write_iter:2897
> 01: file_remove_privs:1818
> 00: inode_has_no_xattr:3163
>
> Found by LockDoc (Alexander Lochmann, Horst Schirmeier and Olaf
> Spinczyk)
>
> Signed-off-by: Alexander Lochmann <[email protected]>
> Signed-off-by: Horst Schirmeier <[email protected]>
The patch looks good to me. You can add:
Reviewed-by: Jan Kara <[email protected]>
Honza
> ---
> fs/inode.c | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/fs/inode.c b/fs/inode.c
> index 35d2108d567c..682088190413 100644
> --- a/fs/inode.c
> +++ b/fs/inode.c
> @@ -1820,8 +1820,13 @@ int file_remove_privs(struct file *file)
> int kill;
> int error = 0;
>
> - /* Fast path for nothing security related */
> - if (IS_NOSEC(inode))
> + /*
> + * Fast path for nothing security related.
> + * As well for non-regular files, e.g. blkdev inodes.
> + * For example, blkdev_write_iter() might get here
> + * trying to remove privs which it is not allowed to.
> + */
> + if (IS_NOSEC(inode) || !S_ISREG(inode->i_mode))
> return 0;
>
> kill = dentry_needs_remove_privs(dentry);
> --
> 2.19.2
>
--
Jan Kara <[email protected]>
SUSE Labs, CR
Hello Al,
Have you had the opportunity to review our patch?
Cheers,
Alex
On 17.12.18 09:28, Jan Kara wrote:
> On Fri 14-12-18 11:55:52, Alexander Lochmann wrote:
>>
>> file_remove_privs() might be called for non-regular files, e.g.
>> blkdev inode. There is no reason to do its job on things
>> like blkdev inodes, pipes, or cdevs. Hence, abort if
>> file does not refer to a regular inode.
>> The following stacktrace shows how to get there:
>> 13: entry_SYSENTER_32:460
>> 12: do_fast_syscall_32:410
>> 11: _static_cpu_has:146
>> 10: do_syscall_32_irqs_on:322
>> 09: SyS_pwrite64:636
>> 08: SYSC_pwrite64:650
>> 07: fdput:38
>> 06: vfs_write:560
>> 05: __vfs_write:512
>> 04: new_sync_write:500
>> 03: blkdev_write_iter:1977
>> 02: __generic_file_write_iter:2897
>> 01: file_remove_privs:1818
>> 00: inode_has_no_xattr:3163
>>
>> Found by LockDoc (Alexander Lochmann, Horst Schirmeier and Olaf
>> Spinczyk)
>>
>> Signed-off-by: Alexander Lochmann <[email protected]>
>> Signed-off-by: Horst Schirmeier <[email protected]>
>
> The patch looks good to me. You can add:
>
> Reviewed-by: Jan Kara <[email protected]>
>
> Honza
>
>> ---
>> fs/inode.c | 9 +++++++--
>> 1 file changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/inode.c b/fs/inode.c
>> index 35d2108d567c..682088190413 100644
>> --- a/fs/inode.c
>> +++ b/fs/inode.c
>> @@ -1820,8 +1820,13 @@ int file_remove_privs(struct file *file)
>> int kill;
>> int error = 0;
>>
>> - /* Fast path for nothing security related */
>> - if (IS_NOSEC(inode))
>> + /*
>> + * Fast path for nothing security related.
>> + * As well for non-regular files, e.g. blkdev inodes.
>> + * For example, blkdev_write_iter() might get here
>> + * trying to remove privs which it is not allowed to.
>> + */
>> + if (IS_NOSEC(inode) || !S_ISREG(inode->i_mode))
>> return 0;
>>
>> kill = dentry_needs_remove_privs(dentry);
>> --
>> 2.19.2
>>
>
>
>
--
Technische Universität Dortmund
Alexander Lochmann PGP key: 0xBC3EF6FD
Otto-Hahn-Str. 16 phone: +49.231.7556141
D-44227 Dortmund fax: +49.231.7556116
http://ess.cs.tu-dortmund.de/Staff/al
Hello Al,
Meanwhile, have you had the opportunity to review our patch?
Regards,
Alex
On 11.01.19 16:42, Alexander Lochmann wrote:
> Hello Al,
>
> Have you had the opportunity to review our patch?
>
> Cheers,
> Alex
>
> On 17.12.18 09:28, Jan Kara wrote:
>> On Fri 14-12-18 11:55:52, Alexander Lochmann wrote:
>>>
>>> file_remove_privs() might be called for non-regular files, e.g.
>>> blkdev inode. There is no reason to do its job on things
>>> like blkdev inodes, pipes, or cdevs. Hence, abort if
>>> file does not refer to a regular inode.
>>> The following stacktrace shows how to get there:
>>> 13: entry_SYSENTER_32:460
>>> 12: do_fast_syscall_32:410
>>> 11: _static_cpu_has:146
>>> 10: do_syscall_32_irqs_on:322
>>> 09: SyS_pwrite64:636
>>> 08: SYSC_pwrite64:650
>>> 07: fdput:38
>>> 06: vfs_write:560
>>> 05: __vfs_write:512
>>> 04: new_sync_write:500
>>> 03: blkdev_write_iter:1977
>>> 02: __generic_file_write_iter:2897
>>> 01: file_remove_privs:1818
>>> 00: inode_has_no_xattr:3163
>>>
>>> Found by LockDoc (Alexander Lochmann, Horst Schirmeier and Olaf
>>> Spinczyk)
>>>
>>> Signed-off-by: Alexander Lochmann <[email protected]>
>>> Signed-off-by: Horst Schirmeier <[email protected]>
>>
>> The patch looks good to me. You can add:
>>
>> Reviewed-by: Jan Kara <[email protected]>
>>
>> Honza
>>
>>> ---
>>> fs/inode.c | 9 +++++++--
>>> 1 file changed, 7 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/fs/inode.c b/fs/inode.c
>>> index 35d2108d567c..682088190413 100644
>>> --- a/fs/inode.c
>>> +++ b/fs/inode.c
>>> @@ -1820,8 +1820,13 @@ int file_remove_privs(struct file *file)
>>> int kill;
>>> int error = 0;
>>>
>>> - /* Fast path for nothing security related */
>>> - if (IS_NOSEC(inode))
>>> + /*
>>> + * Fast path for nothing security related.
>>> + * As well for non-regular files, e.g. blkdev inodes.
>>> + * For example, blkdev_write_iter() might get here
>>> + * trying to remove privs which it is not allowed to.
>>> + */
>>> + if (IS_NOSEC(inode) || !S_ISREG(inode->i_mode))
>>> return 0;
>>>
>>> kill = dentry_needs_remove_privs(dentry);
>>> --
>>> 2.19.2
>>>
>>
>>
>>
>
--
Technische Universität Dortmund
Alexander Lochmann PGP key: 0xBC3EF6FD
Otto-Hahn-Str. 16 phone: +49.231.7556141
D-44227 Dortmund fax: +49.231.7556116
http://ess.cs.tu-dortmund.de/Staff/al