diff mbox series

IMA: Mask O_RDWR if FMODE_READ is set

Message ID 20181126163847.xy3acvltyeb75nb4@merlin (mailing list archive)
State New, archived
Headers show
Series IMA: Mask O_RDWR if FMODE_READ is set | expand

Commit Message

Goldwyn Rodrigues Nov. 26, 2018, 4:38 p.m. UTC 
A file can be opened with open(O_WRONLY | O_RDWR), so a FMORE_READ
will not be set, and overlayfs will consider another copy_up() on the same
file leading to a deadlock on mnt_want_write(). Fix it by masking
O_RDWR while opening the file in read-only mode.


Reported-by: syzbot+ae82084b07d0297e566b@syzkaller.appspotmail.com
Fixes: a408e4a86b36 ("ima: open a new file instance if no read permissions")
Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>

Comments

Mimi Zohar Nov. 27, 2018, 12:05 p.m. UTC | #1 
On Mon, 2018-11-26 at 10:38 -0600, Goldwyn Rodrigues wrote:
> A file can be opened with open(O_WRONLY | O_RDWR), so a FMORE_READ
> will not be set, and overlayfs will consider another copy_up() on the same
> file leading to a deadlock on mnt_want_write(). Fix it by masking
> O_RDWR while opening the file in read-only mode.

Shouldn't the open itself fail if both O_WRONLY and O_RDWR are
specified?

Mimi

> 
> 
> Reported-by: syzbot+ae82084b07d0297e566b@syzkaller.appspotmail.com
> Fixes: a408e4a86b36 ("ima: open a new file instance if no read permissions")
> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
> 
> diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c
> index d9e7728027c6..2efa04e47ff0 100644
> --- a/security/integrity/ima/ima_crypto.c
> +++ b/security/integrity/ima/ima_crypto.c
> @@ -422,7 +422,7 @@ int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash)
>  	/* Open a new file instance in O_RDONLY if we cannot read */
>  	if (!(file->f_mode & FMODE_READ)) {
>  		int flags = file->f_flags & ~(O_WRONLY | O_APPEND |
> -				O_TRUNC | O_CREAT | O_NOCTTY | O_EXCL);
> +				O_RDWR | O_TRUNC | O_CREAT | O_NOCTTY | O_EXCL);
>  		flags |= O_RDONLY;
>  		f = dentry_open(&file->f_path, flags, file->f_cred);
>  		if (IS_ERR(f)) {
>
Amir Goldstein Nov. 27, 2018, 2:06 p.m. UTC | #2 
On Tue, Nov 27, 2018 at 2:05 PM Mimi Zohar <zohar@linux.ibm.com> wrote:
>
> On Mon, 2018-11-26 at 10:38 -0600, Goldwyn Rodrigues wrote:
> > A file can be opened with open(O_WRONLY | O_RDWR), so a FMORE_READ
> > will not be set, and overlayfs will consider another copy_up() on the same
> > file leading to a deadlock on mnt_want_write(). Fix it by masking
> > O_RDWR while opening the file in read-only mode.
>
> Shouldn't the open itself fail if both O_WRONLY and O_RDWR are
> specified?
>

Well, open doesn't fail, so what can you do? prove that no app out there
is using this bizarre flag combination and return -EINVAL?
You can try...

BTW, the freakish thing about O_WRONLY | O_RDWR, is that it actually
translates to !f->f_mode & FMODE_READ && !f->f_mode & FMODE_WRITE:
OPEN_FMODE(1|2) = (3+1) & 3 = 0

It would make more sense if O_WRONLY | O_RDWR would have the
same result as O_RDWR, which is exactly what happens with
ACC_MODE(flags) in build_open_flags().
build_open_flags() would be a good place to check and fix this abnormality.

Thanks,
Amir.
Dmitry Vyukov Nov. 27, 2018, 2:14 p.m. UTC | #3 
On Tue, Nov 27, 2018 at 3:06 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> On Tue, Nov 27, 2018 at 2:05 PM Mimi Zohar <zohar@linux.ibm.com> wrote:
>>
>> On Mon, 2018-11-26 at 10:38 -0600, Goldwyn Rodrigues wrote:
>> > A file can be opened with open(O_WRONLY | O_RDWR), so a FMORE_READ
>> > will not be set, and overlayfs will consider another copy_up() on the same
>> > file leading to a deadlock on mnt_want_write(). Fix it by masking
>> > O_RDWR while opening the file in read-only mode.
>>
>> Shouldn't the open itself fail if both O_WRONLY and O_RDWR are
>> specified?
>>
>
> Well, open doesn't fail, so what can you do? prove that no app out there
> is using this bizarre flag combination and return -EINVAL?
> You can try...
>
> BTW, the freakish thing about O_WRONLY | O_RDWR, is that it actually
> translates to !f->f_mode & FMODE_READ && !f->f_mode & FMODE_WRITE:
> OPEN_FMODE(1|2) = (3+1) & 3 = 0
>
> It would make more sense if O_WRONLY | O_RDWR would have the
> same result as O_RDWR, which is exactly what happens with
> ACC_MODE(flags) in build_open_flags().
> build_open_flags() would be a good place to check and fix this abnormality.

Good idea. Breaking userspace is not an option, but isolating lower
kernel levels from this sounds reasonable.
diff mbox series

Patch

diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c
index d9e7728027c6..2efa04e47ff0 100644
--- a/security/integrity/ima/ima_crypto.c
+++ b/security/integrity/ima/ima_crypto.c
@@ -422,7 +422,7 @@  int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash)
 	/* Open a new file instance in O_RDONLY if we cannot read */
 	if (!(file->f_mode & FMODE_READ)) {
 		int flags = file->f_flags & ~(O_WRONLY | O_APPEND |
-				O_TRUNC | O_CREAT | O_NOCTTY | O_EXCL);
+				O_RDWR | O_TRUNC | O_CREAT | O_NOCTTY | O_EXCL);
 		flags |= O_RDONLY;
 		f = dentry_open(&file->f_path, flags, file->f_cred);
 		if (IS_ERR(f)) {