[PATCH] Fix sync. in blkdev_write_iter() acessing i_flags

[PATCH] Fix sync. in blkdev_write_iter() acessing i_flags

[Alexander Lochmann]

[-- Attachment #1.1: Type: text/plain, Size: 2099 bytes --]


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 <alexander.lochmann@tu-dortmund.de>
Signed-off-by: Horst Schirmeier <horst.schirmeier@tu-dortmund.de>
---
 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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] Fix sync. in blkdev_write_iter() acessing i_flags

[Al Viro]

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...

Re: [PATCH] Fix sync. in blkdev_write_iter() acessing i_flags

[Alexander Lochmann]

[-- Attachment #1.1: Type: text/plain, Size: 1916 bytes --]



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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] Fix sync. in blkdev_write_iter() acessing i_flags

[Al Viro]

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.

Re: [PATCH] Fix sync. in blkdev_write_iter() acessing i_flags

[Jan Kara]

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 <jack@suse.com>
SUSE Labs, CR

[PATCH] Abort file_remove_privs() for non-reg. files

[Alexander Lochmann]

[-- Attachment #1.1: Type: text/plain, Size: 1566 bytes --]


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 <alexander.lochmann@tu-dortmund.de>
Signed-off-by: Horst Schirmeier <horst.schirmeier@tu-dortmund.de>
---
 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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] Abort file_remove_privs() for non-reg. files

[Jan Kara]

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 <alexander.lochmann@tu-dortmund.de>
> Signed-off-by: Horst Schirmeier <horst.schirmeier@tu-dortmund.de>

The patch looks good to me. You can add:

Reviewed-by: Jan Kara <jack@suse.cz>

								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 <jack@suse.com>
SUSE Labs, CR

Re: [PATCH] Abort file_remove_privs() for non-reg. files

[Alexander Lochmann]

[-- Attachment #1.1: Type: text/plain, Size: 2276 bytes --]

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 <alexander.lochmann@tu-dortmund.de>
>> Signed-off-by: Horst Schirmeier <horst.schirmeier@tu-dortmund.de>
> 
> The patch looks good to me. You can add:
> 
> Reviewed-by: Jan Kara <jack@suse.cz>
> 
> 								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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] Abort file_remove_privs() for non-reg. files

[Alexander Lochmann]

[-- Attachment #1.1: Type: text/plain, Size: 2494 bytes --]

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 <alexander.lochmann@tu-dortmund.de>
>>> Signed-off-by: Horst Schirmeier <horst.schirmeier@tu-dortmund.de>
>>
>> The patch looks good to me. You can add:
>>
>> Reviewed-by: Jan Kara <jack@suse.cz>
>>
>> 								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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]