From: Kees Cook <keescook@chromium.org> To: Casey Schaufler <casey@schaufler-ca.com> Cc: LSM <linux-security-module@vger.kernel.org>, James Morris <jmorris@namei.org>, LKLM <linux-kernel@vger.kernel.org>, SE Linux <selinux@tycho.nsa.gov>, John Johansen <john.johansen@canonical.com>, Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>, Paul Moore <paul@paul-moore.com>, Stephen Smalley <sds@tycho.nsa.gov>, "linux-fsdevel@vger.kernel.org" <linux-fsdevel@vger.kernel.org>, Alexey Dobriyan <adobriyan@gmail.com>, "Schaufler, Casey" <casey.schaufler@intel.com> Subject: Re: [PATCH 04/10] LSM: Infrastructure management of the cred security blob Date: Thu, 13 Sep 2018 14:12:50 -0700 [thread overview] Message-ID: <CAGXu5j+apDE-E-FM-55pE7hPQbuKV+x22XK0QrO4qyGnq-6T3A@mail.gmail.com> (raw) In-Reply-To: <5a17beeb-c83c-ce8e-162c-1fbeb74aaae8@schaufler-ca.com> On Thu, Sep 13, 2018 at 12:01 PM, Casey Schaufler <casey@schaufler-ca.com> wrote: > On 9/12/2018 4:53 PM, Kees Cook wrote: >> On Tue, Sep 11, 2018 at 9:41 AM, Casey Schaufler <casey@schaufler-ca.com> wrote: >>> Move management of the cred security blob out of the >>> security modules and into the security infrastructure. >>> Instead of allocating and freeing space the security >>> modules tell the infrastructure how much space they >>> require. >> There's a lot of changes here that I think deserve some longer >> discussion in the changelog. Notably, now we run _two_ cycles of init >> calls? That seems... odd. Notes below... > > The first pass adds up the blob sizes. This has to be done because > some modules allocate blobs during the init phase. I have investigated > alternatives, including blobs that include information about what they > contain, but they're all significantly more complicated. Agreed: I liked the idea of not burdening each LSM with a state machine, but I guess it's not much. Note that "finished" then should be __initdata. >>> - if (selinux_is_enabled() && cred->security) { >>> - if ((unsigned long) cred->security < PAGE_SIZE) >>> - return true; >>> - if ((*(u32 *)cred->security & 0xffffff00) == >>> - (POISON_FREE << 24 | POISON_FREE << 16 | POISON_FREE << 8)) >>> - return true; >> These aren't unreasonable checks -- can we add them back in later? >> (They don't need to be selinux specific, in fact: the LSM could do the >> poison...) > > I had asked the maintainers about the value of these checks, and > they said that they were primarily there for debugging during the > original cred breakout development. I'd have not problem making them > infrastructure managed if there's a strong desire to keep them. Since it was only SELinux doing this, and it was from old debugging, then I concur: drop it. It would be easy to restore (and for all LSMs) in the future. >>> +static inline void set_cred_label(const struct cred *cred, >>> + struct aa_label *label) >>> +{ >>> + struct aa_label **blob = cred->security; >>> + >>> + AA_BUG(!blob); >>> + *blob = label; >>> +} >> This feels like it should be a separate patch? Shouldn't AA not be >> playing with cred->security directly before we get to this "sizing and >> allocation" patch? > > You're correct. This is a harmless patch break-up mistake. The end > result of the set is correct, and the interim state works as intended, > albeit with unnecessary code. I much prefer lots of small easy-to-understand patches than trying to piece together many separate things. Let's please break out each LSM's changes and any refactoring into separate patches. Complexity is harder to review than quantity, IMO. >>> + */ >>> +void lsm_early_cred(struct cred *cred) >>> +{ >>> + int rc; >>> + >>> + if (cred == NULL) >>> + panic("%s: NULL cred.\n", __func__); >> I have been given strongly worded advice to never BUG nor panic. I would: >> >> if (WARN_ON(!cred || !cred->security)) >> return; >> >>> + if (cred->security != NULL) >>> + return; >>> + rc = lsm_cred_alloc(cred, GFP_KERNEL); >>> + if (rc) >>> + panic("%s: Early cred alloc failed.\n", __func__); >> And: >> >> WARN_ON(lsm_cred_alloc(cred, GFP_KERNEL)); > > This duplicates the previous behavior from the SELinux and Smack code. > A WARN_ON will result in an immediate panic when the calling code tries > to dereference the blob. I guess what I'm trying to say is, if you have a patch that does: + .... panic() or + .... BUG() and it has "security" anywhere in the topic, you run an extremely high risk of Linus NAKing the entire series. >>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c >>> index 9d6cdd21acb6..9b49698754a7 100644 >>> --- a/security/selinux/hooks.c >>> +++ b/security/selinux/hooks.c >>> @@ -213,12 +213,9 @@ static void cred_init_security(void) >>> struct cred *cred = (struct cred *) current->real_cred; >>> struct task_security_struct *tsec; >>> >>> - tsec = kzalloc(sizeof(struct task_security_struct), GFP_KERNEL); >>> - if (!tsec) >>> - panic("SELinux: Failed to initialize initial task.\n"); >>> - >>> + lsm_early_cred(cred); >>> + tsec = selinux_cred(cred); >> Perhaps leave the existing panic() as-is? > > Moving the panic into lsm_early_cred() reduces the panic count > and avoids code duplication here and in the Smack equivalent. Agreed: but it means intentionally adding a machine-stop condition which Linus is very opposed to. >> Shouldn't this Tomoyo cred handling get split out? > > Yeah. Again, it's the patch size/count balance. I really do like lots of little patches. SO much easier. The main brain-drain on large patches is that things end up out of order due to filename ordering, etc, so a lot more needs to be kept in your head as you're reading a long patch. Much prefer lots of small patches. -Kees -- Kees Cook Pixel Security
WARNING: multiple messages have this Message-ID (diff)
From: keescook@chromium.org (Kees Cook) To: linux-security-module@vger.kernel.org Subject: [PATCH 04/10] LSM: Infrastructure management of the cred security blob Date: Thu, 13 Sep 2018 14:12:50 -0700 [thread overview] Message-ID: <CAGXu5j+apDE-E-FM-55pE7hPQbuKV+x22XK0QrO4qyGnq-6T3A@mail.gmail.com> (raw) In-Reply-To: <5a17beeb-c83c-ce8e-162c-1fbeb74aaae8@schaufler-ca.com> On Thu, Sep 13, 2018 at 12:01 PM, Casey Schaufler <casey@schaufler-ca.com> wrote: > On 9/12/2018 4:53 PM, Kees Cook wrote: >> On Tue, Sep 11, 2018 at 9:41 AM, Casey Schaufler <casey@schaufler-ca.com> wrote: >>> Move management of the cred security blob out of the >>> security modules and into the security infrastructure. >>> Instead of allocating and freeing space the security >>> modules tell the infrastructure how much space they >>> require. >> There's a lot of changes here that I think deserve some longer >> discussion in the changelog. Notably, now we run _two_ cycles of init >> calls? That seems... odd. Notes below... > > The first pass adds up the blob sizes. This has to be done because > some modules allocate blobs during the init phase. I have investigated > alternatives, including blobs that include information about what they > contain, but they're all significantly more complicated. Agreed: I liked the idea of not burdening each LSM with a state machine, but I guess it's not much. Note that "finished" then should be __initdata. >>> - if (selinux_is_enabled() && cred->security) { >>> - if ((unsigned long) cred->security < PAGE_SIZE) >>> - return true; >>> - if ((*(u32 *)cred->security & 0xffffff00) == >>> - (POISON_FREE << 24 | POISON_FREE << 16 | POISON_FREE << 8)) >>> - return true; >> These aren't unreasonable checks -- can we add them back in later? >> (They don't need to be selinux specific, in fact: the LSM could do the >> poison...) > > I had asked the maintainers about the value of these checks, and > they said that they were primarily there for debugging during the > original cred breakout development. I'd have not problem making them > infrastructure managed if there's a strong desire to keep them. Since it was only SELinux doing this, and it was from old debugging, then I concur: drop it. It would be easy to restore (and for all LSMs) in the future. >>> +static inline void set_cred_label(const struct cred *cred, >>> + struct aa_label *label) >>> +{ >>> + struct aa_label **blob = cred->security; >>> + >>> + AA_BUG(!blob); >>> + *blob = label; >>> +} >> This feels like it should be a separate patch? Shouldn't AA not be >> playing with cred->security directly before we get to this "sizing and >> allocation" patch? > > You're correct. This is a harmless patch break-up mistake. The end > result of the set is correct, and the interim state works as intended, > albeit with unnecessary code. I much prefer lots of small easy-to-understand patches than trying to piece together many separate things. Let's please break out each LSM's changes and any refactoring into separate patches. Complexity is harder to review than quantity, IMO. >>> + */ >>> +void lsm_early_cred(struct cred *cred) >>> +{ >>> + int rc; >>> + >>> + if (cred == NULL) >>> + panic("%s: NULL cred.\n", __func__); >> I have been given strongly worded advice to never BUG nor panic. I would: >> >> if (WARN_ON(!cred || !cred->security)) >> return; >> >>> + if (cred->security != NULL) >>> + return; >>> + rc = lsm_cred_alloc(cred, GFP_KERNEL); >>> + if (rc) >>> + panic("%s: Early cred alloc failed.\n", __func__); >> And: >> >> WARN_ON(lsm_cred_alloc(cred, GFP_KERNEL)); > > This duplicates the previous behavior from the SELinux and Smack code. > A WARN_ON will result in an immediate panic when the calling code tries > to dereference the blob. I guess what I'm trying to say is, if you have a patch that does: + .... panic() or + .... BUG() and it has "security" anywhere in the topic, you run an extremely high risk of Linus NAKing the entire series. >>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c >>> index 9d6cdd21acb6..9b49698754a7 100644 >>> --- a/security/selinux/hooks.c >>> +++ b/security/selinux/hooks.c >>> @@ -213,12 +213,9 @@ static void cred_init_security(void) >>> struct cred *cred = (struct cred *) current->real_cred; >>> struct task_security_struct *tsec; >>> >>> - tsec = kzalloc(sizeof(struct task_security_struct), GFP_KERNEL); >>> - if (!tsec) >>> - panic("SELinux: Failed to initialize initial task.\n"); >>> - >>> + lsm_early_cred(cred); >>> + tsec = selinux_cred(cred); >> Perhaps leave the existing panic() as-is? > > Moving the panic into lsm_early_cred() reduces the panic count > and avoids code duplication here and in the Smack equivalent. Agreed: but it means intentionally adding a machine-stop condition which Linus is very opposed to. >> Shouldn't this Tomoyo cred handling get split out? > > Yeah. Again, it's the patch size/count balance. I really do like lots of little patches. SO much easier. The main brain-drain on large patches is that things end up out of order due to filename ordering, etc, so a lot more needs to be kept in your head as you're reading a long patch. Much prefer lots of small patches. -Kees -- Kees Cook Pixel Security
next prev parent reply other threads:[~2018-09-13 21:12 UTC|newest] Thread overview: 117+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-09-11 16:26 [PATCH v2 00/10] LSM: Module stacking in support of S.A.R.A and Landlock Casey Schaufler 2018-09-11 16:26 ` Casey Schaufler 2018-09-11 16:41 ` [PATCH 01/10] procfs: add smack subdir to attrs Casey Schaufler 2018-09-11 16:41 ` Casey Schaufler 2018-09-11 23:45 ` Ahmed S. Darwish 2018-09-11 23:45 ` Ahmed S. Darwish 2018-09-12 0:01 ` Casey Schaufler 2018-09-12 0:01 ` Casey Schaufler 2018-09-12 22:57 ` Kees Cook 2018-09-12 22:57 ` Kees Cook 2018-09-11 16:41 ` [PATCH 02/10] Smack: Abstract use of cred security blob Casey Schaufler 2018-09-11 16:41 ` Casey Schaufler 2018-09-12 23:04 ` Kees Cook 2018-09-12 23:04 ` Kees Cook 2018-09-11 16:41 ` [PATCH 03/10] SELinux: " Casey Schaufler 2018-09-11 16:41 ` Casey Schaufler 2018-09-12 23:10 ` Kees Cook 2018-09-12 23:10 ` Kees Cook 2018-09-11 16:41 ` [PATCH 04/10] LSM: Infrastructure management of the " Casey Schaufler 2018-09-11 16:41 ` Casey Schaufler 2018-09-12 23:53 ` Kees Cook 2018-09-12 23:53 ` Kees Cook 2018-09-13 19:01 ` Casey Schaufler 2018-09-13 19:01 ` Casey Schaufler 2018-09-13 21:12 ` Kees Cook [this message] 2018-09-13 21:12 ` Kees Cook 2018-09-11 16:41 ` [PATCH 05/10] SELinux: Abstract use of file " Casey Schaufler 2018-09-11 16:41 ` Casey Schaufler 2018-09-12 23:54 ` Kees Cook 2018-09-12 23:54 ` Kees Cook 2018-09-11 16:42 ` [PATCH 06/10] LSM: Infrastructure management of the " Casey Schaufler 2018-09-11 16:42 ` Casey Schaufler 2018-09-13 0:00 ` Kees Cook 2018-09-13 0:00 ` Kees Cook 2018-09-11 16:42 ` [PATCH 07/10] SELinux: Abstract use of inode " Casey Schaufler 2018-09-11 16:42 ` Casey Schaufler 2018-09-13 0:23 ` Kees Cook 2018-09-13 0:23 ` Kees Cook 2018-09-11 16:42 ` [PATCH 08/10] Smack: " Casey Schaufler 2018-09-11 16:42 ` Casey Schaufler 2018-09-13 0:24 ` Kees Cook 2018-09-13 0:24 ` Kees Cook 2018-09-11 16:42 ` [PATCH 09/10] LSM: Infrastructure management of the inode security Casey Schaufler 2018-09-11 16:42 ` Casey Schaufler 2018-09-13 0:30 ` Kees Cook 2018-09-13 0:30 ` Kees Cook 2018-09-11 16:42 ` [PATCH 10/10] LSM: Blob sharing support for S.A.R.A and LandLock Casey Schaufler 2018-09-11 16:42 ` Casey Schaufler 2018-09-13 4:19 ` Kees Cook 2018-09-13 4:19 ` Kees Cook 2018-09-13 13:16 ` Paul Moore 2018-09-13 13:16 ` Paul Moore 2018-09-13 15:19 ` Kees Cook 2018-09-13 15:19 ` Kees Cook 2018-09-13 19:12 ` Paul Moore 2018-09-13 19:12 ` Paul Moore 2018-09-13 20:58 ` Jordan Glover 2018-09-13 20:58 ` Jordan Glover 2018-09-13 20:58 ` Jordan Glover 2018-09-13 21:50 ` Paul Moore 2018-09-13 21:50 ` Paul Moore 2018-09-13 22:04 ` Jordan Glover 2018-09-13 22:04 ` Jordan Glover 2018-09-13 22:04 ` Jordan Glover 2018-09-13 23:01 ` Casey Schaufler 2018-09-13 23:01 ` Casey Schaufler 2018-09-13 21:01 ` Kees Cook 2018-09-13 21:01 ` Kees Cook 2018-09-13 21:38 ` Paul Moore 2018-09-13 21:38 ` Paul Moore 2018-09-13 21:51 ` Kees Cook 2018-09-13 21:51 ` Kees Cook 2018-09-13 23:06 ` Kees Cook 2018-09-13 23:06 ` Kees Cook 2018-09-13 23:32 ` John Johansen 2018-09-13 23:32 ` John Johansen 2018-09-13 23:51 ` Kees Cook 2018-09-13 23:51 ` Kees Cook 2018-09-14 0:03 ` Casey Schaufler 2018-09-14 0:03 ` Casey Schaufler 2018-09-14 0:06 ` Kees Cook 2018-09-14 0:06 ` Kees Cook 2018-09-13 23:51 ` Casey Schaufler 2018-09-13 23:51 ` Casey Schaufler 2018-09-13 23:57 ` Kees Cook 2018-09-13 23:57 ` Kees Cook 2018-09-14 0:08 ` Casey Schaufler 2018-09-14 0:08 ` Casey Schaufler 2018-09-14 0:19 ` Kees Cook 2018-09-14 0:19 ` Kees Cook 2018-09-14 15:57 ` Casey Schaufler 2018-09-14 15:57 ` Casey Schaufler 2018-09-14 20:05 ` Kees Cook 2018-09-14 20:05 ` Kees Cook 2018-09-14 20:47 ` Casey Schaufler 2018-09-14 20:47 ` Casey Schaufler 2018-09-14 18:18 ` James Morris 2018-09-14 18:18 ` James Morris 2018-09-14 18:23 ` John Johansen 2018-09-14 18:23 ` John Johansen 2018-09-14 0:03 ` Kees Cook 2018-09-14 0:03 ` Kees Cook 2018-09-14 2:42 ` Paul Moore 2018-09-14 2:42 ` Paul Moore 2018-09-11 20:43 ` [PATCH v2 00/10] LSM: Module stacking in support of S.A.R.A and Landlock James Morris 2018-09-11 20:43 ` James Morris 2018-09-12 21:29 ` James Morris 2018-09-12 21:29 ` James Morris 2018-09-16 16:54 ` Salvatore Mesoraca 2018-09-16 16:54 ` Salvatore Mesoraca 2018-09-16 17:25 ` Casey Schaufler 2018-09-16 17:25 ` Casey Schaufler 2018-09-16 17:45 ` Salvatore Mesoraca 2018-09-16 17:45 ` Salvatore Mesoraca 2018-09-18 7:44 ` Mickaël Salaün 2018-09-18 15:23 ` Casey Schaufler 2018-09-18 15:23 ` 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=CAGXu5j+apDE-E-FM-55pE7hPQbuKV+x22XK0QrO4qyGnq-6T3A@mail.gmail.com \ --to=keescook@chromium.org \ --cc=adobriyan@gmail.com \ --cc=casey.schaufler@intel.com \ --cc=casey@schaufler-ca.com \ --cc=jmorris@namei.org \ --cc=john.johansen@canonical.com \ --cc=linux-fsdevel@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-security-module@vger.kernel.org \ --cc=paul@paul-moore.com \ --cc=penguin-kernel@i-love.sakura.ne.jp \ --cc=sds@tycho.nsa.gov \ --cc=selinux@tycho.nsa.gov \ /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.