From: Jann Horn <jannh@google.com> To: David Howells <dhowells@redhat.com> Cc: Al Viro <viro@zeniv.linux.org.uk>, raven@themaw.net, linux-fsdevel <linux-fsdevel@vger.kernel.org>, Linux API <linux-api@vger.kernel.org>, linux-block@vger.kernel.org, keyrings@vger.kernel.org, linux-security-module <linux-security-module@vger.kernel.org>, kernel list <linux-kernel@vger.kernel.org> Subject: Re: [PATCH 4/7] vfs: Add superblock notifications Date: Wed, 29 May 2019 16:16:10 +0200 [thread overview] Message-ID: <CAG48ez0Ugv=cfj-v6DaYma0HgyiBjpykSkCr7mCAcMx13LEncg@mail.gmail.com> (raw) In-Reply-To: <24577.1559134719@warthog.procyon.org.uk> On Wed, May 29, 2019 at 2:58 PM David Howells <dhowells@redhat.com> wrote: > Jann Horn <jannh@google.com> wrote: > > It might make sense to require that the path points to the root inode > > of the superblock? That way you wouldn't be able to do this on a bind > > mount that exposes part of a shared filesystem to a container. > > Why prevent that? It doesn't prevent the container denizen from watching a > bind mount that exposes the root of a shared filesystem into a container. Well, yes, but if you expose the root of the shared filesystem to the container, the container is probably meant to have a higher level of access than if only a bind mount is exposed? But I don't know. > It probably makes sense to permit the LSM to rule on whether a watch may be > emplaced, however. We should have some sort of reasonable policy outside of LSM code though - the kernel should still be secure even if no LSMs are built into it. > > > + } > > > + } > > > + up_write(&s->s_umount); > > > + if (ret < 0) > > > + kfree(watch); > > > + } else if (s->s_watchers) { > > > > This should probably have something like a READ_ONCE() for clarity? > > Note that I think I'll rearrange this to: > > } else { > ret = -EBADSLT; > if (s->s_watchers) { > down_write(&s->s_umount); > ret = remove_watch_from_object(s->s_watchers, wqueue, > s->s_unique_id, false); > up_write(&s->s_umount); > } > } > > I'm not sure READ_ONCE() is necessary, since s_watchers can only be > instantiated once and the watch list then persists until the superblock is > deactivated. Furthermore, by the time deactivate_locked_super() is called, we > can't be calling sb_notify() on it as it's become inaccessible. > > So if we see s->s_watchers as non-NULL, we should not see anything different > inside the lock. In fact, I should be able to rewrite the above to: > > } else { > ret = -EBADSLT; > wlist = s->s_watchers; > if (wlist) { > down_write(&s->s_umount); > ret = remove_watch_from_object(wlist, wqueue, > s->s_unique_id, false); > up_write(&s->s_umount); > } > } I'm extremely twitchy when it comes to code like this because AFAIK gcc at least used to sometimes turn code that read a value from memory and then used it multiple times into something with multiple memory reads, leading to critical security vulnerabilities; see e.g. slide 36 of <https://www.blackhat.com/docs/us-16/materials/us-16-Wilhelm-Xenpwn-Breaking-Paravirtualized-Devices.pdf>. I am not aware of any spec that requires the compiler to only perform one read from the memory location in code like this.
WARNING: multiple messages have this Message-ID (diff)
From: Jann Horn <jannh@google.com> To: David Howells <dhowells@redhat.com> Cc: Al Viro <viro@zeniv.linux.org.uk>, raven@themaw.net, linux-fsdevel <linux-fsdevel@vger.kernel.org>, Linux API <linux-api@vger.kernel.org>, linux-block@vger.kernel.org, keyrings@vger.kernel.org, linux-security-module <linux-security-module@vger.kernel.org>, kernel list <linux-kernel@vger.kernel.org> Subject: Re: [PATCH 4/7] vfs: Add superblock notifications Date: Wed, 29 May 2019 14:16:10 +0000 [thread overview] Message-ID: <CAG48ez0Ugv=cfj-v6DaYma0HgyiBjpykSkCr7mCAcMx13LEncg@mail.gmail.com> (raw) In-Reply-To: <24577.1559134719@warthog.procyon.org.uk> On Wed, May 29, 2019 at 2:58 PM David Howells <dhowells@redhat.com> wrote: > Jann Horn <jannh@google.com> wrote: > > It might make sense to require that the path points to the root inode > > of the superblock? That way you wouldn't be able to do this on a bind > > mount that exposes part of a shared filesystem to a container. > > Why prevent that? It doesn't prevent the container denizen from watching a > bind mount that exposes the root of a shared filesystem into a container. Well, yes, but if you expose the root of the shared filesystem to the container, the container is probably meant to have a higher level of access than if only a bind mount is exposed? But I don't know. > It probably makes sense to permit the LSM to rule on whether a watch may be > emplaced, however. We should have some sort of reasonable policy outside of LSM code though - the kernel should still be secure even if no LSMs are built into it. > > > + } > > > + } > > > + up_write(&s->s_umount); > > > + if (ret < 0) > > > + kfree(watch); > > > + } else if (s->s_watchers) { > > > > This should probably have something like a READ_ONCE() for clarity? > > Note that I think I'll rearrange this to: > > } else { > ret = -EBADSLT; > if (s->s_watchers) { > down_write(&s->s_umount); > ret = remove_watch_from_object(s->s_watchers, wqueue, > s->s_unique_id, false); > up_write(&s->s_umount); > } > } > > I'm not sure READ_ONCE() is necessary, since s_watchers can only be > instantiated once and the watch list then persists until the superblock is > deactivated. Furthermore, by the time deactivate_locked_super() is called, we > can't be calling sb_notify() on it as it's become inaccessible. > > So if we see s->s_watchers as non-NULL, we should not see anything different > inside the lock. In fact, I should be able to rewrite the above to: > > } else { > ret = -EBADSLT; > wlist = s->s_watchers; > if (wlist) { > down_write(&s->s_umount); > ret = remove_watch_from_object(wlist, wqueue, > s->s_unique_id, false); > up_write(&s->s_umount); > } > } I'm extremely twitchy when it comes to code like this because AFAIK gcc at least used to sometimes turn code that read a value from memory and then used it multiple times into something with multiple memory reads, leading to critical security vulnerabilities; see e.g. slide 36 of <https://www.blackhat.com/docs/us-16/materials/us-16-Wilhelm-Xenpwn-Breaking-Paravirtualized-Devices.pdf>. I am not aware of any spec that requires the compiler to only perform one read from the memory location in code like this.
next prev parent reply other threads:[~2019-05-29 14:16 UTC|newest] Thread overview: 131+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-05-28 16:01 [RFC][PATCH 0/7] Mount, FS, Block and Keyrings notifications David Howells 2019-05-28 16:01 ` David Howells 2019-05-28 16:01 ` [PATCH 1/7] General notification queue with user mmap()'able ring buffer David Howells 2019-05-28 16:01 ` David Howells 2019-05-28 16:26 ` Greg KH 2019-05-28 16:26 ` Greg KH 2019-05-28 17:30 ` David Howells 2019-05-28 17:30 ` David Howells 2019-05-28 23:12 ` Greg KH 2019-05-28 23:12 ` Greg KH 2019-05-29 16:06 ` David Howells 2019-05-29 16:06 ` David Howells 2019-05-29 17:46 ` Jann Horn 2019-05-29 17:46 ` Jann Horn 2019-05-29 21:02 ` David Howells 2019-05-29 21:02 ` David Howells 2019-05-31 11:14 ` Peter Zijlstra 2019-05-31 11:14 ` Peter Zijlstra 2019-05-31 12:02 ` David Howells 2019-05-31 12:02 ` David Howells 2019-05-31 13:26 ` Peter Zijlstra 2019-05-31 13:26 ` Peter Zijlstra 2019-05-31 14:20 ` David Howells 2019-05-31 14:20 ` David Howells 2019-05-31 16:44 ` Peter Zijlstra 2019-05-31 16:44 ` Peter Zijlstra 2019-05-31 17:12 ` David Howells 2019-05-31 17:12 ` David Howells 2019-06-17 16:24 ` Peter Zijlstra 2019-06-17 16:24 ` Peter Zijlstra 2019-05-29 23:09 ` Greg KH 2019-05-29 23:09 ` Greg KH 2019-05-29 23:11 ` Greg KH 2019-05-29 23:11 ` Greg KH 2019-05-30 9:50 ` Andrea Parri 2019-05-30 9:50 ` Andrea Parri 2019-05-31 8:35 ` Peter Zijlstra 2019-05-31 8:35 ` Peter Zijlstra 2019-05-31 8:47 ` Peter Zijlstra 2019-05-31 8:47 ` Peter Zijlstra 2019-05-31 12:42 ` David Howells 2019-05-31 12:42 ` David Howells 2019-05-31 14:55 ` David Howells 2019-05-31 14:55 ` David Howells 2019-05-28 19:14 ` Jann Horn 2019-05-28 19:14 ` Jann Horn 2019-05-28 22:28 ` David Howells 2019-05-28 22:28 ` David Howells 2019-05-28 23:16 ` Jann Horn 2019-05-28 23:16 ` Jann Horn 2019-05-28 16:02 ` [PATCH 2/7] keys: Add a notification facility David Howells 2019-05-28 16:02 ` David Howells 2019-05-28 16:02 ` [PATCH 3/7] vfs: Add a mount-notification facility David Howells 2019-05-28 16:02 ` David Howells 2019-05-28 20:06 ` Jann Horn 2019-05-28 20:06 ` Jann Horn 2019-05-28 23:04 ` David Howells 2019-05-28 23:04 ` David Howells 2019-05-28 23:23 ` Jann Horn 2019-05-28 23:23 ` Jann Horn 2019-05-29 11:16 ` David Howells 2019-05-29 11:16 ` David Howells 2019-05-28 23:08 ` David Howells 2019-05-28 23:08 ` David Howells 2019-05-29 10:55 ` David Howells 2019-05-29 10:55 ` David Howells 2019-05-29 11:00 ` David Howells 2019-05-29 11:00 ` David Howells 2019-05-29 15:53 ` Casey Schaufler 2019-05-29 15:53 ` Casey Schaufler 2019-05-29 16:12 ` Jann Horn 2019-05-29 16:12 ` Jann Horn 2019-05-29 17:04 ` Casey Schaufler 2019-05-29 17:04 ` Casey Schaufler 2019-06-03 16:30 ` David Howells 2019-06-03 16:30 ` David Howells 2019-05-29 17:13 ` Andy Lutomirski 2019-05-29 17:13 ` Andy Lutomirski 2019-05-29 17:46 ` Casey Schaufler 2019-05-29 17:46 ` Casey Schaufler 2019-05-29 18:11 ` Jann Horn 2019-05-29 18:11 ` Jann Horn 2019-05-29 19:28 ` Casey Schaufler 2019-05-29 19:28 ` Casey Schaufler 2019-05-29 19:47 ` Jann Horn 2019-05-29 19:47 ` Jann Horn 2019-05-29 20:50 ` Casey Schaufler 2019-05-29 20:50 ` Casey Schaufler 2019-05-29 23:12 ` Andy Lutomirski 2019-05-29 23:12 ` Andy Lutomirski 2019-05-29 23:56 ` Casey Schaufler 2019-05-29 23:56 ` Casey Schaufler 2019-05-28 16:02 ` [PATCH 4/7] vfs: Add superblock notifications David Howells 2019-05-28 16:02 ` David Howells 2019-05-28 20:27 ` Jann Horn 2019-05-28 20:27 ` Jann Horn 2019-05-29 12:58 ` David Howells 2019-05-29 12:58 ` David Howells 2019-05-29 14:16 ` Jann Horn [this message] 2019-05-29 14:16 ` Jann Horn 2019-05-28 16:02 ` [PATCH 5/7] fsinfo: Export superblock notification counter David Howells 2019-05-28 16:02 ` David Howells 2019-05-28 16:02 ` [PATCH 6/7] block: Add block layer notifications David Howells 2019-05-28 16:02 ` David Howells 2019-05-28 20:37 ` Jann Horn 2019-05-28 20:37 ` Jann Horn 2019-05-28 16:02 ` [PATCH 7/7] Add sample notification program David Howells 2019-05-28 16:02 ` David Howells 2019-05-28 23:58 ` [RFC][PATCH 0/7] Mount, FS, Block and Keyrings notifications Greg KH 2019-05-28 23:58 ` Greg KH 2019-05-29 6:33 ` Amir Goldstein 2019-05-29 6:33 ` Amir Goldstein 2019-05-29 6:33 ` Amir Goldstein 2019-05-29 14:25 ` Jan Kara 2019-05-29 14:25 ` Jan Kara 2019-05-29 15:10 ` Greg KH 2019-05-29 15:10 ` Greg KH 2019-05-29 15:53 ` Amir Goldstein 2019-05-29 15:53 ` Amir Goldstein 2019-05-30 11:00 ` Jan Kara 2019-05-30 11:00 ` Jan Kara 2019-06-04 12:33 ` David Howells 2019-06-04 12:33 ` David Howells 2019-05-29 6:45 ` David Howells 2019-05-29 6:45 ` David Howells 2019-05-29 7:40 ` Amir Goldstein 2019-05-29 7:40 ` Amir Goldstein 2019-05-29 9:09 ` David Howells 2019-05-29 9:09 ` David Howells 2019-05-29 15:41 ` Casey Schaufler 2019-05-29 15:41 ` Casey Schaufler
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to='CAG48ez0Ugv=cfj-v6DaYma0HgyiBjpykSkCr7mCAcMx13LEncg@mail.gmail.com' \ --to=jannh@google.com \ --cc=dhowells@redhat.com \ --cc=keyrings@vger.kernel.org \ --cc=linux-api@vger.kernel.org \ --cc=linux-block@vger.kernel.org \ --cc=linux-fsdevel@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-security-module@vger.kernel.org \ --cc=raven@themaw.net \ --cc=viro@zeniv.linux.org.uk \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.