* Re: [PATCH v4 00/14] ima: introduce IMA Digest Lists extension
From: Roberto Sassu @ 2019-06-25 12:57 UTC (permalink / raw)
To: zohar, dmitry.kasatkin, mjg59
Cc: linux-integrity, linux-security-module, linux-fsdevel, linux-doc,
linux-kernel, silviu.vlasceanu
In-Reply-To: <9029dd14-1077-ec89-ddc2-e677e16ad314@huawei.com>
On 6/17/2019 8:56 AM, Roberto Sassu wrote:
> On 6/14/2019 7:54 PM, Roberto Sassu wrote:
>> This patch set introduces a new IMA extension called IMA Digest Lists.
>>
>> At early boot, the extension preloads in kernel memory reference digest
>> values, that can be compared with actual file digests when files are
>> accessed in the system.
>>
>> The extension will open for new possibilities: PCR with predictable
>> value,
>> that can be used for sealing policies associated to data or TPM keys;
>> appraisal based on reference digests already provided by Linux
>> distribution
>> vendors in the software packages.
>>
>> The first objective can be achieved because the PCR values does not
>> depend
>> on which and when files are measured: the extension measures digest lists
>> sequentially and files whose digest is not in the digest list.
>>
>> The second objective can be reached because the extension is able to
>> extract reference measurements from packages (with a user space tool) and
>> use it as a source for appraisal verification as the reference came from
>> the security.ima xattr. This approach will also reduce the overhead as
>> only
>> one signature is verified for many files (as opposed to one signature for
>> each file with the current implementation).
>>
>> This version of the patch set provides a clear separation between current
>> and new functionality. First, the new functionality must be explicitly
>> enabled from the kernel command line. Second, results of operations
>> performed by the extension can be distinguished from those obtained from
>> the existing code: measurement entries created by the extension have a
>> different PCR; mutable files appraised with the extension have a
>> different
>> security.ima type.
>>
>> The review of this patch set should start from patch 11 and 12, which
>> modify the IMA-Measure and IMA-Appraise submodules to use digest lists.
>> Patch 1 to 5 are prerequisites. Patch 6 to 10 adds support for digest
>> lists. Finally, patch 13 introduces two new policies to measure/appraise
>> rootfs and patch 14 adds the documentation (including a flow chart to
>> show how IMA has been modified).
>>
>> The user space tools to configure digest lists are available at:
>>
>> https://github.com/euleros/digest-list-tools/releases/tag/v0.3
>>
>> The patch set applies on top of linux-integrity/next-queued-testing
>> (73589972b987).
>>
>> It is necessary to apply also:
>> https://patchwork.kernel.org/cover/10957495/
>
> Another dependency is:
>
> https://patchwork.kernel.org/cover/10979341/
>
> Roberto
I uploaded this patch set and all the required dependencies to:
https://github.com/euleros/linux/releases/tag/ima-digest-lists-v4
It should be easy to test. Let me know if you have questions about the
installation.
Mimi, do you have any thoughts on this version?
Thanks
Roberto
--
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Bo PENG, Jian LI, Yanli SHI
^ permalink raw reply
* Re: possible deadlock in console_trylock_spinning
From: syzbot @ 2019-06-25 8:55 UTC (permalink / raw)
To: gregkh, jamorris, jmorris, jslaby, linux-kernel,
linux-security-module, penguin-kernel, penguin-kernel, serge,
syzkaller-bugs, takedakn
In-Reply-To: <0000000000009ad686058bc12956@google.com>
syzbot has bisected this bug to:
commit e80b18599a39a625bc8b2e39ba3004a62f78805a
Author: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Date: Fri Apr 12 11:04:54 2019 +0000
tomoyo: Add a kernel config option for fuzzing testing.
bisection log: https://syzkaller.appspot.com/x/bisect.txt?x=156e43cea00000
start commit: abf02e29 Merge tag 'pm-5.2-rc6' of git://git.kernel.org/pu..
git tree: upstream
final crash: https://syzkaller.appspot.com/x/report.txt?x=176e43cea00000
console output: https://syzkaller.appspot.com/x/log.txt?x=136e43cea00000
kernel config: https://syzkaller.appspot.com/x/.config?x=28ec3437a5394ee0
dashboard link: https://syzkaller.appspot.com/bug?extid=fc1da0f1a577d15b64fc
syz repro: https://syzkaller.appspot.com/x/repro.syz?x=1357add6a00000
C reproducer: https://syzkaller.appspot.com/x/repro.c?x=1611ac89a00000
Reported-by: syzbot+fc1da0f1a577d15b64fc@syzkaller.appspotmail.com
Fixes: e80b18599a39 ("tomoyo: Add a kernel config option for fuzzing
testing.")
For information about bisection process see: https://goo.gl/tpsmEJ#bisection
^ permalink raw reply
* Re: [PATCH V34 00/29] Lockdown as an LSM
From: John Johansen @ 2019-06-25 8:16 UTC (permalink / raw)
To: James Morris, Matthew Garrett
Cc: linux-security-module, linux-kernel, linux-api, Stephen Smalley,
Andy Lutomirski, Casey Schaufler
In-Reply-To: <alpine.LRH.2.21.1906250853290.7826@namei.org>
On 6/24/19 4:01 PM, James Morris wrote:
> On Fri, 21 Jun 2019, Matthew Garrett wrote:
>
>> Minor updates over V33 - security_is_locked_down renamed to
>> security_locked_down, return value of security_locked_down is returned
>> in most cases, one unnecessary patch was dropped, couple of minor nits
>> fixed.
>
> Thanks for the respin.
>
> We are still not resolved on granularity. Stephen has said he's not sure
> if a useful policy can be constructed with just confidentiality and
> integrity settings. I'd be interested to know JJ and Casey's thoughts on
> lockdown policy flexibility wrt their respective LSMs.
>
> These are also "all or nothing" choices which may prevent deployment due
> to a user needing to allow (presumably controlled or mitigated) exceptions
> to the policy.
>
>
I haven't gotten a chance to play with this the way I want to so there is
still a lot of questions regarding its interaction with apparmor and its
policy, but from what I have seen so far it is looking good.
I expect the all or nothing choices may limit its deployments (we really
need to play with this more to say) but we already face similar issues.
There are options we provide at a distro level that we can't turn on by
default, but we do recommend to more security conscious users.
If lockdown was in kernel we would certainly make it available for our
users, we have even had a few people ask about it.
^ permalink raw reply
* Re: [PATCH V34 00/29] Lockdown as an LSM
From: James Morris @ 2019-06-25 6:04 UTC (permalink / raw)
To: Matthew Garrett
Cc: LSM List, Linux Kernel Mailing List, Linux API, Stephen Smalley,
Andy Lutomirski, John Johansen, Casey Schaufler
In-Reply-To: <CACdnJut2erF9ZKeJQ+uvWZeEnHh=stmEioi_P36DF9mN5i2RGQ@mail.gmail.com>
On Mon, 24 Jun 2019, Matthew Garrett wrote:
> > We are still not resolved on granularity. Stephen has said he's not sure
> > if a useful policy can be constructed with just confidentiality and
> > integrity settings. I'd be interested to know JJ and Casey's thoughts on
> > lockdown policy flexibility wrt their respective LSMs.
>
> This implementation provides arbitrary granularity at the LSM level,
> though the lockdown LSM itself only provides two levels. Other LSMs
> can choose an appropriate level of exposure.
Ahh, OK, I only looked at the patchset description and had not looked at
V33 yet.
This is looking good.
--
James Morris
<jmorris@namei.org>
^ permalink raw reply
* Re: [PATCH V31 07/25] kexec_file: Restrict at runtime if the kernel is locked down
From: Dave Young @ 2019-06-25 2:51 UTC (permalink / raw)
To: Matthew Garrett
Cc: James Morris, Jiri Bohac, Linux API, kexec,
Linux Kernel Mailing List, David Howells, LSM List,
Andy Lutomirski
In-Reply-To: <CACdnJusPtYLdg7ZPhBo=Y5EsBz6B+5M2zYscBrLcc89oNnPkdQ@mail.gmail.com>
On 06/24/19 at 02:06pm, Matthew Garrett wrote:
> On Sun, Jun 23, 2019 at 6:52 PM Dave Young <dyoung@redhat.com> wrote:
> >
> > On 06/21/19 at 01:18pm, Matthew Garrett wrote:
> > > I don't think so - we want it to be possible to load images if they
> > > have a valid signature.
> >
> > I know it works like this way because of the previous patch. But from
> > the patch log "When KEXEC_SIG is not enabled, kernel should not load
> > images", it is simple to check it early for !IS_ENABLED(CONFIG_KEXEC_SIG) &&
> > kernel_is_locked_down(reason, LOCKDOWN_INTEGRITY) instead of depending
> > on the late code to verify signature. In that way, easier to
> > understand the logic, no?
>
> But that combination doesn't enforce signature validation? We can't
> depend on !IS_ENABLED(CONFIG_KEXEC_SIG_FORCE) because then it'll
> enforce signature validation even if lockdown is disabled.
Ok, got your point. still something could be improved though, in the switch
chunk, the errno, reason and IS_ENABLED(CONFIG_KEXEC_SIG_FORCE) etc is
not necessary for this -EPERM case.
/* add some comment to describe the behavior */
if (ret && security_is_locked_down(LOCKDOWN_KEXEC)) {
ret = -EPERM;
goto out;
}
Thanks
Dave
^ permalink raw reply
* Re: [PATCH v3 21/24] Audit: Store LSM audit information in an lsmblob
From: Paul Moore @ 2019-06-25 2:42 UTC (permalink / raw)
To: John Johansen
Cc: Casey Schaufler, casey.schaufler, James Morris,
linux-security-module, selinux, keescook, penguin-kernel,
Stephen Smalley, Eric Paris, linux-audit
In-Reply-To: <41f99313-1aa4-bacc-6767-8ee1389ca220@canonical.com>
On Mon, Jun 24, 2019 at 10:15 PM John Johansen
<john.johansen@canonical.com> wrote:
> On 6/24/19 6:46 PM, Paul Moore wrote:
> > On Mon, Jun 24, 2019 at 9:01 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> >> On 6/24/2019 2:33 PM, John Johansen wrote:
> >>> On 6/21/19 11:52 AM, Casey Schaufler wrote:
> >>>> Change the audit code to store full lsmblob data instead of
> >>>> a single u32 secid. This allows for multiple security modules
> >>>> to use the audit system at the same time. It also allows the
> >>>> removal of scaffolding code that was included during the
> >>>> revision of LSM interfaces.
> >>>>
> >>>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> >>> I know Kees raised this too, but I haven't seen a reply
> >>>
> >>> Eric (Paul is already CCed): I have directly added you because of
> >>> the question below.
> >>>
> >>> In summary there isn't necessarily a single secid any more, and
> >>> we need to know whether dropping the logging of the secid or
> >>> logging all secids is the correct action.
> >>
> >> It is to be considered that this is an error case. If
> >> everything is working normally you should have produced
> >> a secctx previously, which you'll have included in the
> >> audit record. Including the secid in the record ought to
> >> be pointless, as the secid is strictly an internal token
> >> with no meaning outside the running kernel. You are providing
> >> no security relevant information by providing the secid.
> >> I will grant the possibility that the secid might be useful
> >> in debugging, but for that a pr_warn is more appropriate
> >> than a field in the audit record.
> >
> > FWIW, this probably should have been CC'd to the audit list.
> >
> hrmm indeed, sorry
>
> > I agree that this is an error case (security_secid_to_secctx() failed
> > to resolve the secid) and further that logging the secid, or a
> > collection of secids, has little value the way things currently work.
> > Since secids are a private kernel implementation detail, we don't
> > really display them outside the context of the kernel, including in
> > the audit logs. Recording a secid in this case doesn't provide
> > anything meaningful since secids aren't recorded in the audit record
> > stream, only the secctxs, and there is no "magic decoder ring" to go
> > between the two in the audit logs, or anywhere else in userspace for
> > that matter.
>
> Okay, thanks. Casey I am good with just a pr_warn here. I just didn't
> have context of why it was going to the audit_log and didn't want
> to change that without some more input.
Hmm. Actually, let me change my comments slightly ... perhaps what we
should do here is keep the audit_log_format(), but change it from
audit_log_format("osid=%u",...) to audit_log_format("obj=?"). The "?"
is used in audit when we can't determine a piece of information, but
we normally log it. It wasn't used very widely originally, which is
probably why it isn't in this piece of code.
> >>>> ---
> >>>> kernel/audit.h | 6 +++---
> >>>> kernel/auditsc.c | 38 +++++++++++---------------------------
> >>>> 2 files changed, 14 insertions(+), 30 deletions(-)
> >
> > ...
> >
> >>>> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> >>>> index 0478680cd0a8..d3ad13f11788 100644
> >>>> --- a/kernel/auditsc.c
> >>>> +++ b/kernel/auditsc.c
> >>>> @@ -1187,21 +1184,18 @@ static void show_special(struct audit_context *context, int *call_panic)
> >>>> context->socketcall.args[i]);
> >>>> break; }
> >>>> case AUDIT_IPC: {
> >>>> - u32 osid = context->ipc.osid;
> >>>> + struct lsmblob *olsm = &context->ipc.olsm;
> >>>>
> >>>> audit_log_format(ab, "ouid=%u ogid=%u mode=%#ho",
> >>>> from_kuid(&init_user_ns, context->ipc.uid),
> >>>> from_kgid(&init_user_ns, context->ipc.gid),
> >>>> context->ipc.mode);
> >>>> - if (osid) {
> >>>> + if (lsmblob_is_set(olsm)) {
> >>>> struct lsmcontext lsmcxt;
> >>>> - struct lsmblob blob;
> >>>>
> >>>> - lsmblob_init(&blob, osid);
> >>>> - if (security_secid_to_secctx(&blob, &lsmcxt)) {
> >>>> - audit_log_format(ab, " osid=%u", osid);
> >>> I am not comfortable just dropping this I would think logging all secids is the
> >>> correct action here.
> >>>
> >>>
> >>>> + if (security_secid_to_secctx(olsm, &lsmcxt))
> >>>> *call_panic = 1;
> >>>> - } else {
> >>>> + else {
> >>>> audit_log_format(ab, " obj=%s", lsmcxt.context);
> >>>> security_release_secctx(&lsmcxt);
> >>>> }
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [PATCH V34 08/29] kexec_file: split KEXEC_VERIFY_SIG into KEXEC_SIG and KEXEC_SIG_FORCE
From: Dave Young @ 2019-06-25 2:35 UTC (permalink / raw)
To: Matthew Garrett
Cc: jmorris, linux-security-module, linux-kernel, linux-api,
Jiri Bohac, David Howells, Matthew Garrett, kexec
In-Reply-To: <20190624020106.GD2976@dhcp-128-65.nay.redhat.com>
On 06/24/19 at 10:01am, Dave Young wrote:
> On 06/21/19 at 05:03pm, Matthew Garrett wrote:
> > From: Jiri Bohac <jbohac@suse.cz>
> >
> > This is a preparatory patch for kexec_file_load() lockdown. A locked down
> > kernel needs to prevent unsigned kernel images from being loaded with
> > kexec_file_load(). Currently, the only way to force the signature
> > verification is compiling with KEXEC_VERIFY_SIG. This prevents loading
> > usigned images even when the kernel is not locked down at runtime.
> >
> > This patch splits KEXEC_VERIFY_SIG into KEXEC_SIG and KEXEC_SIG_FORCE.
> > Analogous to the MODULE_SIG and MODULE_SIG_FORCE for modules, KEXEC_SIG
> > turns on the signature verification but allows unsigned images to be
> > loaded. KEXEC_SIG_FORCE disallows images without a valid signature.
> >
> > [Modified by David Howells such that:
> >
> > (1) verify_pefile_signature() differentiates between no-signature and
> > sig-didn't-match in its returned errors.
> >
> > (2) kexec fails with EKEYREJECTED if there is a signature for which we
> > have a key, but signature doesn't match - even if in non-forcing mode.
> >
> > (3) kexec fails with EBADMSG or some other error if there is a signature
> > which cannot be parsed - even if in non-forcing mode.
> >
> > (4) kexec fails with ELIBBAD if the PE file cannot be parsed to extract
> > the signature - even if in non-forcing mode.
> >
> > ]
>
> Seems I do not see EBADMSG and ELIBBAD in this patch, also kexec fails
> with proper errno instead of EKEYREJECTED only.
>
> I may missed something? Other than the patch log issue:
>
> Reviewed-by: Dave Young <dyoung@redhat.com>
Hold on :) Noticed another issue, please see comment inline..
>
> >
> > Signed-off-by: Jiri Bohac <jbohac@suse.cz>
> > Signed-off-by: David Howells <dhowells@redhat.com>
> > Signed-off-by: Matthew Garrett <mjg59@google.com>
> > Reviewed-by: Jiri Bohac <jbohac@suse.cz>
> > cc: kexec@lists.infradead.org
> > ---
> > arch/x86/Kconfig | 20 ++++++++---
> > crypto/asymmetric_keys/verify_pefile.c | 4 ++-
> > include/linux/kexec.h | 4 +--
> > kernel/kexec_file.c | 47 ++++++++++++++++++++++----
> > 4 files changed, 60 insertions(+), 15 deletions(-)
> >
> > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> > index c1f9b3cf437c..84381dd60760 100644
> > --- a/arch/x86/Kconfig
> > +++ b/arch/x86/Kconfig
> > @@ -2012,20 +2012,30 @@ config KEXEC_FILE
> > config ARCH_HAS_KEXEC_PURGATORY
> > def_bool KEXEC_FILE
> >
> > -config KEXEC_VERIFY_SIG
> > +config KEXEC_SIG
> > bool "Verify kernel signature during kexec_file_load() syscall"
> > depends on KEXEC_FILE
> > ---help---
> > - This option makes kernel signature verification mandatory for
> > - the kexec_file_load() syscall.
> >
> > - In addition to that option, you need to enable signature
> > + This option makes the kexec_file_load() syscall check for a valid
> > + signature of the kernel image. The image can still be loaded without
> > + a valid signature unless you also enable KEXEC_SIG_FORCE, though if
> > + there's a signature that we can check, then it must be valid.
> > +
> > + In addition to this option, you need to enable signature
> > verification for the corresponding kernel image type being
> > loaded in order for this to work.
> >
> > +config KEXEC_SIG_FORCE
> > + bool "Require a valid signature in kexec_file_load() syscall"
> > + depends on KEXEC_SIG
> > + ---help---
> > + This option makes kernel signature verification mandatory for
> > + the kexec_file_load() syscall.
> > +
> > config KEXEC_BZIMAGE_VERIFY_SIG
> > bool "Enable bzImage signature verification support"
> > - depends on KEXEC_VERIFY_SIG
> > + depends on KEXEC_SIG
> > depends on SIGNED_PE_FILE_VERIFICATION
> > select SYSTEM_TRUSTED_KEYRING
> > ---help---
> > diff --git a/crypto/asymmetric_keys/verify_pefile.c b/crypto/asymmetric_keys/verify_pefile.c
> > index d178650fd524..4473cea1e877 100644
> > --- a/crypto/asymmetric_keys/verify_pefile.c
> > +++ b/crypto/asymmetric_keys/verify_pefile.c
> > @@ -100,7 +100,7 @@ static int pefile_parse_binary(const void *pebuf, unsigned int pelen,
> >
> > if (!ddir->certs.virtual_address || !ddir->certs.size) {
> > pr_debug("Unsigned PE binary\n");
> > - return -EKEYREJECTED;
> > + return -ENODATA;
> > }
> >
> > chkaddr(ctx->header_size, ddir->certs.virtual_address,
> > @@ -408,6 +408,8 @@ static int pefile_digest_pe(const void *pebuf, unsigned int pelen,
> > * (*) 0 if at least one signature chain intersects with the keys in the trust
> > * keyring, or:
> > *
> > + * (*) -ENODATA if there is no signature present.
> > + *
> > * (*) -ENOPKG if a suitable crypto module couldn't be found for a check on a
> > * chain.
> > *
> > diff --git a/include/linux/kexec.h b/include/linux/kexec.h
> > index b9b1bc5f9669..58b27c7bdc2b 100644
> > --- a/include/linux/kexec.h
> > +++ b/include/linux/kexec.h
> > @@ -125,7 +125,7 @@ typedef void *(kexec_load_t)(struct kimage *image, char *kernel_buf,
> > unsigned long cmdline_len);
> > typedef int (kexec_cleanup_t)(void *loader_data);
> >
> > -#ifdef CONFIG_KEXEC_VERIFY_SIG
> > +#ifdef CONFIG_KEXEC_SIG
> > typedef int (kexec_verify_sig_t)(const char *kernel_buf,
> > unsigned long kernel_len);
> > #endif
> > @@ -134,7 +134,7 @@ struct kexec_file_ops {
> > kexec_probe_t *probe;
> > kexec_load_t *load;
> > kexec_cleanup_t *cleanup;
> > -#ifdef CONFIG_KEXEC_VERIFY_SIG
> > +#ifdef CONFIG_KEXEC_SIG
> > kexec_verify_sig_t *verify_sig;
> > #endif
> > };
> > diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
> > index f1d0e00a3971..eec7e5bb2a08 100644
> > --- a/kernel/kexec_file.c
> > +++ b/kernel/kexec_file.c
> > @@ -90,7 +90,7 @@ int __weak arch_kimage_file_post_load_cleanup(struct kimage *image)
> > return kexec_image_post_load_cleanup_default(image);
> > }
> >
> > -#ifdef CONFIG_KEXEC_VERIFY_SIG
> > +#ifdef CONFIG_KEXEC_SIG
> > static int kexec_image_verify_sig_default(struct kimage *image, void *buf,
> > unsigned long buf_len)
> > {
> > @@ -188,7 +188,8 @@ kimage_file_prepare_segments(struct kimage *image, int kernel_fd, int initrd_fd,
> > const char __user *cmdline_ptr,
> > unsigned long cmdline_len, unsigned flags)
> > {
> > - int ret = 0;
> > + const char *reason;
> > + int ret;
> > void *ldata;
> > loff_t size;
> >
> > @@ -207,15 +208,47 @@ kimage_file_prepare_segments(struct kimage *image, int kernel_fd, int initrd_fd,
> > if (ret)
> > goto out;
> >
> > -#ifdef CONFIG_KEXEC_VERIFY_SIG
> > +#ifdef CONFIG_KEXEC_SIG
> > ret = arch_kexec_kernel_verify_sig(image, image->kernel_buf,
> > image->kernel_buf_len);
> > - if (ret) {
> > - pr_debug("kernel signature verification failed.\n");
> > +#else
> > + ret = -ENODATA;
Use -ENODATA for above case looks not correct, please just remove the #else and
move the #endif to the end of the switch chunk.
> > +#endif
> > +
> > + switch (ret) {
> > + case 0:
> > + break;
> > +
> > + /* Certain verification errors are non-fatal if we're not
> > + * checking errors, provided we aren't mandating that there
> > + * must be a valid signature.
> > + */
> > + case -ENODATA:
> > + reason = "kexec of unsigned image";
> > + goto decide;
> > + case -ENOPKG:
> > + reason = "kexec of image with unsupported crypto";
> > + goto decide;
> > + case -ENOKEY:
> > + reason = "kexec of image with unavailable key";
> > + decide:
> > + if (IS_ENABLED(CONFIG_KEXEC_SIG_FORCE)) {
> > + pr_notice("%s rejected\n", reason);
> > + goto out;
> > + }
> > +
> > + ret = 0;
> > + break;
> > +
> > + /* All other errors are fatal, including nomem, unparseable
> > + * signatures and signature check failures - even if signatures
> > + * aren't required.
> > + */
> > + default:
> > + pr_notice("kernel signature verification failed (%d).\n", ret);
> > goto out;
> > }
> > - pr_debug("kernel signature verification successful.\n");
> > -#endif
> > +
> > /* It is possible that there no initramfs is being loaded */
> > if (!(flags & KEXEC_FILE_NO_INITRAMFS)) {
> > ret = kernel_read_file_from_fd(initrd_fd, &image->initrd_buf,
> > --
> > 2.22.0.410.gd8fdbe21b5-goog
> >
Thanks
Dave
^ permalink raw reply
* Re: [PATCH v3 21/24] Audit: Store LSM audit information in an lsmblob
From: John Johansen @ 2019-06-25 2:14 UTC (permalink / raw)
To: Paul Moore, Casey Schaufler
Cc: casey.schaufler, James Morris, linux-security-module, selinux,
keescook, penguin-kernel, Stephen Smalley, Eric Paris,
linux-audit
In-Reply-To: <CAHC9VhSSwCY8L71x4WTr7kJhF1f_oyQ1NcwyXCAgW7ruKACQdQ@mail.gmail.com>
On 6/24/19 6:46 PM, Paul Moore wrote:
> On Mon, Jun 24, 2019 at 9:01 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>> On 6/24/2019 2:33 PM, John Johansen wrote:
>>> On 6/21/19 11:52 AM, Casey Schaufler wrote:
>>>> Change the audit code to store full lsmblob data instead of
>>>> a single u32 secid. This allows for multiple security modules
>>>> to use the audit system at the same time. It also allows the
>>>> removal of scaffolding code that was included during the
>>>> revision of LSM interfaces.
>>>>
>>>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
>>> I know Kees raised this too, but I haven't seen a reply
>>>
>>> Eric (Paul is already CCed): I have directly added you because of
>>> the question below.
>>>
>>> In summary there isn't necessarily a single secid any more, and
>>> we need to know whether dropping the logging of the secid or
>>> logging all secids is the correct action.
>>
>> It is to be considered that this is an error case. If
>> everything is working normally you should have produced
>> a secctx previously, which you'll have included in the
>> audit record. Including the secid in the record ought to
>> be pointless, as the secid is strictly an internal token
>> with no meaning outside the running kernel. You are providing
>> no security relevant information by providing the secid.
>> I will grant the possibility that the secid might be useful
>> in debugging, but for that a pr_warn is more appropriate
>> than a field in the audit record.
>
> FWIW, this probably should have been CC'd to the audit list.
>
hrmm indeed, sorry
> I agree that this is an error case (security_secid_to_secctx() failed
> to resolve the secid) and further that logging the secid, or a
> collection of secids, has little value the way things currently work.
> Since secids are a private kernel implementation detail, we don't
> really display them outside the context of the kernel, including in
> the audit logs. Recording a secid in this case doesn't provide
> anything meaningful since secids aren't recorded in the audit record
> stream, only the secctxs, and there is no "magic decoder ring" to go
> between the two in the audit logs, or anywhere else in userspace for
> that matter.
>
Okay, thanks. Casey I am good with just a pr_warn here. I just didn't
have context of why it was going to the audit_log and didn't want
to change that without some more input.
>>>> ---
>>>> kernel/audit.h | 6 +++---
>>>> kernel/auditsc.c | 38 +++++++++++---------------------------
>>>> 2 files changed, 14 insertions(+), 30 deletions(-)
>
> ...
>
>>>> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
>>>> index 0478680cd0a8..d3ad13f11788 100644
>>>> --- a/kernel/auditsc.c
>>>> +++ b/kernel/auditsc.c
>>>> @@ -1187,21 +1184,18 @@ static void show_special(struct audit_context *context, int *call_panic)
>>>> context->socketcall.args[i]);
>>>> break; }
>>>> case AUDIT_IPC: {
>>>> - u32 osid = context->ipc.osid;
>>>> + struct lsmblob *olsm = &context->ipc.olsm;
>>>>
>>>> audit_log_format(ab, "ouid=%u ogid=%u mode=%#ho",
>>>> from_kuid(&init_user_ns, context->ipc.uid),
>>>> from_kgid(&init_user_ns, context->ipc.gid),
>>>> context->ipc.mode);
>>>> - if (osid) {
>>>> + if (lsmblob_is_set(olsm)) {
>>>> struct lsmcontext lsmcxt;
>>>> - struct lsmblob blob;
>>>>
>>>> - lsmblob_init(&blob, osid);
>>>> - if (security_secid_to_secctx(&blob, &lsmcxt)) {
>>>> - audit_log_format(ab, " osid=%u", osid);
>>> I am not comfortable just dropping this I would think logging all secids is the
>>> correct action here.
>>>
>>>
>>>> + if (security_secid_to_secctx(olsm, &lsmcxt))
>>>> *call_panic = 1;
>>>> - } else {
>>>> + else {
>>>> audit_log_format(ab, " obj=%s", lsmcxt.context);
>>>> security_release_secctx(&lsmcxt);
>>>> }
>
^ permalink raw reply
* Re: [PATCH v3 21/24] Audit: Store LSM audit information in an lsmblob
From: Paul Moore @ 2019-06-25 1:46 UTC (permalink / raw)
To: Casey Schaufler
Cc: John Johansen, casey.schaufler, James Morris,
linux-security-module, selinux, keescook, penguin-kernel,
Stephen Smalley, Eric Paris, linux-audit
In-Reply-To: <0ad8f906-16ff-61af-ce7c-0ea1e9760d03@schaufler-ca.com>
On Mon, Jun 24, 2019 at 9:01 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> On 6/24/2019 2:33 PM, John Johansen wrote:
> > On 6/21/19 11:52 AM, Casey Schaufler wrote:
> >> Change the audit code to store full lsmblob data instead of
> >> a single u32 secid. This allows for multiple security modules
> >> to use the audit system at the same time. It also allows the
> >> removal of scaffolding code that was included during the
> >> revision of LSM interfaces.
> >>
> >> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> > I know Kees raised this too, but I haven't seen a reply
> >
> > Eric (Paul is already CCed): I have directly added you because of
> > the question below.
> >
> > In summary there isn't necessarily a single secid any more, and
> > we need to know whether dropping the logging of the secid or
> > logging all secids is the correct action.
>
> It is to be considered that this is an error case. If
> everything is working normally you should have produced
> a secctx previously, which you'll have included in the
> audit record. Including the secid in the record ought to
> be pointless, as the secid is strictly an internal token
> with no meaning outside the running kernel. You are providing
> no security relevant information by providing the secid.
> I will grant the possibility that the secid might be useful
> in debugging, but for that a pr_warn is more appropriate
> than a field in the audit record.
FWIW, this probably should have been CC'd to the audit list.
I agree that this is an error case (security_secid_to_secctx() failed
to resolve the secid) and further that logging the secid, or a
collection of secids, has little value the way things currently work.
Since secids are a private kernel implementation detail, we don't
really display them outside the context of the kernel, including in
the audit logs. Recording a secid in this case doesn't provide
anything meaningful since secids aren't recorded in the audit record
stream, only the secctxs, and there is no "magic decoder ring" to go
between the two in the audit logs, or anywhere else in userspace for
that matter.
> >> ---
> >> kernel/audit.h | 6 +++---
> >> kernel/auditsc.c | 38 +++++++++++---------------------------
> >> 2 files changed, 14 insertions(+), 30 deletions(-)
...
> >> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> >> index 0478680cd0a8..d3ad13f11788 100644
> >> --- a/kernel/auditsc.c
> >> +++ b/kernel/auditsc.c
> >> @@ -1187,21 +1184,18 @@ static void show_special(struct audit_context *context, int *call_panic)
> >> context->socketcall.args[i]);
> >> break; }
> >> case AUDIT_IPC: {
> >> - u32 osid = context->ipc.osid;
> >> + struct lsmblob *olsm = &context->ipc.olsm;
> >>
> >> audit_log_format(ab, "ouid=%u ogid=%u mode=%#ho",
> >> from_kuid(&init_user_ns, context->ipc.uid),
> >> from_kgid(&init_user_ns, context->ipc.gid),
> >> context->ipc.mode);
> >> - if (osid) {
> >> + if (lsmblob_is_set(olsm)) {
> >> struct lsmcontext lsmcxt;
> >> - struct lsmblob blob;
> >>
> >> - lsmblob_init(&blob, osid);
> >> - if (security_secid_to_secctx(&blob, &lsmcxt)) {
> >> - audit_log_format(ab, " osid=%u", osid);
> > I am not comfortable just dropping this I would think logging all secids is the
> > correct action here.
> >
> >
> >> + if (security_secid_to_secctx(olsm, &lsmcxt))
> >> *call_panic = 1;
> >> - } else {
> >> + else {
> >> audit_log_format(ab, " obj=%s", lsmcxt.context);
> >> security_release_secctx(&lsmcxt);
> >> }
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [PATCH V31 07/25] kexec_file: Restrict at runtime if the kernel is locked down
From: Mimi Zohar @ 2019-06-25 1:46 UTC (permalink / raw)
To: Matthew Garrett
Cc: Dave Young, James Morris, Jiri Bohac, Linux API, kexec,
Linux Kernel Mailing List, David Howells, LSM List,
Andy Lutomirski
In-Reply-To: <CACdnJuvE-MbD42AJTrio=0RaN8SaWo-RHHt21z=3an1vtjTFhA@mail.gmail.com>
On Mon, 2019-06-24 at 17:02 -0700, Matthew Garrett wrote:
> On Mon, Jun 24, 2019 at 2:27 PM Mimi Zohar <zohar@linux.ibm.com> wrote:
>
> > I agree with Dave. There should be a stub lockdown function to
> > prevent enforcing lockdown when it isn't enabled.
>
> Sorry, when what isn't enabled? If no LSMs are enforcing lockdown then
> the check will return 0. The goal here is for distributions to be able
> to ship a kernel that has CONFIG_KEXEC_SIG=y, CONFIG_KEXEC_SIG_FORCE=n
> and at runtime be able to enforce a policy that requires signatures on
> kexec payloads.
Never mind, the call can't be moved earlier.
^ permalink raw reply
* Re: [PATCH v3 21/24] Audit: Store LSM audit information in an lsmblob
From: Casey Schaufler @ 2019-06-25 1:01 UTC (permalink / raw)
To: John Johansen, casey.schaufler, jmorris, linux-security-module,
selinux
Cc: keescook, penguin-kernel, paul, sds, Eric Paris, casey
In-Reply-To: <79cd4a92-c221-eda4-58ba-730b5c2680d7@canonical.com>
On 6/24/2019 2:33 PM, John Johansen wrote:
> On 6/21/19 11:52 AM, Casey Schaufler wrote:
>> Change the audit code to store full lsmblob data instead of
>> a single u32 secid. This allows for multiple security modules
>> to use the audit system at the same time. It also allows the
>> removal of scaffolding code that was included during the
>> revision of LSM interfaces.
>>
>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> I know Kees raised this too, but I haven't seen a reply
>
> Eric (Paul is already CCed): I have directly added you because of
> the question below.
>
> In summary there isn't necessarily a single secid any more, and
> we need to know whether dropping the logging of the secid or
> logging all secids is the correct action.
It is to be considered that this is an error case. If
everything is working normally you should have produced
a secctx previously, which you'll have included in the
audit record. Including the secid in the record ought to
be pointless, as the secid is strictly an internal token
with no meaning outside the running kernel. You are providing
no security relevant information by providing the secid.
I will grant the possibility that the secid might be useful
in debugging, but for that a pr_warn is more appropriate
than a field in the audit record.
>
>> ---
>> kernel/audit.h | 6 +++---
>> kernel/auditsc.c | 38 +++++++++++---------------------------
>> 2 files changed, 14 insertions(+), 30 deletions(-)
>>
>> diff --git a/kernel/audit.h b/kernel/audit.h
>> index 29e29c6f4afb..a8dd479e9556 100644
>> --- a/kernel/audit.h
>> +++ b/kernel/audit.h
>> @@ -91,7 +91,7 @@ struct audit_names {
>> kuid_t uid;
>> kgid_t gid;
>> dev_t rdev;
>> - u32 osid;
>> + struct lsmblob olsm;
>> struct audit_cap_data fcap;
>> unsigned int fcap_ver;
>> unsigned char type; /* record type */
>> @@ -148,7 +148,7 @@ struct audit_context {
>> kuid_t target_auid;
>> kuid_t target_uid;
>> unsigned int target_sessionid;
>> - struct lsmblob target_lsm;
>> + struct lsmblob target_lsm;
>> char target_comm[TASK_COMM_LEN];
>>
>> struct audit_tree_refs *trees, *first_trees;
>> @@ -165,7 +165,7 @@ struct audit_context {
>> kuid_t uid;
>> kgid_t gid;
>> umode_t mode;
>> - u32 osid;
>> + struct lsmblob olsm;
>> int has_perm;
>> uid_t perm_uid;
>> gid_t perm_gid;
>> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
>> index 0478680cd0a8..d3ad13f11788 100644
>> --- a/kernel/auditsc.c
>> +++ b/kernel/auditsc.c
>> @@ -646,17 +646,15 @@ static int audit_filter_rules(struct task_struct *tsk,
>> if (f->lsm_rule) {
>> /* Find files that match */
>> if (name) {
>> - lsmblob_init(&blob, name->osid);
>> result = security_audit_rule_match(
>> - &blob,
>> + &name->olsm,
>> f->type,
>> f->op,
>> f->lsm_rule);
>> } else if (ctx) {
>> list_for_each_entry(n, &ctx->names_list, list) {
>> - lsmblob_init(&blob, n->osid);
>> if (security_audit_rule_match(
>> - &blob,
>> + &n->olsm,
>> f->type,
>> f->op,
>> f->lsm_rule)) {
>> @@ -668,8 +666,7 @@ static int audit_filter_rules(struct task_struct *tsk,
>> /* Find ipc objects that match */
>> if (!ctx || ctx->type != AUDIT_IPC)
>> break;
>> - lsmblob_init(&blob, ctx->ipc.osid);
>> - if (security_audit_rule_match(&blob,
>> + if (security_audit_rule_match(&ctx->ipc.olsm,
>> f->type, f->op,
>> f->lsm_rule))
>> ++result;
>> @@ -1187,21 +1184,18 @@ static void show_special(struct audit_context *context, int *call_panic)
>> context->socketcall.args[i]);
>> break; }
>> case AUDIT_IPC: {
>> - u32 osid = context->ipc.osid;
>> + struct lsmblob *olsm = &context->ipc.olsm;
>>
>> audit_log_format(ab, "ouid=%u ogid=%u mode=%#ho",
>> from_kuid(&init_user_ns, context->ipc.uid),
>> from_kgid(&init_user_ns, context->ipc.gid),
>> context->ipc.mode);
>> - if (osid) {
>> + if (lsmblob_is_set(olsm)) {
>> struct lsmcontext lsmcxt;
>> - struct lsmblob blob;
>>
>> - lsmblob_init(&blob, osid);
>> - if (security_secid_to_secctx(&blob, &lsmcxt)) {
>> - audit_log_format(ab, " osid=%u", osid);
> I am not comfortable just dropping this I would think logging all secids is the
> correct action here.
>
>
>> + if (security_secid_to_secctx(olsm, &lsmcxt))
>> *call_panic = 1;
>> - } else {
>> + else {
>> audit_log_format(ab, " obj=%s", lsmcxt.context);
>> security_release_secctx(&lsmcxt);
>> }
>> @@ -1346,13 +1340,10 @@ static void audit_log_name(struct audit_context *context, struct audit_names *n,
>> from_kgid(&init_user_ns, n->gid),
>> MAJOR(n->rdev),
>> MINOR(n->rdev));
>> - if (n->osid != 0) {
>> - struct lsmblob blob;
>> + if (lsmblob_is_set(&n->olsm)) {
>> struct lsmcontext lsmctx;
>>
>> - lsmblob_init(&blob, n->osid);
>> - if (security_secid_to_secctx(&blob, &lsmctx)) {
>> - audit_log_format(ab, " osid=%u", n->osid);
> and here
>
>
>> + if (security_secid_to_secctx(&n->olsm, &lsmctx)) {
>> if (call_panic)
>> *call_panic = 2;
>> } else {
>> @@ -1906,17 +1897,13 @@ static inline int audit_copy_fcaps(struct audit_names *name,
>> void audit_copy_inode(struct audit_names *name, const struct dentry *dentry,
>> struct inode *inode, unsigned int flags)
>> {
>> - struct lsmblob blob;
>> -
>> name->ino = inode->i_ino;
>> name->dev = inode->i_sb->s_dev;
>> name->mode = inode->i_mode;
>> name->uid = inode->i_uid;
>> name->gid = inode->i_gid;
>> name->rdev = inode->i_rdev;
>> - security_inode_getsecid(inode, &blob);
>> - /* scaffolding until osid is updated */
>> - name->osid = blob.secid[0];
>> + security_inode_getsecid(inode, &name->olsm);
>> if (flags & AUDIT_INODE_NOEVAL) {
>> name->fcap_ver = -1;
>> return;
>> @@ -2266,14 +2253,11 @@ void __audit_mq_getsetattr(mqd_t mqdes, struct mq_attr *mqstat)
>> void __audit_ipc_obj(struct kern_ipc_perm *ipcp)
>> {
>> struct audit_context *context = audit_context();
>> - struct lsmblob blob;
>> context->ipc.uid = ipcp->uid;
>> context->ipc.gid = ipcp->gid;
>> context->ipc.mode = ipcp->mode;
>> context->ipc.has_perm = 0;
>> - security_ipc_getsecid(ipcp, &blob);
>> - /* scaffolding on the [0] - change "osid" to a lsmblob */
>> - context->ipc.osid = blob.secid[0];
>> + security_ipc_getsecid(ipcp, &context->ipc.olsm);
>> context->type = AUDIT_IPC;
>> }
>>
>>
^ permalink raw reply
* Re: [PATCH V31 07/25] kexec_file: Restrict at runtime if the kernel is locked down
From: Matthew Garrett @ 2019-06-25 0:02 UTC (permalink / raw)
To: Mimi Zohar
Cc: Dave Young, James Morris, Jiri Bohac, Linux API, kexec,
Linux Kernel Mailing List, David Howells, LSM List,
Andy Lutomirski
In-Reply-To: <1561411657.4340.70.camel@linux.ibm.com>
On Mon, Jun 24, 2019 at 2:27 PM Mimi Zohar <zohar@linux.ibm.com> wrote:
> I agree with Dave. There should be a stub lockdown function to
> prevent enforcing lockdown when it isn't enabled.
Sorry, when what isn't enabled? If no LSMs are enforcing lockdown then
the check will return 0. The goal here is for distributions to be able
to ship a kernel that has CONFIG_KEXEC_SIG=y, CONFIG_KEXEC_SIG_FORCE=n
and at runtime be able to enforce a policy that requires signatures on
kexec payloads.
^ permalink raw reply
* Re: [PATCH V34 00/29] Lockdown as an LSM
From: Matthew Garrett @ 2019-06-24 23:56 UTC (permalink / raw)
To: James Morris
Cc: LSM List, Linux Kernel Mailing List, Linux API, Stephen Smalley,
Andy Lutomirski, John Johansen, Casey Schaufler
In-Reply-To: <alpine.LRH.2.21.1906250853290.7826@namei.org>
On Mon, Jun 24, 2019 at 4:01 PM James Morris <jmorris@namei.org> wrote:
>
> On Fri, 21 Jun 2019, Matthew Garrett wrote:
>
> > Minor updates over V33 - security_is_locked_down renamed to
> > security_locked_down, return value of security_locked_down is returned
> > in most cases, one unnecessary patch was dropped, couple of minor nits
> > fixed.
>
> Thanks for the respin.
>
> We are still not resolved on granularity. Stephen has said he's not sure
> if a useful policy can be constructed with just confidentiality and
> integrity settings. I'd be interested to know JJ and Casey's thoughts on
> lockdown policy flexibility wrt their respective LSMs.
This implementation provides arbitrary granularity at the LSM level,
though the lockdown LSM itself only provides two levels. Other LSMs
can choose an appropriate level of exposure.
> These are also "all or nothing" choices which may prevent deployment due
> to a user needing to allow (presumably controlled or mitigated) exceptions
> to the policy.
Distributions have been deploying the "all or nothing" solution for
several years now, which implies that it's adequate for the common
case. I think it's reasonable to punt finer grained policies over to
other LSMs - people who want that are probably already using custom
LSM policy.
^ permalink raw reply
* Re: [PATCH V34 00/29] Lockdown as an LSM
From: Casey Schaufler @ 2019-06-24 23:47 UTC (permalink / raw)
To: James Morris, Matthew Garrett
Cc: linux-security-module, linux-kernel, linux-api, Stephen Smalley,
Andy Lutomirski, John Johansen, casey
In-Reply-To: <alpine.LRH.2.21.1906250853290.7826@namei.org>
On 6/24/2019 4:01 PM, James Morris wrote:
> On Fri, 21 Jun 2019, Matthew Garrett wrote:
>
>> Minor updates over V33 - security_is_locked_down renamed to
>> security_locked_down, return value of security_locked_down is returned
>> in most cases, one unnecessary patch was dropped, couple of minor nits
>> fixed.
> Thanks for the respin.
>
> We are still not resolved on granularity. Stephen has said he's not sure
> if a useful policy can be constructed with just confidentiality and
> integrity settings. I'd be interested to know JJ and Casey's thoughts on
> lockdown policy flexibility wrt their respective LSMs.
Smack is a mandatory access control mechanism on named
objects controlled by the system. Issues of administrative
control, like whether hibernation is allowed, are outside
the scope of what Smack controls. There may be some subject/object
implications, but I have not identified any yet.
> These are also "all or nothing" choices which may prevent deployment due
> to a user needing to allow (presumably controlled or mitigated) exceptions
> to the policy.
^ permalink raw reply
* Re: [PATCH V34 00/29] Lockdown as an LSM
From: James Morris @ 2019-06-24 23:01 UTC (permalink / raw)
To: Matthew Garrett
Cc: linux-security-module, linux-kernel, linux-api, Stephen Smalley,
Andy Lutomirski, John Johansen, Casey Schaufler
In-Reply-To: <20190622000358.19895-1-matthewgarrett@google.com>
On Fri, 21 Jun 2019, Matthew Garrett wrote:
> Minor updates over V33 - security_is_locked_down renamed to
> security_locked_down, return value of security_locked_down is returned
> in most cases, one unnecessary patch was dropped, couple of minor nits
> fixed.
Thanks for the respin.
We are still not resolved on granularity. Stephen has said he's not sure
if a useful policy can be constructed with just confidentiality and
integrity settings. I'd be interested to know JJ and Casey's thoughts on
lockdown policy flexibility wrt their respective LSMs.
These are also "all or nothing" choices which may prevent deployment due
to a user needing to allow (presumably controlled or mitigated) exceptions
to the policy.
--
James Morris
<jmorris@namei.org>
^ permalink raw reply
* Re: [PATCH V10 2/3] IMA: Define a new template field buf
From: Thiago Jung Bauermann @ 2019-06-24 22:03 UTC (permalink / raw)
To: Prakhar Srivastava
Cc: linux-integrity, linux-security-module, linux-kernel, zohar,
roberto.sassu, vgoyal
In-Reply-To: <20190624062331.388-3-prsriva02@gmail.com>
Hello Prakhar,
Prakhar Srivastava <prsriva02@gmail.com> writes:
> diff --git a/security/integrity/ima/ima_template.c b/security/integrity/ima/ima_template.c
> index 00dd5a434689..a01a17e5c581 100644
> --- a/security/integrity/ima/ima_template.c
> +++ b/security/integrity/ima/ima_template.c
> @@ -26,6 +26,7 @@ static struct ima_template_desc builtin_templates[] = {
> {.name = IMA_TEMPLATE_IMA_NAME, .fmt = IMA_TEMPLATE_IMA_FMT},
> {.name = "ima-ng", .fmt = "d-ng|n-ng"},
> {.name = "ima-sig", .fmt = "d-ng|n-ng|sig"},
> + {.name = "ima-buf", .fmt = "d-ng|n-ng|buf"},
> {.name = "", .fmt = ""}, /* placeholder for a custom format */
> };
>
> @@ -43,6 +44,8 @@ static const struct ima_template_field supported_fields[] = {
> .field_show = ima_show_template_string},
> {.field_id = "sig", .field_init = ima_eventsig_init,
> .field_show = ima_show_template_sig},
> + {.field_id = "buf", .field_init = ima_eventbuf_init,
> + .field_show = ima_show_template_buf},
> };
> #define MAX_TEMPLATE_NAME_LEN 15
Currently, MAX_TEMPLATE_NAME_LEN is the length of a template that
contains all valid fields. It may make sense to increase it since
there's a new field being added.
I suggest using a sizeof() to show where the number comes from (and
which can be visually shown to be correct):
#define MAX_TEMPLATE_NAME_LEN sizeof("d|n|d-ng|n-ng|sig|buf")
The sizeof() is calculated at compile time.
--
Thiago Jung Bauermann
IBM Linux Technology Center
^ permalink raw reply
* Re: [PATCH v3 22/24] LSM: Return the lsmblob slot on initialization
From: Casey Schaufler @ 2019-06-24 21:53 UTC (permalink / raw)
To: John Johansen, Kees Cook
Cc: casey.schaufler, jmorris, linux-security-module, selinux,
penguin-kernel, paul, sds
In-Reply-To: <6d18ee4f-fe1b-39ae-dbe6-59ff120112c4@canonical.com>
On 6/24/2019 2:39 PM, John Johansen wrote:
> On 6/22/19 4:13 PM, Kees Cook wrote:
>> On Fri, Jun 21, 2019 at 11:52:31AM -0700, Casey Schaufler wrote:
>>> Return the slot allocated to the calling LSM in the lsmblob
>>> structure. This can be used to set lsmblobs explicitly for
>>> netlabel interfaces.
>>>
>>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
>> Reviewed-by: Kees Cook <keescook@chromium.org>
>>
>> (I have some thoughts on refactoring the slot assignment, but that
>> should happen after this series -- it's nothing more than a storage
>> optimization.)
>>
>> -Kees
> haha so do I, now I am curious as to how close they align
Plan A: Each LSM has a lsm_id {.name, .slot}, the address of which
is passed to security_add_hooks() in place of the current char *lsm.
This is added to each hook instead of the *lsm. The slot value is
set if the LSM requests one. One slot integer per LSM instead of one
per hook.
>
>
>>> ---
>>> include/linux/lsm_hooks.h | 4 ++--
>>> security/apparmor/lsm.c | 8 ++++++--
>>> security/security.c | 9 +++++++--
>>> security/selinux/hooks.c | 5 ++++-
>>> security/smack/smack_lsm.c | 5 ++++-
>>> 5 files changed, 23 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
>>> index 4d1ddf1a2aa6..ce341bcbce5d 100644
>>> --- a/include/linux/lsm_hooks.h
>>> +++ b/include/linux/lsm_hooks.h
>>> @@ -2068,8 +2068,8 @@ struct lsm_blob_sizes {
>>> extern struct security_hook_heads security_hook_heads;
>>> extern char *lsm_names;
>>>
>>> -extern void security_add_hooks(struct security_hook_list *hooks, int count,
>>> - char *lsm);
>>> +extern int security_add_hooks(struct security_hook_list *hooks, int count,
>>> + char *lsm);
>>>
>>> #define LSM_FLAG_LEGACY_MAJOR BIT(0)
>>> #define LSM_FLAG_EXCLUSIVE BIT(1)
>>> diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
>>> index 2716e7731279..dcbbefbd95ff 100644
>>> --- a/security/apparmor/lsm.c
>>> +++ b/security/apparmor/lsm.c
>>> @@ -47,6 +47,9 @@
>>> /* Flag indicating whether initialization completed */
>>> int apparmor_initialized;
>>>
>>> +/* Slot for the AppArmor secid in the lsmblob structure */
>>> +int apparmor_lsmblob_slot;
>>> +
>>> DEFINE_PER_CPU(struct aa_buffers, aa_buffers);
>>>
>>>
>>> @@ -1678,8 +1681,9 @@ static int __init apparmor_init(void)
>>> aa_free_root_ns();
>>> goto buffers_out;
>>> }
>>> - security_add_hooks(apparmor_hooks, ARRAY_SIZE(apparmor_hooks),
>>> - "apparmor");
>>> + apparmor_lsmblob_slot = security_add_hooks(apparmor_hooks,
>>> + ARRAY_SIZE(apparmor_hooks),
>>> + "apparmor");
>>>
>>> /* Report that AppArmor successfully initialized */
>>> apparmor_initialized = 1;
>>> diff --git a/security/security.c b/security/security.c
>>> index b2ffcd1f3057..c93a368b697b 100644
>>> --- a/security/security.c
>>> +++ b/security/security.c
>>> @@ -437,9 +437,12 @@ static int lsm_slot __initdata;
>>> * Each LSM has to register its hooks with the infrastructure.
>>> * If the LSM is using hooks that export secids allocate a slot
>>> * for it in the lsmblob.
>>> + *
>>> + * Returns the slot number in the lsmblob structure if one is
>>> + * allocated or LSMBLOB_INVALID if one was not allocated.
>>> */
>>> -void __init security_add_hooks(struct security_hook_list *hooks, int count,
>>> - char *lsm)
>>> +int __init security_add_hooks(struct security_hook_list *hooks, int count,
>>> + char *lsm)
>>> {
>>> int slot = LSMBLOB_INVALID;
>>> int i;
>>> @@ -479,6 +482,8 @@ void __init security_add_hooks(struct security_hook_list *hooks, int count,
>>> }
>>> if (lsm_append(lsm, &lsm_names) < 0)
>>> panic("%s - Cannot get early memory.\n", __func__);
>>> +
>>> + return slot;
>>> }
>>>
>>> int call_lsm_notifier(enum lsm_event event, void *data)
>>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>>> index ee840fecfebb..1e09acbf9630 100644
>>> --- a/security/selinux/hooks.c
>>> +++ b/security/selinux/hooks.c
>>> @@ -103,6 +103,7 @@
>>> #include "avc_ss.h"
>>>
>>> struct selinux_state selinux_state;
>>> +int selinux_lsmblob_slot;
>>>
>>> /* SECMARK reference count */
>>> static atomic_t selinux_secmark_refcount = ATOMIC_INIT(0);
>>> @@ -6877,7 +6878,9 @@ static __init int selinux_init(void)
>>>
>>> hashtab_cache_init();
>>>
>>> - security_add_hooks(selinux_hooks, ARRAY_SIZE(selinux_hooks), "selinux");
>>> + selinux_lsmblob_slot = security_add_hooks(selinux_hooks,
>>> + ARRAY_SIZE(selinux_hooks),
>>> + "selinux");
>>>
>>> if (avc_add_callback(selinux_netcache_avc_callback, AVC_CALLBACK_RESET))
>>> panic("SELinux: Unable to register AVC netcache callback\n");
>>> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
>>> index 3834b751d1e9..273f311fb153 100644
>>> --- a/security/smack/smack_lsm.c
>>> +++ b/security/smack/smack_lsm.c
>>> @@ -60,6 +60,7 @@ static LIST_HEAD(smk_ipv6_port_list);
>>> #endif
>>> static struct kmem_cache *smack_inode_cache;
>>> int smack_enabled;
>>> +int smack_lsmblob_slot;
>>>
>>> #define A(s) {"smack"#s, sizeof("smack"#s) - 1, Opt_##s}
>>> static struct {
>>> @@ -4749,7 +4750,9 @@ static __init int smack_init(void)
>>> /*
>>> * Register with LSM
>>> */
>>> - security_add_hooks(smack_hooks, ARRAY_SIZE(smack_hooks), "smack");
>>> + smack_lsmblob_slot = security_add_hooks(smack_hooks,
>>> + ARRAY_SIZE(smack_hooks),
>>> + "smack");
>>> smack_enabled = 1;
>>>
>>> pr_info("Smack: Initializing.\n");
>>> --
>>> 2.20.1
>>>
^ permalink raw reply
* Re: [PATCH v3 22/24] LSM: Return the lsmblob slot on initialization
From: Kees Cook @ 2019-06-24 21:50 UTC (permalink / raw)
To: John Johansen
Cc: Casey Schaufler, casey.schaufler, jmorris, linux-security-module,
selinux, penguin-kernel, paul, sds
In-Reply-To: <6d18ee4f-fe1b-39ae-dbe6-59ff120112c4@canonical.com>
On Mon, Jun 24, 2019 at 02:39:23PM -0700, John Johansen wrote:
> On 6/22/19 4:13 PM, Kees Cook wrote:
> > On Fri, Jun 21, 2019 at 11:52:31AM -0700, Casey Schaufler wrote:
> >> Return the slot allocated to the calling LSM in the lsmblob
> >> structure. This can be used to set lsmblobs explicitly for
> >> netlabel interfaces.
> >>
> >> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> >
> > Reviewed-by: Kees Cook <keescook@chromium.org>
> >
> > (I have some thoughts on refactoring the slot assignment, but that
> > should happen after this series -- it's nothing more than a storage
> > optimization.)
> >
> > -Kees
>
> haha so do I, now I am curious as to how close they align
My plan is to create a __ro_after_init "LSM State" structure to hold
various things that are needed after init time. All of the lsm_info
structures are __initdata, so they can't be retained after init
(currently). As far as I can see, there are already a few things that
are retained after init:
LSM name (currently a pointer in every hook)
enabled state (technically optional, currently known internally to each LSM)
blob size details (currently known internally to each LSM)
and now we'll be adding the "slot". So I was thinking something like:
struct lsm_state {
const char *name;
int enabled;
struct blob_sizes lbs;
int slot;
};
And then change the hooks to each carry a pointer to the lsm_state
(instead of a name pointer). Also I think it'd be cleanest to define
and export the lsm_state instance it via the DEFINE_LSM macro, possibly
instead of the existing blob_size pointer. It's possible that lsm_info
should just gain some fields and no longer be __initdata, too. We'll
see!
--
Kees Cook
^ permalink raw reply
* [PATCH v3 22/24] LSM: Return the lsmblob slot on initialization
From: John Johansen @ 2019-06-24 21:47 UTC (permalink / raw)
To: Casey Schaufler, casey.schaufler, jmorris, linux-security-module,
selinux
Cc: keescook, penguin-kernel, paul, sds
In-Reply-To: <20190621185233.6766-23-casey@schaufler-ca.com>
On 6/21/19 11:52 AM, Casey Schaufler wrote:
> Return the slot allocated to the calling LSM in the lsmblob
> structure. This can be used to set lsmblobs explicitly for
> netlabel interfaces.
>
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
Reviewed-by: John Johansen <john.johansen@canonical.com>
> ---
> include/linux/lsm_hooks.h | 4 ++--
> security/apparmor/lsm.c | 8 ++++++--
> security/security.c | 9 +++++++--
> security/selinux/hooks.c | 5 ++++-
> security/smack/smack_lsm.c | 5 ++++-
> 5 files changed, 23 insertions(+), 8 deletions(-)
>
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index 4d1ddf1a2aa6..ce341bcbce5d 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -2068,8 +2068,8 @@ struct lsm_blob_sizes {
> extern struct security_hook_heads security_hook_heads;
> extern char *lsm_names;
>
> -extern void security_add_hooks(struct security_hook_list *hooks, int count,
> - char *lsm);
> +extern int security_add_hooks(struct security_hook_list *hooks, int count,
> + char *lsm);
>
> #define LSM_FLAG_LEGACY_MAJOR BIT(0)
> #define LSM_FLAG_EXCLUSIVE BIT(1)
> diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
> index 2716e7731279..dcbbefbd95ff 100644
> --- a/security/apparmor/lsm.c
> +++ b/security/apparmor/lsm.c
> @@ -47,6 +47,9 @@
> /* Flag indicating whether initialization completed */
> int apparmor_initialized;
>
> +/* Slot for the AppArmor secid in the lsmblob structure */
> +int apparmor_lsmblob_slot;
> +
> DEFINE_PER_CPU(struct aa_buffers, aa_buffers);
>
>
> @@ -1678,8 +1681,9 @@ static int __init apparmor_init(void)
> aa_free_root_ns();
> goto buffers_out;
> }
> - security_add_hooks(apparmor_hooks, ARRAY_SIZE(apparmor_hooks),
> - "apparmor");
> + apparmor_lsmblob_slot = security_add_hooks(apparmor_hooks,
> + ARRAY_SIZE(apparmor_hooks),
> + "apparmor");
>
> /* Report that AppArmor successfully initialized */
> apparmor_initialized = 1;
> diff --git a/security/security.c b/security/security.c
> index b2ffcd1f3057..c93a368b697b 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -437,9 +437,12 @@ static int lsm_slot __initdata;
> * Each LSM has to register its hooks with the infrastructure.
> * If the LSM is using hooks that export secids allocate a slot
> * for it in the lsmblob.
> + *
> + * Returns the slot number in the lsmblob structure if one is
> + * allocated or LSMBLOB_INVALID if one was not allocated.
> */
> -void __init security_add_hooks(struct security_hook_list *hooks, int count,
> - char *lsm)
> +int __init security_add_hooks(struct security_hook_list *hooks, int count,
> + char *lsm)
> {
> int slot = LSMBLOB_INVALID;
> int i;
> @@ -479,6 +482,8 @@ void __init security_add_hooks(struct security_hook_list *hooks, int count,
> }
> if (lsm_append(lsm, &lsm_names) < 0)
> panic("%s - Cannot get early memory.\n", __func__);
> +
> + return slot;
> }
>
> int call_lsm_notifier(enum lsm_event event, void *data)
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index ee840fecfebb..1e09acbf9630 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -103,6 +103,7 @@
> #include "avc_ss.h"
>
> struct selinux_state selinux_state;
> +int selinux_lsmblob_slot;
>
> /* SECMARK reference count */
> static atomic_t selinux_secmark_refcount = ATOMIC_INIT(0);
> @@ -6877,7 +6878,9 @@ static __init int selinux_init(void)
>
> hashtab_cache_init();
>
> - security_add_hooks(selinux_hooks, ARRAY_SIZE(selinux_hooks), "selinux");
> + selinux_lsmblob_slot = security_add_hooks(selinux_hooks,
> + ARRAY_SIZE(selinux_hooks),
> + "selinux");
>
> if (avc_add_callback(selinux_netcache_avc_callback, AVC_CALLBACK_RESET))
> panic("SELinux: Unable to register AVC netcache callback\n");
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index 3834b751d1e9..273f311fb153 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -60,6 +60,7 @@ static LIST_HEAD(smk_ipv6_port_list);
> #endif
> static struct kmem_cache *smack_inode_cache;
> int smack_enabled;
> +int smack_lsmblob_slot;
>
> #define A(s) {"smack"#s, sizeof("smack"#s) - 1, Opt_##s}
> static struct {
> @@ -4749,7 +4750,9 @@ static __init int smack_init(void)
> /*
> * Register with LSM
> */
> - security_add_hooks(smack_hooks, ARRAY_SIZE(smack_hooks), "smack");
> + smack_lsmblob_slot = security_add_hooks(smack_hooks,
> + ARRAY_SIZE(smack_hooks),
> + "smack");
> smack_enabled = 1;
>
> pr_info("Smack: Initializing.\n");
>
^ permalink raw reply
* [PATCH v3 20/24] LSM: security_secid_to_secctx in netlink netfilter
From: John Johansen @ 2019-06-24 21:47 UTC (permalink / raw)
To: Casey Schaufler, casey.schaufler, jmorris, linux-security-module,
selinux
Cc: keescook, penguin-kernel, paul, sds
In-Reply-To: <20190621185233.6766-21-casey@schaufler-ca.com>
On 6/21/19 11:52 AM, Casey Schaufler wrote:
> Change netlink netfilter interfaces to use lsmcontext
> pointers, and remove scaffolding.
>
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
Reviewed-by: John Johansen <john.johansen@canonical.com>
> ---
> net/netfilter/nfnetlink_queue.c | 32 +++++++++++++-------------------
> 1 file changed, 13 insertions(+), 19 deletions(-)
>
> diff --git a/net/netfilter/nfnetlink_queue.c b/net/netfilter/nfnetlink_queue.c
> index 6da00c7add5b..69efb688383f 100644
> --- a/net/netfilter/nfnetlink_queue.c
> +++ b/net/netfilter/nfnetlink_queue.c
> @@ -305,12 +305,10 @@ static int nfqnl_put_sk_uidgid(struct sk_buff *skb, struct sock *sk)
> return -1;
> }
>
> -static u32 nfqnl_get_sk_secctx(struct sk_buff *skb, char **secdata)
> +static u32 nfqnl_get_sk_secctx(struct sk_buff *skb, struct lsmcontext *context)
> {
> - u32 seclen = 0;
> #if IS_ENABLED(CONFIG_NETWORK_SECMARK)
> struct lsmblob blob;
> - struct lsmcontext context;
>
> if (!skb || !sk_fullsock(skb->sk))
> return 0;
> @@ -318,15 +316,16 @@ static u32 nfqnl_get_sk_secctx(struct sk_buff *skb, char **secdata)
> read_lock_bh(&skb->sk->sk_callback_lock);
>
> if (skb->secmark) {
> + /* Any LSM might be looking for the secmark */
> lsmblob_init(&blob, skb->secmark);
> - security_secid_to_secctx(&blob, &context);
> - *secdata = context.context;
> + security_secid_to_secctx(&blob, context);
> }
>
> read_unlock_bh(&skb->sk->sk_callback_lock);
> - seclen = context.len;
> + return context->len;
> +#else
> + return 0;
> #endif
> - return seclen;
> }
>
> static u32 nfqnl_get_bridge_size(struct nf_queue_entry *entry)
> @@ -402,8 +401,7 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
> enum ip_conntrack_info uninitialized_var(ctinfo);
> struct nfnl_ct_hook *nfnl_ct;
> bool csum_verify;
> - struct lsmcontext scaff; /* scaffolding */
> - char *secdata = NULL;
> + struct lsmcontext context;
> u32 seclen = 0;
>
> size = nlmsg_total_size(sizeof(struct nfgenmsg))
> @@ -470,7 +468,7 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
> }
>
> if ((queue->flags & NFQA_CFG_F_SECCTX) && entskb->sk) {
> - seclen = nfqnl_get_sk_secctx(entskb, &secdata);
> + seclen = nfqnl_get_sk_secctx(entskb, &context);
> if (seclen)
> size += nla_total_size(seclen);
> }
> @@ -605,7 +603,7 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
> nfqnl_put_sk_uidgid(skb, entskb->sk) < 0)
> goto nla_put_failure;
>
> - if (seclen && nla_put(skb, NFQA_SECCTX, seclen, secdata))
> + if (seclen && nla_put(skb, NFQA_SECCTX, context.len, context.context))
> goto nla_put_failure;
>
> if (ct && nfnl_ct->build(skb, ct, ctinfo, NFQA_CT, NFQA_CT_INFO) < 0)
> @@ -633,10 +631,8 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
> }
>
> nlh->nlmsg_len = skb->len;
> - if (seclen) {
> - lsmcontext_init(&scaff, secdata, seclen, 0);
> - security_release_secctx(&scaff);
> - }
> + if (seclen)
> + security_release_secctx(&context);
> return skb;
>
> nla_put_failure:
> @@ -644,10 +640,8 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
> kfree_skb(skb);
> net_err_ratelimited("nf_queue: error creating packet message\n");
> nlmsg_failure:
> - if (seclen) {
> - lsmcontext_init(&scaff, secdata, seclen, 0);
> - security_release_secctx(&scaff);
> - }
> + if (seclen)
> + security_release_secctx(&context);
> return NULL;
> }
>
>
^ permalink raw reply
* [PATCH v3 19/24] LSM: Use lsmcontext in security_inode_getsecctx
From: John Johansen @ 2019-06-24 21:46 UTC (permalink / raw)
To: Casey Schaufler, casey.schaufler, jmorris, linux-security-module,
selinux
Cc: keescook, penguin-kernel, paul, sds
In-Reply-To: <20190621185233.6766-20-casey@schaufler-ca.com>
On 6/21/19 11:52 AM, Casey Schaufler wrote:
> Change the security_inode_getsecctx() interface to fill
> a lsmcontext structure instead of data and length pointers.
> This provides the information about which LSM created the
> context so that security_release_secctx() can use the
> correct hook. A lsmcontext is used within kernfs to store
> the security information as well.
>
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
with the issue below fixed
Reviewed-by: John Johansen <john.johansen@canonical.com>
> ---
> fs/kernfs/dir.c | 8 ++------
> fs/kernfs/inode.c | 34 ++++++++++++----------------------
> fs/kernfs/kernfs-internal.h | 3 +--
> fs/nfsd/nfs4xdr.c | 23 +++++++++--------------
> include/linux/security.h | 5 +++--
> security/security.c | 13 +++++++++++--
> 6 files changed, 38 insertions(+), 48 deletions(-)
>
> diff --git a/fs/kernfs/dir.c b/fs/kernfs/dir.c
> index 92afad387237..1d000289d8b7 100644
> --- a/fs/kernfs/dir.c
> +++ b/fs/kernfs/dir.c
> @@ -532,12 +532,8 @@ void kernfs_put(struct kernfs_node *kn)
> kfree_const(kn->name);
>
> if (kn->iattr) {
> - struct lsmcontext scaff; /* scaffolding */
> - if (kn->iattr->ia_secdata) {
> - lsmcontext_init(&scaff, kn->iattr->ia_secdata,
> - kn->iattr->ia_secdata_len, 0);
> - security_release_secctx(&scaff);
> - }
> + if (kn->iattr->ia_context.context)
> + security_release_secctx(&kn->iattr->ia_context);
> simple_xattrs_free(&kn->iattr->xattrs);
> kmem_cache_free(kernfs_iattrs_cache, kn->iattr);
> }
> diff --git a/fs/kernfs/inode.c b/fs/kernfs/inode.c
> index 02cde9dac5ee..ffbf7863306d 100644
> --- a/fs/kernfs/inode.c
> +++ b/fs/kernfs/inode.c
> @@ -135,21 +135,14 @@ int kernfs_iop_setattr(struct dentry *dentry, struct iattr *iattr)
> return error;
> }
>
> -static int kernfs_node_setsecdata(struct kernfs_iattrs *attrs, void **secdata,
> - u32 *secdata_len)
> +static void kernfs_node_setsecdata(struct kernfs_iattrs *attrs,
> + struct lsmcontext *cp)
> {
> - void *old_secdata;
> - size_t old_secdata_len;
> + struct lsmcontext old_context;
>
> - old_secdata = attrs->ia_secdata;
> - old_secdata_len = attrs->ia_secdata_len;
> -
> - attrs->ia_secdata = *secdata;
> - attrs->ia_secdata_len = *secdata_len;
> -
> - *secdata = old_secdata;
> - *secdata_len = old_secdata_len;
> - return 0;
> + old_context = attrs->ia_context;
> + attrs->ia_context = *cp;
> + *cp = old_context;
> }
>
> ssize_t kernfs_iop_listxattr(struct dentry *dentry, char *buf, size_t size)
> @@ -192,8 +185,8 @@ static void kernfs_refresh_inode(struct kernfs_node *kn, struct inode *inode)
> * persistent copy in kernfs_node.
> */
> set_inode_attr(inode, &attrs->ia_iattr);
> - security_inode_notifysecctx(inode, attrs->ia_secdata,
> - attrs->ia_secdata_len);
> + security_inode_notifysecctx(inode, attrs->ia_context.context,
> + attrs->ia_context.len);
> }
>
> if (kernfs_type(kn) == KERNFS_DIR)
> @@ -350,8 +343,6 @@ static int kernfs_security_xattr_set(const struct xattr_handler *handler,
> struct kernfs_node *kn = inode->i_private;
> struct kernfs_iattrs *attrs;
> struct lsmcontext context;
> - void *secdata;
> - u32 secdata_len = 0;
> int error;
>
> attrs = kernfs_iattrs(kn);
> @@ -361,18 +352,17 @@ static int kernfs_security_xattr_set(const struct xattr_handler *handler,
> error = security_inode_setsecurity(inode, suffix, value, size, flags);
> if (error)
> return error;
> - error = security_inode_getsecctx(inode, &secdata, &secdata_len);
> + error = security_inode_getsecctx(inode, &context);
> if (error)
> return error;
>
> mutex_lock(&kernfs_mutex);
> - error = kernfs_node_setsecdata(attrs, &secdata, &secdata_len);
> + kernfs_node_setsecdata(attrs, &context);
> mutex_unlock(&kernfs_mutex);
>
> - if (secdata) {
> - lsmcontext_init(&context, secdata, secdata_len, 0);
> + if (context.context)
> security_release_secctx(&context);
> - }
> +
> return error;
> }
>
> diff --git a/fs/kernfs/kernfs-internal.h b/fs/kernfs/kernfs-internal.h
> index 0b7d197a904c..844a028d282f 100644
> --- a/fs/kernfs/kernfs-internal.h
> +++ b/fs/kernfs/kernfs-internal.h
> @@ -21,8 +21,7 @@
>
> struct kernfs_iattrs {
> struct iattr ia_iattr;
> - void *ia_secdata;
> - u32 ia_secdata_len;
> + struct lsmcontext ia_context;
>
> struct simple_xattrs xattrs;
> };
> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> index bb3db033e144..1209083565dd 100644
> --- a/fs/nfsd/nfs4xdr.c
> +++ b/fs/nfsd/nfs4xdr.c
> @@ -2304,11 +2304,11 @@ nfsd4_encode_layout_types(struct xdr_stream *xdr, u32 layout_types)
> #ifdef CONFIG_NFSD_V4_SECURITY_LABEL
> static inline __be32
> nfsd4_encode_security_label(struct xdr_stream *xdr, struct svc_rqst *rqstp,
> - void *context, int len)
> + struct lsmcontext *context)
> {
> __be32 *p;
>
> - p = xdr_reserve_space(xdr, len + 4 + 4 + 4);
> + p = xdr_reserve_space(xdr, context->len + 4 + 4 + 4);
> if (!p)
> return nfserr_resource;
>
> @@ -2318,13 +2318,13 @@ nfsd4_encode_security_label(struct xdr_stream *xdr, struct svc_rqst *rqstp,
> */
> *p++ = cpu_to_be32(0); /* lfs */
> *p++ = cpu_to_be32(0); /* pi */
> - p = xdr_encode_opaque(p, context, len);
> + p = xdr_encode_opaque(p, context->context, context->len);
> return 0;
> }
> #else
> static inline __be32
> nfsd4_encode_security_label(struct xdr_stream *xdr, struct svc_rqst *rqstp,
> - void *context, int len)
> + struct lsmcontext *context)
> { return 0; }
> #endif
>
> @@ -2420,9 +2420,7 @@ nfsd4_encode_fattr(struct xdr_stream *xdr, struct svc_fh *fhp,
> __be32 status;
> int err;
> struct nfs4_acl *acl = NULL;
> - struct lsmcontext scaff; /* scaffolding */
> - void *context = NULL;
> - int contextlen;
> + struct lsmcontext context;
> bool contextsupport = false;
> struct nfsd4_compoundres *resp = rqstp->rq_resp;
> u32 minorversion = resp->cstate.minorversion;
> @@ -2479,7 +2477,7 @@ nfsd4_encode_fattr(struct xdr_stream *xdr, struct svc_fh *fhp,
> bmval0 & FATTR4_WORD0_SUPPORTED_ATTRS) {
> if (exp->ex_flags & NFSEXP_SECURITY_LABEL)
> err = security_inode_getsecctx(d_inode(dentry),
> - &context, &contextlen);
> + &context);
> else
> err = -EOPNOTSUPP;
> contextsupport = (err == 0);
> @@ -2908,8 +2906,7 @@ nfsd4_encode_fattr(struct xdr_stream *xdr, struct svc_fh *fhp,
> }
>
> if (bmval2 & FATTR4_WORD2_SECURITY_LABEL) {
> - status = nfsd4_encode_security_label(xdr, rqstp, context,
> - contextlen);
> + status = nfsd4_encode_security_label(xdr, rqstp, &context);
> if (status)
> goto out;
> }
> @@ -2920,10 +2917,8 @@ nfsd4_encode_fattr(struct xdr_stream *xdr, struct svc_fh *fhp,
>
> out:
> #ifdef CONFIG_NFSD_V4_SECURITY_LABEL
> - if (context) {
> - lsmcontext_init(&scaff, context, contextlen, 0); /*scaffolding*/
> - security_release_secctx(&scaff);
> - }
> + if (context.context)
> + security_release_secctx(&context);
> #endif /* CONFIG_NFSD_V4_SECURITY_LABEL */
> kfree(acl);
> if (tempfh) {
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 2a2785a4e752..bfdb06bc5466 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -485,7 +485,7 @@ void security_release_secctx(struct lsmcontext *cp);
> void security_inode_invalidate_secctx(struct inode *inode);
> int security_inode_notifysecctx(struct inode *inode, void *ctx, u32 ctxlen);
> int security_inode_setsecctx(struct dentry *dentry, void *ctx, u32 ctxlen);
> -int security_inode_getsecctx(struct inode *inode, void **ctx, u32 *ctxlen);
> +int security_inode_getsecctx(struct inode *inode, struct lsmcontext *cp);
> #else /* CONFIG_SECURITY */
>
> static inline int call_lsm_notifier(enum lsm_event event, void *data)
> @@ -1286,7 +1286,8 @@ static inline int security_inode_setsecctx(struct dentry *dentry, void *ctx, u32
> {
> return -EOPNOTSUPP;
> }
> -static inline int security_inode_getsecctx(struct inode *inode, void **ctx, u32 *ctxlen)
> +static inline int security_inode_getsecctx(struct inode *inode,
> + struct lsmcontext *cp)
> {
> return -EOPNOTSUPP;
> }
> diff --git a/security/security.c b/security/security.c
> index 842ac65abc08..b2ffcd1f3057 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -2139,9 +2139,18 @@ int security_inode_setsecctx(struct dentry *dentry, void *ctx, u32 ctxlen)
> }
> EXPORT_SYMBOL(security_inode_setsecctx);
>
> -int security_inode_getsecctx(struct inode *inode, void **ctx, u32 *ctxlen)
> +int security_inode_getsecctx(struct inode *inode, struct lsmcontext *cp)
> {
> - return call_int_hook(inode_getsecctx, -EOPNOTSUPP, inode, ctx, ctxlen);
> + int *display = current->security;
> + struct security_hook_list *hp;
> +
> + hlist_for_each_entry(hp, &security_hook_heads.inode_getsecctx, list)
> + if (*display == 0 || *display == hp->slot) {
should be
if (*display == LSMBLOB_INVALID || *display == hp->slot)
> + cp->slot = hp->slot;
> + return hp->hook.inode_getsecctx(inode,
> + (void **)&cp->context, &cp->len);
> + }
> + return -EOPNOTSUPP;
> }
> EXPORT_SYMBOL(security_inode_getsecctx);
>
>
^ permalink raw reply
* [PATCH v3 18/24] LSM: Use lsmcontext in security_dentry_init_security
From: John Johansen @ 2019-06-24 21:46 UTC (permalink / raw)
To: Casey Schaufler, casey.schaufler, jmorris, linux-security-module,
selinux
Cc: keescook, penguin-kernel, paul, sds
In-Reply-To: <20190621185233.6766-19-casey@schaufler-ca.com>
On 6/21/19 11:52 AM, Casey Schaufler wrote:
> Change the security_dentry_init_security() interface to
> fill an lsmcontext structure instead of a void * data area
> and a length. The lone caller of this interface is NFS4,
> which may make copies of the data using its own mechanisms.
> A rework of the nfs4 code to use the lsmcontext properly
> is a significant project, so the coward's way out is taken,
> and the lsmcontext data from security_dentry_init_security()
> is copied, then released directly.
>
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
With the needed fix (below) that Kees already pointed out
Reviewed-by: John Johansen <john.johansen@canonical.com>
> ---
> fs/nfs/nfs4proc.c | 26 ++++++++++++++++----------
> include/linux/security.h | 7 +++----
> security/security.c | 20 ++++++++++++++++----
> 3 files changed, 35 insertions(+), 18 deletions(-)
>
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index af1c0db29c39..952f805965bb 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -113,6 +113,7 @@ static inline struct nfs4_label *
> nfs4_label_init_security(struct inode *dir, struct dentry *dentry,
> struct iattr *sattr, struct nfs4_label *label)
> {
> + struct lsmcontext context;
> int err;
>
> if (label == NULL)
> @@ -122,21 +123,26 @@ nfs4_label_init_security(struct inode *dir, struct dentry *dentry,
> return NULL;
>
> err = security_dentry_init_security(dentry, sattr->ia_mode,
> - &dentry->d_name, (void **)&label->label, &label->len);
> - if (err == 0)
> - return label;
> + &dentry->d_name, &context);
> +
> + if (err)
> + return NULL;
> +
> + label->label = kmemdup(context.context, context.len, GFP_KERNEL);
> + if (label->label == NULL)
> + label = NULL;
> + else
> + label->len = context.len;
> +
> + security_release_secctx(&context);
> +
> + return label;
>
> - return NULL;
> }
> static inline void
> nfs4_label_release_security(struct nfs4_label *label)
> {
> - struct lsmcontext scaff; /* scaffolding */
> -
> - if (label) {
> - lsmcontext_init(&scaff, label->label, label->len, 0);
> - security_release_secctx(&scaff);
> - }
> + kfree(label->label);
> }
> static inline u32 *nfs4_bitmask(struct nfs_server *server, struct nfs4_label *label)
> {
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 3cbe43db16f5..2a2785a4e752 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -348,8 +348,8 @@ int security_sb_clone_mnt_opts(const struct super_block *oldsb,
> int security_add_mnt_opt(const char *option, const char *val,
> int len, void **mnt_opts);
> int security_dentry_init_security(struct dentry *dentry, int mode,
> - const struct qstr *name, void **ctx,
> - u32 *ctxlen);
> + const struct qstr *name,
> + struct lsmcontext *ctx);
> int security_dentry_create_files_as(struct dentry *dentry, int mode,
> struct qstr *name,
> const struct cred *old,
> @@ -720,8 +720,7 @@ static inline void security_inode_free(struct inode *inode)
> static inline int security_dentry_init_security(struct dentry *dentry,
> int mode,
> const struct qstr *name,
> - void **ctx,
> - u32 *ctxlen)
> + struct lsmcontext *ctx)
> {
> return -EOPNOTSUPP;
> }
> diff --git a/security/security.c b/security/security.c
> index f461ab3fb9c4..842ac65abc08 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -453,6 +453,8 @@ void __init security_add_hooks(struct security_hook_list *hooks, int count,
> * secid in the lsmblob structure.
> */
> if (hooks[i].head == &security_hook_heads.audit_rule_match ||
> + hooks[i].head ==
> + &security_hook_heads.dentry_init_security ||
> hooks[i].head == &security_hook_heads.kernel_act_as ||
> hooks[i].head ==
> &security_hook_heads.socket_getpeersec_dgram ||
> @@ -1040,11 +1042,21 @@ void security_inode_free(struct inode *inode)
> }
>
> int security_dentry_init_security(struct dentry *dentry, int mode,
> - const struct qstr *name, void **ctx,
> - u32 *ctxlen)
> + const struct qstr *name,
> + struct lsmcontext *cp)
> {
> - return call_int_hook(dentry_init_security, -EOPNOTSUPP, dentry, mode,
> - name, ctx, ctxlen);
> + int *display = current->security;
> + struct security_hook_list *hp;
> +
> + hlist_for_each_entry(hp, &security_hook_heads.dentry_init_security,
> + list)
> + if (*display == 0 || *display == hp->slot) {
> + cp->slot = hp->slot;
> + return hp->hook.dentry_init_security(dentry, mode,
> + name, (void **)&cp->context, &cp->len);
> + }
> +
should be
if (*display == LSMBLOB_INVALID || *display == hp->slot)
> + return -EOPNOTSUPP;
> }
> EXPORT_SYMBOL(security_dentry_init_security);
>
>
^ permalink raw reply
* [PATCH v3 17/24] LSM: Use lsmcontext in security_secid_to_secctx
From: John Johansen @ 2019-06-24 21:46 UTC (permalink / raw)
To: Casey Schaufler, casey.schaufler, jmorris, linux-security-module,
selinux
Cc: keescook, penguin-kernel, paul, sds
In-Reply-To: <20190621185233.6766-18-casey@schaufler-ca.com>
On 6/21/19 11:52 AM, Casey Schaufler wrote:
> Replace the (secctx,seclen) pointer pair with a single
> lsmcontext pointer to allow return of the LSM identifier
> along with the context and context length. This allows
> security_release_secctx() to know how to release the
> context. Callers have been modified to use or save the
> returned data from the new structure.
>
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
Reviewed-by: John Johansen <john.johansen@canonical.com>
> ---
> drivers/android/binder.c | 24 ++++++---------
> include/linux/security.h | 4 +--
> include/net/scm.h | 9 ++----
> kernel/audit.c | 29 +++++++-----------
> kernel/auditsc.c | 31 +++++++------------
> net/ipv4/ip_sockglue.c | 7 ++---
> net/netfilter/nf_conntrack_netlink.c | 14 +++++----
> net/netfilter/nf_conntrack_standalone.c | 7 ++---
> net/netfilter/nfnetlink_queue.c | 5 +++-
> net/netlabel/netlabel_unlabeled.c | 40 ++++++++-----------------
> net/netlabel/netlabel_user.c | 7 ++---
> security/security.c | 9 +++---
> 12 files changed, 71 insertions(+), 115 deletions(-)
>
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index 89e574be34cc..5d417a7b9bb3 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -2874,9 +2874,7 @@ static void binder_transaction(struct binder_proc *proc,
> binder_size_t last_fixup_min_off = 0;
> struct binder_context *context = proc->context;
> int t_debug_id = atomic_inc_return(&binder_last_id);
> - char *secctx = NULL;
> - u32 secctx_sz = 0;
> - struct lsmcontext scaff; /* scaffolding */
> + struct lsmcontext lsmctx;
>
> e = binder_transaction_log_add(&binder_transaction_log);
> e->debug_id = t_debug_id;
> @@ -3123,14 +3121,14 @@ static void binder_transaction(struct binder_proc *proc,
> struct lsmblob blob;
>
> security_task_getsecid(proc->tsk, &blob);
> - ret = security_secid_to_secctx(&blob, &secctx, &secctx_sz);
> + ret = security_secid_to_secctx(&blob, &lsmctx);
> if (ret) {
> return_error = BR_FAILED_REPLY;
> return_error_param = ret;
> return_error_line = __LINE__;
> goto err_get_secctx_failed;
> }
> - extra_buffers_size += ALIGN(secctx_sz, sizeof(u64));
> + extra_buffers_size += ALIGN(lsmctx.len, sizeof(u64));
> }
>
> trace_binder_transaction(reply, t, target_node);
> @@ -3149,19 +3147,17 @@ static void binder_transaction(struct binder_proc *proc,
> t->buffer = NULL;
> goto err_binder_alloc_buf_failed;
> }
> - if (secctx) {
> + if (lsmctx.context) {
> size_t buf_offset = ALIGN(tr->data_size, sizeof(void *)) +
> ALIGN(tr->offsets_size, sizeof(void *)) +
> ALIGN(extra_buffers_size, sizeof(void *)) -
> - ALIGN(secctx_sz, sizeof(u64));
> + ALIGN(lsmctx.len, sizeof(u64));
>
> t->security_ctx = (uintptr_t)t->buffer->user_data + buf_offset;
> binder_alloc_copy_to_buffer(&target_proc->alloc,
> t->buffer, buf_offset,
> - secctx, secctx_sz);
> - lsmcontext_init(&scaff, secctx, secctx_sz, 0);
> - security_release_secctx(&scaff);
> - secctx = NULL;
> + lsmctx.context, lsmctx.len);
> + security_release_secctx(&lsmctx);
> }
> t->buffer->debug_id = t->debug_id;
> t->buffer->transaction = t;
> @@ -3481,10 +3477,8 @@ static void binder_transaction(struct binder_proc *proc,
> t->buffer->transaction = NULL;
> binder_alloc_free_buf(&target_proc->alloc, t->buffer);
> err_binder_alloc_buf_failed:
> - if (secctx) {
> - lsmcontext_init(&scaff, secctx, secctx_sz, 0);
> - security_release_secctx(&scaff);
> - }
> + if (lsmctx.context)
> + security_release_secctx(&lsmctx);
> err_get_secctx_failed:
> kfree(tcomplete);
> binder_stats_deleted(BINDER_STAT_TRANSACTION_COMPLETE);
> diff --git a/include/linux/security.h b/include/linux/security.h
> index ddf6d7cb23f1..3cbe43db16f5 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -477,7 +477,7 @@ int security_setprocattr(const char *lsm, const char *name, void *value,
> size_t size);
> int security_netlink_send(struct sock *sk, struct sk_buff *skb);
> int security_ismaclabel(const char *name);
> -int security_secid_to_secctx(struct lsmblob *blob, char **secdata, u32 *seclen);
> +int security_secid_to_secctx(struct lsmblob *blob, struct lsmcontext *cp);
> int security_secctx_to_secid(const char *secdata, u32 seclen,
> struct lsmblob *blob);
> void security_release_secctx(struct lsmcontext *cp);
> @@ -1259,7 +1259,7 @@ static inline int security_ismaclabel(const char *name)
> }
>
> static inline int security_secid_to_secctx(struct lsmblob *blob,
> - char **secdata, u32 *seclen)
> + struct lsmcontext *cp)
> {
> return -EOPNOTSUPP;
> }
> diff --git a/include/net/scm.h b/include/net/scm.h
> index 6c7c3c229e4a..4a6ad8caf423 100644
> --- a/include/net/scm.h
> +++ b/include/net/scm.h
> @@ -93,17 +93,14 @@ static __inline__ int scm_send(struct socket *sock, struct msghdr *msg,
> static inline void scm_passec(struct socket *sock, struct msghdr *msg, struct scm_cookie *scm)
> {
> struct lsmcontext context;
> - char *secdata;
> - u32 seclen;
> int err;
>
> if (test_bit(SOCK_PASSSEC, &sock->flags)) {
> - err = security_secid_to_secctx(&scm->lsmblob, &secdata,
> - &seclen);
> + err = security_secid_to_secctx(&scm->lsmblob, &context);
>
> if (!err) {
> - put_cmsg(msg, SOL_SOCKET, SCM_SECURITY, seclen, secdata);
> - lsmcontext_init(&context, secdata, seclen, 0);/*scaffolding*/
> + put_cmsg(msg, SOL_SOCKET, SCM_SECURITY,
> + context.len, context.context);
> security_release_secctx(&context);
> }
> }
> diff --git a/kernel/audit.c b/kernel/audit.c
> index f844a2a642e6..436c23429319 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -1191,9 +1191,8 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
> struct audit_buffer *ab;
> u16 msg_type = nlh->nlmsg_type;
> struct audit_sig_info *sig_data;
> - char *ctx = NULL;
> u32 len;
> - struct lsmcontext scaff; /* scaffolding */
> + struct lsmcontext context;
>
> err = audit_netlink_ok(skb, msg_type);
> if (err)
> @@ -1431,25 +1430,22 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
> case AUDIT_SIGNAL_INFO:
> len = 0;
> if (lsmblob_is_set(&audit_sig_lsm)) {
> - err = security_secid_to_secctx(&audit_sig_lsm, &ctx,
> - &len);
> + err = security_secid_to_secctx(&audit_sig_lsm,
> + &context);
> if (err)
> return err;
> }
> sig_data = kmalloc(sizeof(*sig_data) + len, GFP_KERNEL);
> if (!sig_data) {
> - if (lsmblob_is_set(&audit_sig_lsm)) {
> - lsmcontext_init(&scaff, ctx, len, 0);
> - security_release_secctx(&scaff);
> - }
> + if (lsmblob_is_set(&audit_sig_lsm))
> + security_release_secctx(&context);
> return -ENOMEM;
> }
> sig_data->uid = from_kuid(&init_user_ns, audit_sig_uid);
> sig_data->pid = audit_sig_pid;
> if (lsmblob_is_set(&audit_sig_lsm)) {
> - memcpy(sig_data->ctx, ctx, len);
> - lsmcontext_init(&scaff, ctx, len, 0);
> - security_release_secctx(&scaff);
> + memcpy(sig_data->ctx, context.context, context.len);
> + security_release_secctx(&context);
> }
> audit_send_reply(skb, seq, AUDIT_SIGNAL_INFO, 0, 0,
> sig_data, sizeof(*sig_data) + len);
> @@ -2074,26 +2070,23 @@ void audit_log_key(struct audit_buffer *ab, char *key)
>
> int audit_log_task_context(struct audit_buffer *ab)
> {
> - char *ctx = NULL;
> - unsigned len;
> int error;
> struct lsmblob blob;
> - struct lsmcontext scaff; /* scaffolding */
> + struct lsmcontext context;
>
> security_task_getsecid(current, &blob);
> if (!lsmblob_is_set(&blob))
> return 0;
>
> - error = security_secid_to_secctx(&blob, &ctx, &len);
> + error = security_secid_to_secctx(&blob, &context);
> if (error) {
> if (error != -EINVAL)
> goto error_path;
> return 0;
> }
>
> - audit_log_format(ab, " subj=%s", ctx);
> - lsmcontext_init(&scaff, ctx, len, 0);
> - security_release_secctx(&scaff);
> + audit_log_format(ab, " subj=%s", context.context);
> + security_release_secctx(&context);
> return 0;
>
> error_path:
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 9fab0e7d90c3..0478680cd0a8 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -943,9 +943,7 @@ static int audit_log_pid_context(struct audit_context *context, pid_t pid,
> struct lsmblob *blob, char *comm)
> {
> struct audit_buffer *ab;
> - struct lsmcontext lsmcxt;
> - char *ctx = NULL;
> - u32 len;
> + struct lsmcontext lsmctx;
> int rc = 0;
>
> ab = audit_log_start(context, GFP_KERNEL, AUDIT_OBJ_PID);
> @@ -956,13 +954,12 @@ static int audit_log_pid_context(struct audit_context *context, pid_t pid,
> from_kuid(&init_user_ns, auid),
> from_kuid(&init_user_ns, uid), sessionid);
> if (lsmblob_is_set(blob)) {
> - if (security_secid_to_secctx(blob, &ctx, &len)) {
> + if (security_secid_to_secctx(blob, &lsmctx)) {
> audit_log_format(ab, " obj=(none)");
> rc = 1;
> } else {
> - audit_log_format(ab, " obj=%s", ctx);
> - lsmcontext_init(&lsmcxt, ctx, len, 0); /*scaffolding*/
> - security_release_secctx(&lsmcxt);
> + audit_log_format(ab, " obj=%s", lsmctx.context);
> + security_release_secctx(&lsmctx);
> }
> }
> audit_log_format(ab, " ocomm=");
> @@ -1174,7 +1171,6 @@ static void audit_log_fcaps(struct audit_buffer *ab, struct audit_names *name)
>
> static void show_special(struct audit_context *context, int *call_panic)
> {
> - struct lsmcontext lsmcxt;
> struct audit_buffer *ab;
> int i;
>
> @@ -1198,17 +1194,15 @@ static void show_special(struct audit_context *context, int *call_panic)
> from_kgid(&init_user_ns, context->ipc.gid),
> context->ipc.mode);
> if (osid) {
> - char *ctx = NULL;
> - u32 len;
> + struct lsmcontext lsmcxt;
> struct lsmblob blob;
>
> lsmblob_init(&blob, osid);
> - if (security_secid_to_secctx(&blob, &ctx, &len)) {
> + if (security_secid_to_secctx(&blob, &lsmcxt)) {
> audit_log_format(ab, " osid=%u", osid);
> *call_panic = 1;
> } else {
> - audit_log_format(ab, " obj=%s", ctx);
> - lsmcontext_init(&lsmcxt, ctx, len, 0);
> + audit_log_format(ab, " obj=%s", lsmcxt.context);
> security_release_secctx(&lsmcxt);
> }
> }
> @@ -1353,20 +1347,17 @@ static void audit_log_name(struct audit_context *context, struct audit_names *n,
> MAJOR(n->rdev),
> MINOR(n->rdev));
> if (n->osid != 0) {
> - char *ctx = NULL;
> - u32 len;
> struct lsmblob blob;
> - struct lsmcontext lsmcxt;
> + struct lsmcontext lsmctx;
>
> lsmblob_init(&blob, n->osid);
> - if (security_secid_to_secctx(&blob, &ctx, &len)) {
> + if (security_secid_to_secctx(&blob, &lsmctx)) {
> audit_log_format(ab, " osid=%u", n->osid);
> if (call_panic)
> *call_panic = 2;
> } else {
> - audit_log_format(ab, " obj=%s", ctx);
> - lsmcontext_init(&lsmcxt, ctx, len, 0); /* scaffolding */
> - security_release_secctx(&lsmcxt);
> + audit_log_format(ab, " obj=%s", lsmctx.context);
> + security_release_secctx(&lsmctx);
> }
> }
>
> diff --git a/net/ipv4/ip_sockglue.c b/net/ipv4/ip_sockglue.c
> index 7834c357b60b..80ae0c5a1301 100644
> --- a/net/ipv4/ip_sockglue.c
> +++ b/net/ipv4/ip_sockglue.c
> @@ -132,20 +132,17 @@ static void ip_cmsg_recv_security(struct msghdr *msg, struct sk_buff *skb)
> {
> struct lsmcontext context;
> struct lsmblob lb;
> - char *secdata;
> - u32 seclen;
> int err;
>
> err = security_socket_getpeersec_dgram(NULL, skb, &lb);
> if (err)
> return;
>
> - err = security_secid_to_secctx(&lb, &secdata, &seclen);
> + err = security_secid_to_secctx(&lb, &context);
> if (err)
> return;
>
> - put_cmsg(msg, SOL_IP, SCM_SECURITY, seclen, secdata);
> - lsmcontext_init(&context, secdata, seclen, 0); /* scaffolding */
> + put_cmsg(msg, SOL_IP, SCM_SECURITY, context.len, context.context);
> security_release_secctx(&context);
> }
>
> diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
> index 6954e6600583..403307ff0fff 100644
> --- a/net/netfilter/nf_conntrack_netlink.c
> +++ b/net/netfilter/nf_conntrack_netlink.c
> @@ -328,13 +328,12 @@ static int ctnetlink_dump_mark(struct sk_buff *skb, const struct nf_conn *ct)
> static int ctnetlink_dump_secctx(struct sk_buff *skb, const struct nf_conn *ct)
> {
> struct nlattr *nest_secctx;
> - int len, ret;
> - char *secctx;
> + int ret;
> struct lsmblob blob;
> struct lsmcontext context;
>
> lsmblob_init(&blob, ct->secmark);
> - ret = security_secid_to_secctx(&blob, &secctx, &len);
> + ret = security_secid_to_secctx(&blob, &context);
> if (ret)
> return 0;
>
> @@ -343,13 +342,12 @@ static int ctnetlink_dump_secctx(struct sk_buff *skb, const struct nf_conn *ct)
> if (!nest_secctx)
> goto nla_put_failure;
>
> - if (nla_put_string(skb, CTA_SECCTX_NAME, secctx))
> + if (nla_put_string(skb, CTA_SECCTX_NAME, context.context))
> goto nla_put_failure;
> nla_nest_end(skb, nest_secctx);
>
> ret = 0;
> nla_put_failure:
> - lsmcontext_init(&context, secctx, len, 0); /* scaffolding */
> security_release_secctx(&context);
> return ret;
> }
> @@ -620,12 +618,16 @@ static inline int ctnetlink_secctx_size(const struct nf_conn *ct)
> #ifdef CONFIG_NF_CONNTRACK_SECMARK
> int len, ret;
> struct lsmblob blob;
> + struct lsmcontext context;
>
> lsmblob_init(&blob, ct->secmark);
> - ret = security_secid_to_secctx(&blob, NULL, &len);
> + ret = security_secid_to_secctx(&blob, &context);
> if (ret)
> return 0;
>
> + len = context.len;
> + security_release_secctx(&context);
> +
> return nla_total_size(0) /* CTA_SECCTX */
> + nla_total_size(sizeof(char) * len); /* CTA_SECCTX_NAME */
> #else
> diff --git a/net/netfilter/nf_conntrack_standalone.c b/net/netfilter/nf_conntrack_standalone.c
> index 79158ad0486e..fcb51ab2bb8b 100644
> --- a/net/netfilter/nf_conntrack_standalone.c
> +++ b/net/netfilter/nf_conntrack_standalone.c
> @@ -173,19 +173,16 @@ static void ct_seq_stop(struct seq_file *s, void *v)
> static void ct_show_secctx(struct seq_file *s, const struct nf_conn *ct)
> {
> int ret;
> - u32 len;
> - char *secctx;
> struct lsmblob blob;
> struct lsmcontext context;
>
> lsmblob_init(&blob, ct->secmark);
> - ret = security_secid_to_secctx(&blob, &secctx, &len);
> + ret = security_secid_to_secctx(&blob, &context);
> if (ret)
> return;
>
> - seq_printf(s, "secctx=%s ", secctx);
> + seq_printf(s, "secctx=%s ", context.context);
>
> - lsmcontext_init(&context, secctx, len, 0); /* scaffolding */
> security_release_secctx(&context);
> }
> #else
> diff --git a/net/netfilter/nfnetlink_queue.c b/net/netfilter/nfnetlink_queue.c
> index fe8403ef4e89..6da00c7add5b 100644
> --- a/net/netfilter/nfnetlink_queue.c
> +++ b/net/netfilter/nfnetlink_queue.c
> @@ -310,6 +310,7 @@ static u32 nfqnl_get_sk_secctx(struct sk_buff *skb, char **secdata)
> u32 seclen = 0;
> #if IS_ENABLED(CONFIG_NETWORK_SECMARK)
> struct lsmblob blob;
> + struct lsmcontext context;
>
> if (!skb || !sk_fullsock(skb->sk))
> return 0;
> @@ -318,10 +319,12 @@ static u32 nfqnl_get_sk_secctx(struct sk_buff *skb, char **secdata)
>
> if (skb->secmark) {
> lsmblob_init(&blob, skb->secmark);
> - security_secid_to_secctx(&blob, secdata, &seclen);
> + security_secid_to_secctx(&blob, &context);
> + *secdata = context.context;
> }
>
> read_unlock_bh(&skb->sk->sk_callback_lock);
> + seclen = context.len;
> #endif
> return seclen;
> }
> diff --git a/net/netlabel/netlabel_unlabeled.c b/net/netlabel/netlabel_unlabeled.c
> index 15b1945853be..4716e0011ba5 100644
> --- a/net/netlabel/netlabel_unlabeled.c
> +++ b/net/netlabel/netlabel_unlabeled.c
> @@ -388,8 +388,6 @@ int netlbl_unlhsh_add(struct net *net,
> struct netlbl_unlhsh_iface *iface;
> struct audit_buffer *audit_buf = NULL;
> struct lsmcontext context;
> - char *secctx = NULL;
> - u32 secctx_len;
> struct lsmblob blob;
>
> if (addr_len != sizeof(struct in_addr) &&
> @@ -454,12 +452,9 @@ int netlbl_unlhsh_add(struct net *net,
> rcu_read_unlock();
> if (audit_buf != NULL) {
> lsmblob_init(&blob, secid);
> - if (security_secid_to_secctx(&blob,
> - &secctx,
> - &secctx_len) == 0) {
> - audit_log_format(audit_buf, " sec_obj=%s", secctx);
> - /* scaffolding */
> - lsmcontext_init(&context, secctx, secctx_len, 0);
> + if (security_secid_to_secctx(&blob, &context) == 0) {
> + audit_log_format(audit_buf, " sec_obj=%s",
> + context.context);
> security_release_secctx(&context);
> }
> audit_log_format(audit_buf, " res=%u", ret_val == 0 ? 1 : 0);
> @@ -492,8 +487,6 @@ static int netlbl_unlhsh_remove_addr4(struct net *net,
> struct audit_buffer *audit_buf;
> struct net_device *dev;
> struct lsmcontext context;
> - char *secctx;
> - u32 secctx_len;
> struct lsmblob blob;
>
> spin_lock(&netlbl_unlhsh_lock);
> @@ -517,11 +510,9 @@ static int netlbl_unlhsh_remove_addr4(struct net *net,
> if (entry != NULL)
> lsmblob_init(&blob, entry->secid);
> if (entry != NULL &&
> - security_secid_to_secctx(&blob,
> - &secctx, &secctx_len) == 0) {
> - audit_log_format(audit_buf, " sec_obj=%s", secctx);
> - /* scaffolding */
> - lsmcontext_init(&context, secctx, secctx_len, 0);
> + security_secid_to_secctx(&blob, &context) == 0) {
> + audit_log_format(audit_buf, " sec_obj=%s",
> + context.context);
> security_release_secctx(&context);
> }
> audit_log_format(audit_buf, " res=%u", entry != NULL ? 1 : 0);
> @@ -560,8 +551,6 @@ static int netlbl_unlhsh_remove_addr6(struct net *net,
> struct audit_buffer *audit_buf;
> struct net_device *dev;
> struct lsmcontext context;
> - char *secctx;
> - u32 secctx_len;
> struct lsmblob blob;
>
> spin_lock(&netlbl_unlhsh_lock);
> @@ -584,10 +573,9 @@ static int netlbl_unlhsh_remove_addr6(struct net *net,
> if (entry != NULL)
> lsmblob_init(&blob, entry->secid);
> if (entry != NULL &&
> - security_secid_to_secctx(&blob,
> - &secctx, &secctx_len) == 0) {
> - audit_log_format(audit_buf, " sec_obj=%s", secctx);
> - lsmcontext_init(&context, secctx, secctx_len, 0);
> + security_secid_to_secctx(&blob, &context) == 0) {
> + audit_log_format(audit_buf, " sec_obj=%s",
> + context.context);
> security_release_secctx(&context);
> }
> audit_log_format(audit_buf, " res=%u", entry != NULL ? 1 : 0);
> @@ -1105,8 +1093,6 @@ static int netlbl_unlabel_staticlist_gen(u32 cmd,
> struct lsmcontext context;
> void *data;
> u32 secid;
> - char *secctx;
> - u32 secctx_len;
> struct lsmblob blob;
>
> data = genlmsg_put(cb_arg->skb, NETLINK_CB(cb_arg->nl_cb->skb).portid,
> @@ -1163,15 +1149,13 @@ static int netlbl_unlabel_staticlist_gen(u32 cmd,
> }
>
> lsmblob_init(&blob, secid);
> - ret_val = security_secid_to_secctx(&blob, &secctx, &secctx_len);
> + ret_val = security_secid_to_secctx(&blob, &context);
> if (ret_val != 0)
> goto list_cb_failure;
> ret_val = nla_put(cb_arg->skb,
> NLBL_UNLABEL_A_SECCTX,
> - secctx_len,
> - secctx);
> - /* scaffolding */
> - lsmcontext_init(&context, secctx, secctx_len, 0);
> + context.len,
> + context.context);
> security_release_secctx(&context);
> if (ret_val != 0)
> goto list_cb_failure;
> diff --git a/net/netlabel/netlabel_user.c b/net/netlabel/netlabel_user.c
> index 94aea4985b74..2d1307f65250 100644
> --- a/net/netlabel/netlabel_user.c
> +++ b/net/netlabel/netlabel_user.c
> @@ -99,8 +99,6 @@ struct audit_buffer *netlbl_audit_start_common(int type,
> {
> struct audit_buffer *audit_buf;
> struct lsmcontext context;
> - char *secctx;
> - u32 secctx_len;
> struct lsmblob blob;
>
> if (audit_enabled == AUDIT_OFF)
> @@ -116,9 +114,8 @@ struct audit_buffer *netlbl_audit_start_common(int type,
>
> lsmblob_init(&blob, audit_info->secid);
> if (audit_info->secid != 0 &&
> - security_secid_to_secctx(&blob, &secctx, &secctx_len) == 0) {
> - audit_log_format(audit_buf, " subj=%s", secctx);
> - lsmcontext_init(&context, secctx, secctx_len, 0);/*scaffolding*/
> + security_secid_to_secctx(&blob, &context) == 0) {
> + audit_log_format(audit_buf, " subj=%s", context.context);
> security_release_secctx(&context);
> }
>
> diff --git a/security/security.c b/security/security.c
> index d5f173e85393..f461ab3fb9c4 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -459,7 +459,6 @@ void __init security_add_hooks(struct security_hook_list *hooks, int count,
> hooks[i].head == &security_hook_heads.getprocattr ||
> hooks[i].head == &security_hook_heads.setprocattr ||
> hooks[i].head == &security_hook_heads.secctx_to_secid ||
> - hooks[i].head == &security_hook_heads.secid_to_secctx ||
> hooks[i].head == &security_hook_heads.release_secctx ||
> hooks[i].head == &security_hook_heads.ipc_getsecid ||
> hooks[i].head == &security_hook_heads.task_getsecid ||
> @@ -2064,15 +2063,17 @@ int security_ismaclabel(const char *name)
> }
> EXPORT_SYMBOL(security_ismaclabel);
>
> -int security_secid_to_secctx(struct lsmblob *blob, char **secdata, u32 *seclen)
> +int security_secid_to_secctx(struct lsmblob *blob, struct lsmcontext *cp)
> {
> struct security_hook_list *hp;
> int *display = current->security;
>
> hlist_for_each_entry(hp, &security_hook_heads.secid_to_secctx, list)
> - if (*display == LSMBLOB_INVALID || *display == hp->slot)
> + if (*display == LSMBLOB_INVALID || *display == hp->slot) {
> + cp->slot = hp->slot;
> return hp->hook.secid_to_secctx(blob->secid[hp->slot],
> - secdata, seclen);
> + &cp->context, &cp->len);
> + }
> return 0;
> }
> EXPORT_SYMBOL(security_secid_to_secctx);
>
^ permalink raw reply
* [PATCH v3 16/24] LSM: Ensure the correct LSM context releaser
From: John Johansen @ 2019-06-24 21:46 UTC (permalink / raw)
To: Casey Schaufler, casey.schaufler, jmorris, linux-security-module,
selinux
Cc: keescook, penguin-kernel, paul, sds
In-Reply-To: <20190621185233.6766-17-casey@schaufler-ca.com>
On 6/21/19 11:52 AM, Casey Schaufler wrote:
> Add a new lsmcontext data structure to hold all the information
> about a "security context", including the string, its size and
> which LSM allocated the string. The allocation information is
> necessary because LSMs have different policies regarding the
> lifecycle of these strings. SELinux allocates and destroys
> them on each use, whereas Smack provides a pointer to an entry
> in a list that never goes away.
>
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
Reviewed-by: John Johansen <john.johansen@canonical.com>
> ---
> drivers/android/binder.c | 10 +++++--
> fs/kernfs/dir.c | 9 ++++--
> fs/kernfs/inode.c | 7 +++--
> fs/nfs/nfs4proc.c | 8 +++--
> fs/nfsd/nfs4xdr.c | 7 +++--
> include/linux/security.h | 39 +++++++++++++++++++++++--
> include/net/scm.h | 4 ++-
> kernel/audit.c | 14 ++++++---
> kernel/auditsc.c | 12 ++++++--
> net/ipv4/ip_sockglue.c | 4 ++-
> net/netfilter/nf_conntrack_netlink.c | 4 ++-
> net/netfilter/nf_conntrack_standalone.c | 4 ++-
> net/netfilter/nfnetlink_queue.c | 13 ++++++---
> net/netlabel/netlabel_unlabeled.c | 19 +++++++++---
> net/netlabel/netlabel_user.c | 4 ++-
> security/security.c | 12 +++++---
> security/smack/smack_lsm.c | 14 ++++++---
> 17 files changed, 142 insertions(+), 42 deletions(-)
>
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index 144ac4f1c24f..89e574be34cc 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -2876,6 +2876,7 @@ static void binder_transaction(struct binder_proc *proc,
> int t_debug_id = atomic_inc_return(&binder_last_id);
> char *secctx = NULL;
> u32 secctx_sz = 0;
> + struct lsmcontext scaff; /* scaffolding */
>
> e = binder_transaction_log_add(&binder_transaction_log);
> e->debug_id = t_debug_id;
> @@ -3158,7 +3159,8 @@ static void binder_transaction(struct binder_proc *proc,
> binder_alloc_copy_to_buffer(&target_proc->alloc,
> t->buffer, buf_offset,
> secctx, secctx_sz);
> - security_release_secctx(secctx, secctx_sz);
> + lsmcontext_init(&scaff, secctx, secctx_sz, 0);
> + security_release_secctx(&scaff);
> secctx = NULL;
> }
> t->buffer->debug_id = t->debug_id;
> @@ -3479,8 +3481,10 @@ static void binder_transaction(struct binder_proc *proc,
> t->buffer->transaction = NULL;
> binder_alloc_free_buf(&target_proc->alloc, t->buffer);
> err_binder_alloc_buf_failed:
> - if (secctx)
> - security_release_secctx(secctx, secctx_sz);
> + if (secctx) {
> + lsmcontext_init(&scaff, secctx, secctx_sz, 0);
> + security_release_secctx(&scaff);
> + }
> err_get_secctx_failed:
> kfree(tcomplete);
> binder_stats_deleted(BINDER_STAT_TRANSACTION_COMPLETE);
> diff --git a/fs/kernfs/dir.c b/fs/kernfs/dir.c
> index b84d635567d3..92afad387237 100644
> --- a/fs/kernfs/dir.c
> +++ b/fs/kernfs/dir.c
> @@ -532,9 +532,12 @@ void kernfs_put(struct kernfs_node *kn)
> kfree_const(kn->name);
>
> if (kn->iattr) {
> - if (kn->iattr->ia_secdata)
> - security_release_secctx(kn->iattr->ia_secdata,
> - kn->iattr->ia_secdata_len);
> + struct lsmcontext scaff; /* scaffolding */
> + if (kn->iattr->ia_secdata) {
> + lsmcontext_init(&scaff, kn->iattr->ia_secdata,
> + kn->iattr->ia_secdata_len, 0);
> + security_release_secctx(&scaff);
> + }
> simple_xattrs_free(&kn->iattr->xattrs);
> kmem_cache_free(kernfs_iattrs_cache, kn->iattr);
> }
> diff --git a/fs/kernfs/inode.c b/fs/kernfs/inode.c
> index 0c1fd945ce42..02cde9dac5ee 100644
> --- a/fs/kernfs/inode.c
> +++ b/fs/kernfs/inode.c
> @@ -349,6 +349,7 @@ static int kernfs_security_xattr_set(const struct xattr_handler *handler,
> {
> struct kernfs_node *kn = inode->i_private;
> struct kernfs_iattrs *attrs;
> + struct lsmcontext context;
> void *secdata;
> u32 secdata_len = 0;
> int error;
> @@ -368,8 +369,10 @@ static int kernfs_security_xattr_set(const struct xattr_handler *handler,
> error = kernfs_node_setsecdata(attrs, &secdata, &secdata_len);
> mutex_unlock(&kernfs_mutex);
>
> - if (secdata)
> - security_release_secctx(secdata, secdata_len);
> + if (secdata) {
> + lsmcontext_init(&context, secdata, secdata_len, 0);
> + security_release_secctx(&context);
> + }
> return error;
> }
>
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index 4dbb0ee23432..af1c0db29c39 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -131,8 +131,12 @@ nfs4_label_init_security(struct inode *dir, struct dentry *dentry,
> static inline void
> nfs4_label_release_security(struct nfs4_label *label)
> {
> - if (label)
> - security_release_secctx(label->label, label->len);
> + struct lsmcontext scaff; /* scaffolding */
> +
> + if (label) {
> + lsmcontext_init(&scaff, label->label, label->len, 0);
> + security_release_secctx(&scaff);
> + }
> }
> static inline u32 *nfs4_bitmask(struct nfs_server *server, struct nfs4_label *label)
> {
> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> index 3de42a729093..bb3db033e144 100644
> --- a/fs/nfsd/nfs4xdr.c
> +++ b/fs/nfsd/nfs4xdr.c
> @@ -2420,6 +2420,7 @@ nfsd4_encode_fattr(struct xdr_stream *xdr, struct svc_fh *fhp,
> __be32 status;
> int err;
> struct nfs4_acl *acl = NULL;
> + struct lsmcontext scaff; /* scaffolding */
> void *context = NULL;
> int contextlen;
> bool contextsupport = false;
> @@ -2919,8 +2920,10 @@ nfsd4_encode_fattr(struct xdr_stream *xdr, struct svc_fh *fhp,
>
> out:
> #ifdef CONFIG_NFSD_V4_SECURITY_LABEL
> - if (context)
> - security_release_secctx(context, contextlen);
> + if (context) {
> + lsmcontext_init(&scaff, context, contextlen, 0); /*scaffolding*/
> + security_release_secctx(&scaff);
> + }
> #endif /* CONFIG_NFSD_V4_SECURITY_LABEL */
> kfree(acl);
> if (tempfh) {
> diff --git a/include/linux/security.h b/include/linux/security.h
> index c712fc72b7bd..ddf6d7cb23f1 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -76,6 +76,41 @@ enum lsm_event {
> LSM_POLICY_CHANGE,
> };
>
> +/*
> + * A "security context" is the text representation of
> + * the information used by LSMs.
> + * This structure contains the string, its length, and which LSM
> + * it is useful for.
> + */
> +struct lsmcontext {
> + char *context; /* Provided by the module */
> + u32 len;
> + int slot; /* Identifies the module */
> +};
> +
> +/**
> + * lsmcontext_init - initialize an lsmcontext structure.
> + * @cp: Pointer to the context to initialize
> + * @context: Initial context, or NULL
> + * @size: Size of context, or 0
> + * @slot: Which LSM provided the context
> + *
> + * Fill in the lsmcontext from the provided information.
> + * This is a scaffolding function that will be removed when
> + * lsmcontext integration is complete.
> + */
> +static inline void lsmcontext_init(struct lsmcontext *cp, char *context,
> + u32 size, int slot)
> +{
> + cp->slot = slot;
> + cp->context = context;
> +
> + if (context == NULL || size == 0)
> + cp->len = 0;
> + else
> + cp->len = strlen(context);
> +}
> +
> /*
> * Data exported by the security modules
> */
> @@ -445,7 +480,7 @@ int security_ismaclabel(const char *name);
> int security_secid_to_secctx(struct lsmblob *blob, char **secdata, u32 *seclen);
> int security_secctx_to_secid(const char *secdata, u32 seclen,
> struct lsmblob *blob);
> -void security_release_secctx(char *secdata, u32 seclen);
> +void security_release_secctx(struct lsmcontext *cp);
>
> void security_inode_invalidate_secctx(struct inode *inode);
> int security_inode_notifysecctx(struct inode *inode, void *ctx, u32 ctxlen);
> @@ -1236,7 +1271,7 @@ static inline int security_secctx_to_secid(const char *secdata,
> return -EOPNOTSUPP;
> }
>
> -static inline void security_release_secctx(char *secdata, u32 seclen)
> +static inline void security_release_secctx(struct lsmcontext *cp)
> {
> }
>
> diff --git a/include/net/scm.h b/include/net/scm.h
> index 31ae605fcc0a..6c7c3c229e4a 100644
> --- a/include/net/scm.h
> +++ b/include/net/scm.h
> @@ -92,6 +92,7 @@ static __inline__ int scm_send(struct socket *sock, struct msghdr *msg,
> #ifdef CONFIG_SECURITY_NETWORK
> static inline void scm_passec(struct socket *sock, struct msghdr *msg, struct scm_cookie *scm)
> {
> + struct lsmcontext context;
> char *secdata;
> u32 seclen;
> int err;
> @@ -102,7 +103,8 @@ static inline void scm_passec(struct socket *sock, struct msghdr *msg, struct sc
>
> if (!err) {
> put_cmsg(msg, SOL_SOCKET, SCM_SECURITY, seclen, secdata);
> - security_release_secctx(secdata, seclen);
> + lsmcontext_init(&context, secdata, seclen, 0);/*scaffolding*/
> + security_release_secctx(&context);
> }
> }
> }
> diff --git a/kernel/audit.c b/kernel/audit.c
> index 1b51e907f131..f844a2a642e6 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -1193,6 +1193,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
> struct audit_sig_info *sig_data;
> char *ctx = NULL;
> u32 len;
> + struct lsmcontext scaff; /* scaffolding */
>
> err = audit_netlink_ok(skb, msg_type);
> if (err)
> @@ -1437,15 +1438,18 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
> }
> sig_data = kmalloc(sizeof(*sig_data) + len, GFP_KERNEL);
> if (!sig_data) {
> - if (lsmblob_is_set(&audit_sig_lsm))
> - security_release_secctx(ctx, len);
> + if (lsmblob_is_set(&audit_sig_lsm)) {
> + lsmcontext_init(&scaff, ctx, len, 0);
> + security_release_secctx(&scaff);
> + }
> return -ENOMEM;
> }
> sig_data->uid = from_kuid(&init_user_ns, audit_sig_uid);
> sig_data->pid = audit_sig_pid;
> if (lsmblob_is_set(&audit_sig_lsm)) {
> memcpy(sig_data->ctx, ctx, len);
> - security_release_secctx(ctx, len);
> + lsmcontext_init(&scaff, ctx, len, 0);
> + security_release_secctx(&scaff);
> }
> audit_send_reply(skb, seq, AUDIT_SIGNAL_INFO, 0, 0,
> sig_data, sizeof(*sig_data) + len);
> @@ -2074,6 +2078,7 @@ int audit_log_task_context(struct audit_buffer *ab)
> unsigned len;
> int error;
> struct lsmblob blob;
> + struct lsmcontext scaff; /* scaffolding */
>
> security_task_getsecid(current, &blob);
> if (!lsmblob_is_set(&blob))
> @@ -2087,7 +2092,8 @@ int audit_log_task_context(struct audit_buffer *ab)
> }
>
> audit_log_format(ab, " subj=%s", ctx);
> - security_release_secctx(ctx, len);
> + lsmcontext_init(&scaff, ctx, len, 0);
> + security_release_secctx(&scaff);
> return 0;
>
> error_path:
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index c7aa39bda5cc..9fab0e7d90c3 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -943,6 +943,7 @@ static int audit_log_pid_context(struct audit_context *context, pid_t pid,
> struct lsmblob *blob, char *comm)
> {
> struct audit_buffer *ab;
> + struct lsmcontext lsmcxt;
> char *ctx = NULL;
> u32 len;
> int rc = 0;
> @@ -960,7 +961,8 @@ static int audit_log_pid_context(struct audit_context *context, pid_t pid,
> rc = 1;
> } else {
> audit_log_format(ab, " obj=%s", ctx);
> - security_release_secctx(ctx, len);
> + lsmcontext_init(&lsmcxt, ctx, len, 0); /*scaffolding*/
> + security_release_secctx(&lsmcxt);
> }
> }
> audit_log_format(ab, " ocomm=");
> @@ -1172,6 +1174,7 @@ static void audit_log_fcaps(struct audit_buffer *ab, struct audit_names *name)
>
> static void show_special(struct audit_context *context, int *call_panic)
> {
> + struct lsmcontext lsmcxt;
> struct audit_buffer *ab;
> int i;
>
> @@ -1205,7 +1208,8 @@ static void show_special(struct audit_context *context, int *call_panic)
> *call_panic = 1;
> } else {
> audit_log_format(ab, " obj=%s", ctx);
> - security_release_secctx(ctx, len);
> + lsmcontext_init(&lsmcxt, ctx, len, 0);
> + security_release_secctx(&lsmcxt);
> }
> }
> if (context->ipc.has_perm) {
> @@ -1352,6 +1356,7 @@ static void audit_log_name(struct audit_context *context, struct audit_names *n,
> char *ctx = NULL;
> u32 len;
> struct lsmblob blob;
> + struct lsmcontext lsmcxt;
>
> lsmblob_init(&blob, n->osid);
> if (security_secid_to_secctx(&blob, &ctx, &len)) {
> @@ -1360,7 +1365,8 @@ static void audit_log_name(struct audit_context *context, struct audit_names *n,
> *call_panic = 2;
> } else {
> audit_log_format(ab, " obj=%s", ctx);
> - security_release_secctx(ctx, len);
> + lsmcontext_init(&lsmcxt, ctx, len, 0); /* scaffolding */
> + security_release_secctx(&lsmcxt);
> }
> }
>
> diff --git a/net/ipv4/ip_sockglue.c b/net/ipv4/ip_sockglue.c
> index e05f4ef68bd8..7834c357b60b 100644
> --- a/net/ipv4/ip_sockglue.c
> +++ b/net/ipv4/ip_sockglue.c
> @@ -130,6 +130,7 @@ static void ip_cmsg_recv_checksum(struct msghdr *msg, struct sk_buff *skb,
>
> static void ip_cmsg_recv_security(struct msghdr *msg, struct sk_buff *skb)
> {
> + struct lsmcontext context;
> struct lsmblob lb;
> char *secdata;
> u32 seclen;
> @@ -144,7 +145,8 @@ static void ip_cmsg_recv_security(struct msghdr *msg, struct sk_buff *skb)
> return;
>
> put_cmsg(msg, SOL_IP, SCM_SECURITY, seclen, secdata);
> - security_release_secctx(secdata, seclen);
> + lsmcontext_init(&context, secdata, seclen, 0); /* scaffolding */
> + security_release_secctx(&context);
> }
>
> static void ip_cmsg_recv_dstaddr(struct msghdr *msg, struct sk_buff *skb)
> diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
> index ca0968f13240..6954e6600583 100644
> --- a/net/netfilter/nf_conntrack_netlink.c
> +++ b/net/netfilter/nf_conntrack_netlink.c
> @@ -331,6 +331,7 @@ static int ctnetlink_dump_secctx(struct sk_buff *skb, const struct nf_conn *ct)
> int len, ret;
> char *secctx;
> struct lsmblob blob;
> + struct lsmcontext context;
>
> lsmblob_init(&blob, ct->secmark);
> ret = security_secid_to_secctx(&blob, &secctx, &len);
> @@ -348,7 +349,8 @@ static int ctnetlink_dump_secctx(struct sk_buff *skb, const struct nf_conn *ct)
>
> ret = 0;
> nla_put_failure:
> - security_release_secctx(secctx, len);
> + lsmcontext_init(&context, secctx, len, 0); /* scaffolding */
> + security_release_secctx(&context);
> return ret;
> }
> #else
> diff --git a/net/netfilter/nf_conntrack_standalone.c b/net/netfilter/nf_conntrack_standalone.c
> index c793103f3cd7..79158ad0486e 100644
> --- a/net/netfilter/nf_conntrack_standalone.c
> +++ b/net/netfilter/nf_conntrack_standalone.c
> @@ -176,6 +176,7 @@ static void ct_show_secctx(struct seq_file *s, const struct nf_conn *ct)
> u32 len;
> char *secctx;
> struct lsmblob blob;
> + struct lsmcontext context;
>
> lsmblob_init(&blob, ct->secmark);
> ret = security_secid_to_secctx(&blob, &secctx, &len);
> @@ -184,7 +185,8 @@ static void ct_show_secctx(struct seq_file *s, const struct nf_conn *ct)
>
> seq_printf(s, "secctx=%s ", secctx);
>
> - security_release_secctx(secctx, len);
> + lsmcontext_init(&context, secctx, len, 0); /* scaffolding */
> + security_release_secctx(&context);
> }
> #else
> static inline void ct_show_secctx(struct seq_file *s, const struct nf_conn *ct)
> diff --git a/net/netfilter/nfnetlink_queue.c b/net/netfilter/nfnetlink_queue.c
> index 59211bff90ab..fe8403ef4e89 100644
> --- a/net/netfilter/nfnetlink_queue.c
> +++ b/net/netfilter/nfnetlink_queue.c
> @@ -399,6 +399,7 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
> enum ip_conntrack_info uninitialized_var(ctinfo);
> struct nfnl_ct_hook *nfnl_ct;
> bool csum_verify;
> + struct lsmcontext scaff; /* scaffolding */
> char *secdata = NULL;
> u32 seclen = 0;
>
> @@ -629,8 +630,10 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
> }
>
> nlh->nlmsg_len = skb->len;
> - if (seclen)
> - security_release_secctx(secdata, seclen);
> + if (seclen) {
> + lsmcontext_init(&scaff, secdata, seclen, 0);
> + security_release_secctx(&scaff);
> + }
> return skb;
>
> nla_put_failure:
> @@ -638,8 +641,10 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
> kfree_skb(skb);
> net_err_ratelimited("nf_queue: error creating packet message\n");
> nlmsg_failure:
> - if (seclen)
> - security_release_secctx(secdata, seclen);
> + if (seclen) {
> + lsmcontext_init(&scaff, secdata, seclen, 0);
> + security_release_secctx(&scaff);
> + }
> return NULL;
> }
>
> diff --git a/net/netlabel/netlabel_unlabeled.c b/net/netlabel/netlabel_unlabeled.c
> index 2294aa9471e6..15b1945853be 100644
> --- a/net/netlabel/netlabel_unlabeled.c
> +++ b/net/netlabel/netlabel_unlabeled.c
> @@ -387,6 +387,7 @@ int netlbl_unlhsh_add(struct net *net,
> struct net_device *dev;
> struct netlbl_unlhsh_iface *iface;
> struct audit_buffer *audit_buf = NULL;
> + struct lsmcontext context;
> char *secctx = NULL;
> u32 secctx_len;
> struct lsmblob blob;
> @@ -457,7 +458,9 @@ int netlbl_unlhsh_add(struct net *net,
> &secctx,
> &secctx_len) == 0) {
> audit_log_format(audit_buf, " sec_obj=%s", secctx);
> - security_release_secctx(secctx, secctx_len);
> + /* scaffolding */
> + lsmcontext_init(&context, secctx, secctx_len, 0);
> + security_release_secctx(&context);
> }
> audit_log_format(audit_buf, " res=%u", ret_val == 0 ? 1 : 0);
> audit_log_end(audit_buf);
> @@ -488,6 +491,7 @@ static int netlbl_unlhsh_remove_addr4(struct net *net,
> struct netlbl_unlhsh_addr4 *entry;
> struct audit_buffer *audit_buf;
> struct net_device *dev;
> + struct lsmcontext context;
> char *secctx;
> u32 secctx_len;
> struct lsmblob blob;
> @@ -516,7 +520,9 @@ static int netlbl_unlhsh_remove_addr4(struct net *net,
> security_secid_to_secctx(&blob,
> &secctx, &secctx_len) == 0) {
> audit_log_format(audit_buf, " sec_obj=%s", secctx);
> - security_release_secctx(secctx, secctx_len);
> + /* scaffolding */
> + lsmcontext_init(&context, secctx, secctx_len, 0);
> + security_release_secctx(&context);
> }
> audit_log_format(audit_buf, " res=%u", entry != NULL ? 1 : 0);
> audit_log_end(audit_buf);
> @@ -553,6 +559,7 @@ static int netlbl_unlhsh_remove_addr6(struct net *net,
> struct netlbl_unlhsh_addr6 *entry;
> struct audit_buffer *audit_buf;
> struct net_device *dev;
> + struct lsmcontext context;
> char *secctx;
> u32 secctx_len;
> struct lsmblob blob;
> @@ -580,7 +587,8 @@ static int netlbl_unlhsh_remove_addr6(struct net *net,
> security_secid_to_secctx(&blob,
> &secctx, &secctx_len) == 0) {
> audit_log_format(audit_buf, " sec_obj=%s", secctx);
> - security_release_secctx(secctx, secctx_len);
> + lsmcontext_init(&context, secctx, secctx_len, 0);
> + security_release_secctx(&context);
> }
> audit_log_format(audit_buf, " res=%u", entry != NULL ? 1 : 0);
> audit_log_end(audit_buf);
> @@ -1094,6 +1102,7 @@ static int netlbl_unlabel_staticlist_gen(u32 cmd,
> int ret_val = -ENOMEM;
> struct netlbl_unlhsh_walk_arg *cb_arg = arg;
> struct net_device *dev;
> + struct lsmcontext context;
> void *data;
> u32 secid;
> char *secctx;
> @@ -1161,7 +1170,9 @@ static int netlbl_unlabel_staticlist_gen(u32 cmd,
> NLBL_UNLABEL_A_SECCTX,
> secctx_len,
> secctx);
> - security_release_secctx(secctx, secctx_len);
> + /* scaffolding */
> + lsmcontext_init(&context, secctx, secctx_len, 0);
> + security_release_secctx(&context);
> if (ret_val != 0)
> goto list_cb_failure;
>
> diff --git a/net/netlabel/netlabel_user.c b/net/netlabel/netlabel_user.c
> index 2ccc6567e2a2..94aea4985b74 100644
> --- a/net/netlabel/netlabel_user.c
> +++ b/net/netlabel/netlabel_user.c
> @@ -98,6 +98,7 @@ struct audit_buffer *netlbl_audit_start_common(int type,
> struct netlbl_audit *audit_info)
> {
> struct audit_buffer *audit_buf;
> + struct lsmcontext context;
> char *secctx;
> u32 secctx_len;
> struct lsmblob blob;
> @@ -117,7 +118,8 @@ struct audit_buffer *netlbl_audit_start_common(int type,
> if (audit_info->secid != 0 &&
> security_secid_to_secctx(&blob, &secctx, &secctx_len) == 0) {
> audit_log_format(audit_buf, " subj=%s", secctx);
> - security_release_secctx(secctx, secctx_len);
> + lsmcontext_init(&context, secctx, secctx_len, 0);/*scaffolding*/
> + security_release_secctx(&context);
> }
>
> return audit_buf;
> diff --git a/security/security.c b/security/security.c
> index 92c5aa427b53..d5f173e85393 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -460,6 +460,7 @@ void __init security_add_hooks(struct security_hook_list *hooks, int count,
> hooks[i].head == &security_hook_heads.setprocattr ||
> hooks[i].head == &security_hook_heads.secctx_to_secid ||
> hooks[i].head == &security_hook_heads.secid_to_secctx ||
> + hooks[i].head == &security_hook_heads.release_secctx ||
> hooks[i].head == &security_hook_heads.ipc_getsecid ||
> hooks[i].head == &security_hook_heads.task_getsecid ||
> hooks[i].head == &security_hook_heads.inode_getsecid ||
> @@ -2091,16 +2092,19 @@ int security_secctx_to_secid(const char *secdata, u32 seclen,
> }
> EXPORT_SYMBOL(security_secctx_to_secid);
>
> -void security_release_secctx(char *secdata, u32 seclen)
> +void security_release_secctx(struct lsmcontext *cp)
> {
> struct security_hook_list *hp;
> - int *display = current->security;
>
> hlist_for_each_entry(hp, &security_hook_heads.release_secctx, list)
> - if (*display == LSMBLOB_INVALID || *display == hp->slot) {
> - hp->hook.release_secctx(secdata, seclen);
> + if (cp->slot == hp->slot) {
> + hp->hook.release_secctx(cp->context, cp->len);
> + memset(cp, 0, sizeof(*cp));
> return;
> }
> +
> + pr_warn("%s context \"%s\" from slot %d not released\n", __func__,
> + cp->context, cp->slot);
> }
> EXPORT_SYMBOL(security_release_secctx);
>
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index e9560b078efe..3834b751d1e9 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -4439,11 +4439,16 @@ static int smack_secctx_to_secid(const char *secdata, u32 seclen, u32 *secid)
> return 0;
> }
>
> -/*
> - * There used to be a smack_release_secctx hook
> - * that did nothing back when hooks were in a vector.
> - * Now that there's a list such a hook adds cost.
> +/**
> + * smack_release_secctx - do everything necessary to free a context
> + * @secdata: Unused
> + * @seclen: Unused
> + *
> + * Do nothing but hold a slot in the hooks list.
> */
> +static void smack_release_secctx(char *secdata, u32 seclen)
> +{
> +}
>
> static int smack_inode_notifysecctx(struct inode *inode, void *ctx, u32 ctxlen)
> {
> @@ -4683,6 +4688,7 @@ static struct security_hook_list smack_hooks[] __lsm_ro_after_init = {
> LSM_HOOK_INIT(ismaclabel, smack_ismaclabel),
> LSM_HOOK_INIT(secid_to_secctx, smack_secid_to_secctx),
> LSM_HOOK_INIT(secctx_to_secid, smack_secctx_to_secid),
> + LSM_HOOK_INIT(release_secctx, smack_release_secctx),
> LSM_HOOK_INIT(inode_notifysecctx, smack_inode_notifysecctx),
> LSM_HOOK_INIT(inode_setsecctx, smack_inode_setsecctx),
> LSM_HOOK_INIT(inode_getsecctx, smack_inode_getsecctx),
>
^ permalink raw reply
* Re: [PATCH v3 24/24] AppArmor: Remove the exclusive flag
From: John Johansen @ 2019-06-24 21:45 UTC (permalink / raw)
To: Casey Schaufler, casey.schaufler, jmorris, linux-security-module,
selinux
Cc: keescook, penguin-kernel, paul, sds
In-Reply-To: <20190621185233.6766-25-casey@schaufler-ca.com>
On 6/21/19 11:52 AM, Casey Schaufler wrote:
> With the inclusion of the "display" process attribute
> mechanism AppArmor no longer needs to be treated as an
> "exclusive" security module. Remove the flag that indicates
> it is exclusive. Remove the stub getpeersec_dgram AppArmor
> hook as it has no effect in the single LSM case and
> interferes in the multiple LSM case.
>
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
Reviewed-by: John Johansen <john.johansen@canonical.com>
> ---
> security/apparmor/lsm.c | 20 +-------------------
> 1 file changed, 1 insertion(+), 19 deletions(-)
>
> diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
> index dcbbefbd95ff..c4365434f10b 100644
> --- a/security/apparmor/lsm.c
> +++ b/security/apparmor/lsm.c
> @@ -1082,22 +1082,6 @@ static int apparmor_socket_getpeersec_stream(struct socket *sock,
> return error;
> }
>
> -/**
> - * apparmor_socket_getpeersec_dgram - get security label of packet
> - * @sock: the peer socket
> - * @skb: packet data
> - * @secid: pointer to where to put the secid of the packet
> - *
> - * Sets the netlabel socket state on sk from parent
> - */
> -static int apparmor_socket_getpeersec_dgram(struct socket *sock,
> - struct sk_buff *skb, u32 *secid)
> -
> -{
> - /* TODO: requires secid support */
> - return -ENOPROTOOPT;
> -}
> -
> /**
> * apparmor_sock_graft - Initialize newly created socket
> * @sk: child sock
> @@ -1196,8 +1180,6 @@ static struct security_hook_list apparmor_hooks[] __lsm_ro_after_init = {
> #endif
> LSM_HOOK_INIT(socket_getpeersec_stream,
> apparmor_socket_getpeersec_stream),
> - LSM_HOOK_INIT(socket_getpeersec_dgram,
> - apparmor_socket_getpeersec_dgram),
> LSM_HOOK_INIT(sock_graft, apparmor_sock_graft),
> #ifdef CONFIG_NETWORK_SECMARK
> LSM_HOOK_INIT(inet_conn_request, apparmor_inet_conn_request),
> @@ -1709,7 +1691,7 @@ static int __init apparmor_init(void)
>
> DEFINE_LSM(apparmor) = {
> .name = "apparmor",
> - .flags = LSM_FLAG_LEGACY_MAJOR | LSM_FLAG_EXCLUSIVE,
> + .flags = LSM_FLAG_LEGACY_MAJOR,
> .enabled = &apparmor_enabled,
> .blobs = &apparmor_blob_sizes,
> .init = apparmor_init,
>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox