* Re: [PATCH] seccomp: Add pkru into seccomp_data
From: Andy Lutomirski @ 2018-10-25 23:00 UTC (permalink / raw)
To: Michael Sammler
Cc: Florian Weimer, Will Drewry, Kees Cook, Linux API, linuxram,
linuxppc-dev
In-Reply-To: <c4d3f5c1-a2dc-5b44-4d12-1fae829a372c@mpi-sws.org>
On Thu, Oct 25, 2018 at 9:42 AM Michael Sammler <msammler@mpi-sws.org> wrote:
>
> On 10/25/2018 11:12 AM, Florian Weimer wrote:
> >> I understand your concern about exposing the number of protection keys
> >> in the ABI. One idea would be to state, that the pkru field (which
> >> should probably be renamed) contains an architecture specific value,
> >> which could then be the PKRU on x86 and AMR (or another register) on
> >> POWER. This new field should probably be extended to __u64 and the
> >> reserved field removed.
> > POWER also has proper read/write bit separation, not PKEY_DISABLE_ACCESS
> > (disable read and write) and PKEY_DISABLE_WRITE like Intel. It's
> > currently translated by the kernel, but I really need a
> > PKEY_DISABLE_READ bit in glibc to implement pkey_get in case the memory
> > is write-only.
> The idea here would be to simply provide the raw value of the register
> (PKRU on x86, AMR on POWER) to the BPF program and let the BPF program
> (or maybe a higher level library like libseccomp) deal with the
> complications of interpreting this architecture specific value (similar
> how the BPF program currently already has to deal with architecture
> specific system call numbers). If an architecture were to support more
> protection keys than fit into the field, the architecture specific value
> stored in the field might simply be the first protection keys. If there
> was interest, it would be possible to add more architecture specific
> fields to seccomp_data.
> >> Another idea would be to not add a field in the seccomp_data
> >> structure, but instead provide a new BPF instruction, which reads the
> >> value of a specified protection key.
> > I would prefer that if it's possible. We should make sure that the bits
> > are the same as those returned from pkey_get. I have an implementation
> > on POWER, but have yet to figure out the implications for 32-bit because
> > I do not know the AMR register size there.
> >
> > Thanks,
> > Florian
> I have had a look at how BPF is implemented and it does not seem to be
> easy to just add an BPF instruction for seccomp since (as far as I
> understand) the code of the classical BPF (as used by seccomp) is shared
> with the code of eBPF, which is used in many parts of the kernel and
> there is at least one interpreter and one JIT compiler for BPF. But
> maybe someone with more experience than me can comment on how hard it
> would be to add an instruction to BPF.
>
You could bite the bullet and add seccomp eBPF support :)
^ permalink raw reply
* Re: [PATCH ghak90 (was ghak32) V4 03/10] audit: log container info of syscalls
From: Steve Grubb @ 2018-10-25 21:55 UTC (permalink / raw)
To: Paul Moore
Cc: rgb, simo, carlos, linux-api, containers, linux-kernel, dhowells,
linux-audit, netfilter-devel, ebiederm, luto, netdev,
linux-fsdevel, Eric Paris, Serge Hallyn, viro
In-Reply-To: <CAHC9VhScaG8aOFYRV5hPXEKob1QRth5YFEbHNY=ZgKKAKxBBsQ@mail.gmail.com>
On Thu, 25 Oct 2018 16:40:19 -0400
Paul Moore <paul@paul-moore.com> wrote:
> On Thu, Oct 25, 2018 at 1:38 PM Richard Guy Briggs <rgb@redhat.com>
> wrote:
> > On 2018-10-25 17:57, Steve Grubb wrote:
> > > On Thu, 25 Oct 2018 08:27:32 -0400
> > > Richard Guy Briggs <rgb@redhat.com> wrote:
> > >
> > > > On 2018-10-25 06:49, Paul Moore wrote:
> > > > > On Thu, Oct 25, 2018 at 2:06 AM Steve Grubb
> > > > > <sgrubb@redhat.com> wrote:
> > > > > > On Wed, 24 Oct 2018 20:42:55 -0400
> > > > > > Richard Guy Briggs <rgb@redhat.com> wrote:
> > > > > > > On 2018-10-24 16:55, Paul Moore wrote:
> > > > > > > > On Wed, Oct 24, 2018 at 11:15 AM Richard Guy Briggs
> > > > > > > > <rgb@redhat.com> wrote:
> > > > > > > > > On 2018-10-19 19:16, Paul Moore wrote:
> > > > > > > > > > On Sun, Aug 5, 2018 at 4:32 AM Richard Guy Briggs
> > > > > > > > > > <rgb@redhat.com> wrote:
> > > > >
> > > > > ...
> > > > >
> > > > > > > > > > > +/*
> > > > > > > > > > > + * audit_log_contid - report container info
> > > > > > > > > > > + * @tsk: task to be recorded
> > > > > > > > > > > + * @context: task or local context for record
> > > > > > > > > > > + * @op: contid string description
> > > > > > > > > > > + */
> > > > > > > > > > > +int audit_log_contid(struct task_struct *tsk,
> > > > > > > > > > > + struct audit_context
> > > > > > > > > > > *context, char *op) +{
> > > > > > > > > > > + struct audit_buffer *ab;
> > > > > > > > > > > +
> > > > > > > > > > > + if (!audit_contid_set(tsk))
> > > > > > > > > > > + return 0;
> > > > > > > > > > > + /* Generate AUDIT_CONTAINER record with
> > > > > > > > > > > container ID */
> > > > > > > > > > > + ab = audit_log_start(context, GFP_KERNEL,
> > > > > > > > > > > AUDIT_CONTAINER);
> > > > > > > > > > > + if (!ab)
> > > > > > > > > > > + return -ENOMEM;
> > > > > > > > > > > + audit_log_format(ab, "op=%s contid=%llu",
> > > > > > > > > > > + op,
> > > > > > > > > > > audit_get_contid(tsk));
> > > > > > > > > > > + audit_log_end(ab);
> > > > > > > > > > > + return 0;
> > > > > > > > > > > +}
> > > > > > > > > > > +EXPORT_SYMBOL(audit_log_contid);
> > > > > > > > > >
> > > > > > > > > > As discussed in the previous iteration of the
> > > > > > > > > > patch, I prefer AUDIT_CONTAINER_ID here over
> > > > > > > > > > AUDIT_CONTAINER. If you feel strongly about
> > > > > > > > > > keeping it as-is with AUDIT_CONTAINER I suppose I
> > > > > > > > > > could live with that, but it is isn't my first
> > > > > > > > > > choice.
> > > > > > > > >
> > > > > > > > > I don't have a strong opinion on this one, mildly
> > > > > > > > > preferring the shorter one only because it is
> > > > > > > > > shorter.
> > > > > > > >
> > > > > > > > We already have multiple AUDIT_CONTAINER* record types,
> > > > > > > > so it seems as though we should use "AUDIT_CONTAINER"
> > > > > > > > as a prefix of sorts, rather than a type itself.
> > > > > > >
> > > > > > > I'm fine with that. I'd still like to hear Steve's
> > > > > > > input. He had stronger opinions than me.
> > > > > >
> > > > > > The creation event should be separate and distinct from the
> > > > > > continuing use when its used as a supplemental record. IOW,
> > > > > > binding the ID to a container is part of the lifecycle and
> > > > > > needs to be kept distinct.
> > > > >
> > > > > Steve's comment is pretty ambiguous when it comes to
> > > > > AUDIT_CONTAINER vs AUDIT_CONTAINER_ID, but one could argue
> > > > > that AUDIT_CONTAINER_ID helps distinguish the audit container
> > > > > id marking record and gets to what I believe is the spirit of
> > > > > Steve's comment. Taking this in context with my previous
> > > > > remarks, let's switch to using AUDIT_CONTAINER_ID.
> > > >
> > > > I suspect Steve is mixing up AUDIT_CONTAINER_OP with
> > > > AUDIT_CONTAINER_ID, confusing the fact that they are two
> > > > seperate records. As a summary, the suggested records are:
> > > > CONTAINER_OP audit container identifier creation
> > > > CONTAINER audit container identifier aux record to an
> > > > event
> > > >
> > > > and what Paul is suggesting (which is fine by me) is:
> > > > CONTAINER_OP audit container identifier creation event
> > > > CONTAINER_ID audit container identifier aux record to
> > > > an event
> > > >
> > > > Steve, please indicate you are fine with this.
> > >
> > > I thought it was:
> >
> > It *was*. It was changed at Paul's request in this v3 thread:
> > https://www.redhat.com/archives/linux-audit/2018-July/msg00087.html
> >
> > And listed in the examples and changelog to this v4 patchset:
> > https://www.redhat.com/archives/linux-audit/2018-July/msg00178.html
> >
> > It is also listed in this userspace patchset update v4 (which should
> > also have had a changelog added to it, note to self...):
> > https://www.redhat.com/archives/linux-audit/2018-July/msg00189.html
> >
> > I realize it is hard to keep up with all the detail changes in these
> > patchsets...
> >
> > > CONTAINER_ID audit container identifier creation event
> > > event. CONTAINER audit container identifier aux record to an
> > > event
> > >
> > > Or vice versa. Don't mix up creation of the identifier with
> > > operations.
> >
> > Exactly what I'm trying to avoid... Worded another way: "Don't mix
> > up the creation operation with routine reporting of the identifier
> > in events." Steve, can you and Paul discuss and agree on what they
> > should be called? I don't have a horse in this race, but I need to
> > record the result of that run. ;-)
>
> See my previous comments, I think I've been pretty clear on what I
> would like to see.
And historically speaking setting audit loginuid produces a LOGIN
event, so it only makes sense to consider binding container ID to
container as a CONTAINER event. For other supplemental records, we name
things what they are: PATH, CWD, SOCKADDR, etc. So, CONTAINER_ID makes
sense. CONTAINER_OP sounds like its for operations on a container. Do
we have any operations on a container?
-Steve
^ permalink raw reply
* Re: [PATCH ghak90 (was ghak32) V4 03/10] audit: log container info of syscalls
From: Paul Moore @ 2018-10-25 20:40 UTC (permalink / raw)
To: rgb
Cc: sgrubb, simo, carlos, linux-api, containers, linux-kernel,
dhowells, linux-audit, netfilter-devel, ebiederm, luto, netdev,
linux-fsdevel, Eric Paris, Serge Hallyn, viro
In-Reply-To: <20181025173830.4yklhnrydt5qvr67@madcap2.tricolour.ca>
On Thu, Oct 25, 2018 at 1:38 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2018-10-25 17:57, Steve Grubb wrote:
> > On Thu, 25 Oct 2018 08:27:32 -0400
> > Richard Guy Briggs <rgb@redhat.com> wrote:
> >
> > > On 2018-10-25 06:49, Paul Moore wrote:
> > > > On Thu, Oct 25, 2018 at 2:06 AM Steve Grubb <sgrubb@redhat.com>
> > > > wrote:
> > > > > On Wed, 24 Oct 2018 20:42:55 -0400
> > > > > Richard Guy Briggs <rgb@redhat.com> wrote:
> > > > > > On 2018-10-24 16:55, Paul Moore wrote:
> > > > > > > On Wed, Oct 24, 2018 at 11:15 AM Richard Guy Briggs
> > > > > > > <rgb@redhat.com> wrote:
> > > > > > > > On 2018-10-19 19:16, Paul Moore wrote:
> > > > > > > > > On Sun, Aug 5, 2018 at 4:32 AM Richard Guy Briggs
> > > > > > > > > <rgb@redhat.com> wrote:
> > > >
> > > > ...
> > > >
> > > > > > > > > > +/*
> > > > > > > > > > + * audit_log_contid - report container info
> > > > > > > > > > + * @tsk: task to be recorded
> > > > > > > > > > + * @context: task or local context for record
> > > > > > > > > > + * @op: contid string description
> > > > > > > > > > + */
> > > > > > > > > > +int audit_log_contid(struct task_struct *tsk,
> > > > > > > > > > + struct audit_context
> > > > > > > > > > *context, char *op) +{
> > > > > > > > > > + struct audit_buffer *ab;
> > > > > > > > > > +
> > > > > > > > > > + if (!audit_contid_set(tsk))
> > > > > > > > > > + return 0;
> > > > > > > > > > + /* Generate AUDIT_CONTAINER record with
> > > > > > > > > > container ID */
> > > > > > > > > > + ab = audit_log_start(context, GFP_KERNEL,
> > > > > > > > > > AUDIT_CONTAINER);
> > > > > > > > > > + if (!ab)
> > > > > > > > > > + return -ENOMEM;
> > > > > > > > > > + audit_log_format(ab, "op=%s contid=%llu",
> > > > > > > > > > + op, audit_get_contid(tsk));
> > > > > > > > > > + audit_log_end(ab);
> > > > > > > > > > + return 0;
> > > > > > > > > > +}
> > > > > > > > > > +EXPORT_SYMBOL(audit_log_contid);
> > > > > > > > >
> > > > > > > > > As discussed in the previous iteration of the patch, I
> > > > > > > > > prefer AUDIT_CONTAINER_ID here over AUDIT_CONTAINER. If
> > > > > > > > > you feel strongly about keeping it as-is with
> > > > > > > > > AUDIT_CONTAINER I suppose I could live with that, but it
> > > > > > > > > is isn't my first choice.
> > > > > > > >
> > > > > > > > I don't have a strong opinion on this one, mildly
> > > > > > > > preferring the shorter one only because it is shorter.
> > > > > > >
> > > > > > > We already have multiple AUDIT_CONTAINER* record types, so it
> > > > > > > seems as though we should use "AUDIT_CONTAINER" as a prefix
> > > > > > > of sorts, rather than a type itself.
> > > > > >
> > > > > > I'm fine with that. I'd still like to hear Steve's input. He
> > > > > > had stronger opinions than me.
> > > > >
> > > > > The creation event should be separate and distinct from the
> > > > > continuing use when its used as a supplemental record. IOW,
> > > > > binding the ID to a container is part of the lifecycle and needs
> > > > > to be kept distinct.
> > > >
> > > > Steve's comment is pretty ambiguous when it comes to AUDIT_CONTAINER
> > > > vs AUDIT_CONTAINER_ID, but one could argue that AUDIT_CONTAINER_ID
> > > > helps distinguish the audit container id marking record and gets to
> > > > what I believe is the spirit of Steve's comment. Taking this in
> > > > context with my previous remarks, let's switch to using
> > > > AUDIT_CONTAINER_ID.
> > >
> > > I suspect Steve is mixing up AUDIT_CONTAINER_OP with
> > > AUDIT_CONTAINER_ID, confusing the fact that they are two seperate
> > > records. As a summary, the suggested records are:
> > > CONTAINER_OP audit container identifier creation
> > > CONTAINER audit container identifier aux record to an
> > > event
> > >
> > > and what Paul is suggesting (which is fine by me) is:
> > > CONTAINER_OP audit container identifier creation event
> > > CONTAINER_ID audit container identifier aux record to
> > > an event
> > >
> > > Steve, please indicate you are fine with this.
> >
> > I thought it was:
>
> It *was*. It was changed at Paul's request in this v3 thread:
> https://www.redhat.com/archives/linux-audit/2018-July/msg00087.html
>
> And listed in the examples and changelog to this v4 patchset:
> https://www.redhat.com/archives/linux-audit/2018-July/msg00178.html
>
> It is also listed in this userspace patchset update v4 (which should
> also have had a changelog added to it, note to self...):
> https://www.redhat.com/archives/linux-audit/2018-July/msg00189.html
>
> I realize it is hard to keep up with all the detail changes in these
> patchsets...
>
> > CONTAINER_ID audit container identifier creation event
> > CONTAINER audit container identifier aux record to an event
> >
> > Or vice versa. Don't mix up creation of the identifier with operations.
>
> Exactly what I'm trying to avoid... Worded another way: "Don't mix up
> the creation operation with routine reporting of the identifier in
> events." Steve, can you and Paul discuss and agree on what they should
> be called? I don't have a horse in this race, but I need to record the
> result of that run. ;-)
See my previous comments, I think I've been pretty clear on what I
would like to see.
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [PATCH ghak90 (was ghak32) V4 03/10] audit: log container info of syscalls
From: Richard Guy Briggs @ 2018-10-25 17:38 UTC (permalink / raw)
To: Steve Grubb
Cc: Paul Moore, simo, carlos, linux-api, containers, linux-kernel,
dhowells, linux-audit, netfilter-devel, ebiederm, luto, netdev,
linux-fsdevel, Eric Paris, Serge Hallyn, viro
In-Reply-To: <20181025175745.5b2b13e9@ivy-bridge>
On 2018-10-25 17:57, Steve Grubb wrote:
> On Thu, 25 Oct 2018 08:27:32 -0400
> Richard Guy Briggs <rgb@redhat.com> wrote:
>
> > On 2018-10-25 06:49, Paul Moore wrote:
> > > On Thu, Oct 25, 2018 at 2:06 AM Steve Grubb <sgrubb@redhat.com>
> > > wrote:
> > > > On Wed, 24 Oct 2018 20:42:55 -0400
> > > > Richard Guy Briggs <rgb@redhat.com> wrote:
> > > > > On 2018-10-24 16:55, Paul Moore wrote:
> > > > > > On Wed, Oct 24, 2018 at 11:15 AM Richard Guy Briggs
> > > > > > <rgb@redhat.com> wrote:
> > > > > > > On 2018-10-19 19:16, Paul Moore wrote:
> > > > > > > > On Sun, Aug 5, 2018 at 4:32 AM Richard Guy Briggs
> > > > > > > > <rgb@redhat.com> wrote:
> > >
> > > ...
> > >
> > > > > > > > > +/*
> > > > > > > > > + * audit_log_contid - report container info
> > > > > > > > > + * @tsk: task to be recorded
> > > > > > > > > + * @context: task or local context for record
> > > > > > > > > + * @op: contid string description
> > > > > > > > > + */
> > > > > > > > > +int audit_log_contid(struct task_struct *tsk,
> > > > > > > > > + struct audit_context
> > > > > > > > > *context, char *op) +{
> > > > > > > > > + struct audit_buffer *ab;
> > > > > > > > > +
> > > > > > > > > + if (!audit_contid_set(tsk))
> > > > > > > > > + return 0;
> > > > > > > > > + /* Generate AUDIT_CONTAINER record with
> > > > > > > > > container ID */
> > > > > > > > > + ab = audit_log_start(context, GFP_KERNEL,
> > > > > > > > > AUDIT_CONTAINER);
> > > > > > > > > + if (!ab)
> > > > > > > > > + return -ENOMEM;
> > > > > > > > > + audit_log_format(ab, "op=%s contid=%llu",
> > > > > > > > > + op, audit_get_contid(tsk));
> > > > > > > > > + audit_log_end(ab);
> > > > > > > > > + return 0;
> > > > > > > > > +}
> > > > > > > > > +EXPORT_SYMBOL(audit_log_contid);
> > > > > > > >
> > > > > > > > As discussed in the previous iteration of the patch, I
> > > > > > > > prefer AUDIT_CONTAINER_ID here over AUDIT_CONTAINER. If
> > > > > > > > you feel strongly about keeping it as-is with
> > > > > > > > AUDIT_CONTAINER I suppose I could live with that, but it
> > > > > > > > is isn't my first choice.
> > > > > > >
> > > > > > > I don't have a strong opinion on this one, mildly
> > > > > > > preferring the shorter one only because it is shorter.
> > > > > >
> > > > > > We already have multiple AUDIT_CONTAINER* record types, so it
> > > > > > seems as though we should use "AUDIT_CONTAINER" as a prefix
> > > > > > of sorts, rather than a type itself.
> > > > >
> > > > > I'm fine with that. I'd still like to hear Steve's input. He
> > > > > had stronger opinions than me.
> > > >
> > > > The creation event should be separate and distinct from the
> > > > continuing use when its used as a supplemental record. IOW,
> > > > binding the ID to a container is part of the lifecycle and needs
> > > > to be kept distinct.
> > >
> > > Steve's comment is pretty ambiguous when it comes to AUDIT_CONTAINER
> > > vs AUDIT_CONTAINER_ID, but one could argue that AUDIT_CONTAINER_ID
> > > helps distinguish the audit container id marking record and gets to
> > > what I believe is the spirit of Steve's comment. Taking this in
> > > context with my previous remarks, let's switch to using
> > > AUDIT_CONTAINER_ID.
> >
> > I suspect Steve is mixing up AUDIT_CONTAINER_OP with
> > AUDIT_CONTAINER_ID, confusing the fact that they are two seperate
> > records. As a summary, the suggested records are:
> > CONTAINER_OP audit container identifier creation
> > CONTAINER audit container identifier aux record to an
> > event
> >
> > and what Paul is suggesting (which is fine by me) is:
> > CONTAINER_OP audit container identifier creation event
> > CONTAINER_ID audit container identifier aux record to
> > an event
> >
> > Steve, please indicate you are fine with this.
>
> I thought it was:
It *was*. It was changed at Paul's request in this v3 thread:
https://www.redhat.com/archives/linux-audit/2018-July/msg00087.html
And listed in the examples and changelog to this v4 patchset:
https://www.redhat.com/archives/linux-audit/2018-July/msg00178.html
It is also listed in this userspace patchset update v4 (which should
also have had a changelog added to it, note to self...):
https://www.redhat.com/archives/linux-audit/2018-July/msg00189.html
I realize it is hard to keep up with all the detail changes in these
patchsets...
> CONTAINER_ID audit container identifier creation event
> CONTAINER audit container identifier aux record to an event
>
> Or vice versa. Don't mix up creation of the identifier with operations.
Exactly what I'm trying to avoid... Worded another way: "Don't mix up
the creation operation with routine reporting of the identifier in
events." Steve, can you and Paul discuss and agree on what they should
be called? I don't have a horse in this race, but I need to record the
result of that run. ;-)
> -Steve
- RGB
--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635
^ permalink raw reply
* Re: [PATCH] seccomp: Add pkru into seccomp_data
From: Michael Sammler @ 2018-10-25 16:42 UTC (permalink / raw)
To: Florian Weimer
Cc: Will Drewry, Kees Cook, linux-api, Ram Pai, Andy Lutomirski,
linuxppc-dev
In-Reply-To: <875zxqo0ee.fsf@oldenburg.str.redhat.com>
On 10/25/2018 11:12 AM, Florian Weimer wrote:
>> I understand your concern about exposing the number of protection keys
>> in the ABI. One idea would be to state, that the pkru field (which
>> should probably be renamed) contains an architecture specific value,
>> which could then be the PKRU on x86 and AMR (or another register) on
>> POWER. This new field should probably be extended to __u64 and the
>> reserved field removed.
> POWER also has proper read/write bit separation, not PKEY_DISABLE_ACCESS
> (disable read and write) and PKEY_DISABLE_WRITE like Intel. It's
> currently translated by the kernel, but I really need a
> PKEY_DISABLE_READ bit in glibc to implement pkey_get in case the memory
> is write-only.
The idea here would be to simply provide the raw value of the register
(PKRU on x86, AMR on POWER) to the BPF program and let the BPF program
(or maybe a higher level library like libseccomp) deal with the
complications of interpreting this architecture specific value (similar
how the BPF program currently already has to deal with architecture
specific system call numbers). If an architecture were to support more
protection keys than fit into the field, the architecture specific value
stored in the field might simply be the first protection keys. If there
was interest, it would be possible to add more architecture specific
fields to seccomp_data.
>> Another idea would be to not add a field in the seccomp_data
>> structure, but instead provide a new BPF instruction, which reads the
>> value of a specified protection key.
> I would prefer that if it's possible. We should make sure that the bits
> are the same as those returned from pkey_get. I have an implementation
> on POWER, but have yet to figure out the implications for 32-bit because
> I do not know the AMR register size there.
>
> Thanks,
> Florian
I have had a look at how BPF is implemented and it does not seem to be
easy to just add an BPF instruction for seccomp since (as far as I
understand) the code of the classical BPF (as used by seccomp) is shared
with the code of eBPF, which is used in many parts of the kernel and
there is at least one interpreter and one JIT compiler for BPF. But
maybe someone with more experience than me can comment on how hard it
would be to add an instruction to BPF.
- Michael
^ permalink raw reply
* Re: [PATCH ghak90 (was ghak32) V4 03/10] audit: log container info of syscalls
From: Steve Grubb @ 2018-10-25 15:57 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: Paul Moore, simo, carlos, linux-api, containers, linux-kernel,
dhowells, linux-audit, netfilter-devel, ebiederm, luto, netdev,
linux-fsdevel, Eric Paris, Serge Hallyn, viro
In-Reply-To: <20181025122732.4j4rbychjse3gemt@madcap2.tricolour.ca>
On Thu, 25 Oct 2018 08:27:32 -0400
Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2018-10-25 06:49, Paul Moore wrote:
> > On Thu, Oct 25, 2018 at 2:06 AM Steve Grubb <sgrubb@redhat.com>
> > wrote:
> > > On Wed, 24 Oct 2018 20:42:55 -0400
> > > Richard Guy Briggs <rgb@redhat.com> wrote:
> > > > On 2018-10-24 16:55, Paul Moore wrote:
> > > > > On Wed, Oct 24, 2018 at 11:15 AM Richard Guy Briggs
> > > > > <rgb@redhat.com> wrote:
> > > > > > On 2018-10-19 19:16, Paul Moore wrote:
> > > > > > > On Sun, Aug 5, 2018 at 4:32 AM Richard Guy Briggs
> > > > > > > <rgb@redhat.com> wrote:
> >
> > ...
> >
> > > > > > > > +/*
> > > > > > > > + * audit_log_contid - report container info
> > > > > > > > + * @tsk: task to be recorded
> > > > > > > > + * @context: task or local context for record
> > > > > > > > + * @op: contid string description
> > > > > > > > + */
> > > > > > > > +int audit_log_contid(struct task_struct *tsk,
> > > > > > > > + struct audit_context
> > > > > > > > *context, char *op) +{
> > > > > > > > + struct audit_buffer *ab;
> > > > > > > > +
> > > > > > > > + if (!audit_contid_set(tsk))
> > > > > > > > + return 0;
> > > > > > > > + /* Generate AUDIT_CONTAINER record with
> > > > > > > > container ID */
> > > > > > > > + ab = audit_log_start(context, GFP_KERNEL,
> > > > > > > > AUDIT_CONTAINER);
> > > > > > > > + if (!ab)
> > > > > > > > + return -ENOMEM;
> > > > > > > > + audit_log_format(ab, "op=%s contid=%llu",
> > > > > > > > + op, audit_get_contid(tsk));
> > > > > > > > + audit_log_end(ab);
> > > > > > > > + return 0;
> > > > > > > > +}
> > > > > > > > +EXPORT_SYMBOL(audit_log_contid);
> > > > > > >
> > > > > > > As discussed in the previous iteration of the patch, I
> > > > > > > prefer AUDIT_CONTAINER_ID here over AUDIT_CONTAINER. If
> > > > > > > you feel strongly about keeping it as-is with
> > > > > > > AUDIT_CONTAINER I suppose I could live with that, but it
> > > > > > > is isn't my first choice.
> > > > > >
> > > > > > I don't have a strong opinion on this one, mildly
> > > > > > preferring the shorter one only because it is shorter.
> > > > >
> > > > > We already have multiple AUDIT_CONTAINER* record types, so it
> > > > > seems as though we should use "AUDIT_CONTAINER" as a prefix
> > > > > of sorts, rather than a type itself.
> > > >
> > > > I'm fine with that. I'd still like to hear Steve's input. He
> > > > had stronger opinions than me.
> > >
> > > The creation event should be separate and distinct from the
> > > continuing use when its used as a supplemental record. IOW,
> > > binding the ID to a container is part of the lifecycle and needs
> > > to be kept distinct.
> >
> > Steve's comment is pretty ambiguous when it comes to AUDIT_CONTAINER
> > vs AUDIT_CONTAINER_ID, but one could argue that AUDIT_CONTAINER_ID
> > helps distinguish the audit container id marking record and gets to
> > what I believe is the spirit of Steve's comment. Taking this in
> > context with my previous remarks, let's switch to using
> > AUDIT_CONTAINER_ID.
>
> I suspect Steve is mixing up AUDIT_CONTAINER_OP with
> AUDIT_CONTAINER_ID, confusing the fact that they are two seperate
> records. As a summary, the suggested records are:
> CONTAINER_OP audit container identifier creation
> CONTAINER audit container identifier aux record to an
> event
>
> and what Paul is suggesting (which is fine by me) is:
> CONTAINER_OP audit container identifier creation event
> CONTAINER_ID audit container identifier aux record to
> an event
>
> Steve, please indicate you are fine with this.
I thought it was:
CONTAINER_ID audit container identifier creation event
CONTAINER audit container identifier aux record to an event
Or vice versa. Don't mix up creation of the identifier with operations.
-Steve
^ permalink raw reply
* Re: [PATCH v2 1/3] bpf: allow zero-initializing hash map seed
From: Lorenz Bauer @ 2018-10-25 15:12 UTC (permalink / raw)
To: liu.song.a23; +Cc: Alexei Starovoitov, Daniel Borkmann, netdev, linux-api
In-Reply-To: <CAPhsuW5_1LRw=tjaNWfgrPKaS_Rs12BYAHPOjmXwEs8C9YSy1Q@mail.gmail.com>
On Tue, 9 Oct 2018 at 01:08, Song Liu <liu.song.a23@gmail.com> wrote:
>
> > --- a/include/uapi/linux/bpf.h
> > +++ b/include/uapi/linux/bpf.h
> > @@ -253,6 +253,8 @@ enum bpf_attach_type {
> > #define BPF_F_NO_COMMON_LRU (1U << 1)
> > /* Specify numa node during map creation */
> > #define BPF_F_NUMA_NODE (1U << 2)
> > +/* Zero-initialize hash function seed. This should only be used for testing. */
> > +#define BPF_F_ZERO_SEED (1U << 6)
>
> Please add this line after
> #define BPF_F_STACK_BUILD_ID (1U << 5)
I wanted to keep the flags for BPF_MAP_CREATE grouped together.
Maybe the correct value is (1U << 3)? It seemed like the other flags
were allocated to avoid
overlap between different BPF commands, however, so I tried to follow suit.
--
Lorenz Bauer | Systems Engineer
25 Lavington St., London SE1 0NZ
www.cloudflare.com
^ permalink raw reply
* Re: [PATCH v2 2/3] tools: sync linux/bpf.h
From: Lorenz Bauer @ 2018-10-25 15:07 UTC (permalink / raw)
To: liu.song.a23; +Cc: Alexei Starovoitov, Daniel Borkmann, netdev, linux-api
In-Reply-To: <CAPhsuW5fbhQ1SBurL2q6hm=yV0a_Vw-_gjsz8LHJ76QwcqVXsQ@mail.gmail.com>
On Tue, 9 Oct 2018 at 01:12, Song Liu <liu.song.a23@gmail.com> wrote:
>
> On Mon, Oct 8, 2018 at 3:34 AM Lorenz Bauer <lmb@cloudflare.com> wrote:
> >
> > Synchronize changes to linux/bpf.h from
> > commit 88db241b34bf ("bpf: allow zero-initializing hash map seed").
> I guess we cannot keep this hash during git-am? We probably don't
> need this hash anyway, as the two patches will be applied back to back.
I copied what was done in one of the previous commits that synced the
header. I'm a bit at a
loss what to put in the commit message otherwise.
--
Lorenz Bauer | Systems Engineer
25 Lavington St., London SE1 0NZ
www.cloudflare.com
^ permalink raw reply
* Re: [PATCH ghak90 (was ghak32) V4 03/10] audit: log container info of syscalls
From: Richard Guy Briggs @ 2018-10-25 12:27 UTC (permalink / raw)
To: Paul Moore
Cc: luto, carlos, linux-api, containers, linux-kernel, viro, dhowells,
linux-audit, netfilter-devel, ebiederm, simo, netdev,
linux-fsdevel, Eric Paris, Serge Hallyn
In-Reply-To: <CAHC9VhR02siURgadQs0EikcOm0d_TPAi4jJYCx4NkA=wQ5rjSA@mail.gmail.com>
On 2018-10-25 06:49, Paul Moore wrote:
> On Thu, Oct 25, 2018 at 2:06 AM Steve Grubb <sgrubb@redhat.com> wrote:
> > On Wed, 24 Oct 2018 20:42:55 -0400
> > Richard Guy Briggs <rgb@redhat.com> wrote:
> > > On 2018-10-24 16:55, Paul Moore wrote:
> > > > On Wed, Oct 24, 2018 at 11:15 AM Richard Guy Briggs
> > > > <rgb@redhat.com> wrote:
> > > > > On 2018-10-19 19:16, Paul Moore wrote:
> > > > > > On Sun, Aug 5, 2018 at 4:32 AM Richard Guy Briggs
> > > > > > <rgb@redhat.com> wrote:
>
> ...
>
> > > > > > > +/*
> > > > > > > + * audit_log_contid - report container info
> > > > > > > + * @tsk: task to be recorded
> > > > > > > + * @context: task or local context for record
> > > > > > > + * @op: contid string description
> > > > > > > + */
> > > > > > > +int audit_log_contid(struct task_struct *tsk,
> > > > > > > + struct audit_context *context,
> > > > > > > char *op) +{
> > > > > > > + struct audit_buffer *ab;
> > > > > > > +
> > > > > > > + if (!audit_contid_set(tsk))
> > > > > > > + return 0;
> > > > > > > + /* Generate AUDIT_CONTAINER record with container ID
> > > > > > > */
> > > > > > > + ab = audit_log_start(context, GFP_KERNEL,
> > > > > > > AUDIT_CONTAINER);
> > > > > > > + if (!ab)
> > > > > > > + return -ENOMEM;
> > > > > > > + audit_log_format(ab, "op=%s contid=%llu",
> > > > > > > + op, audit_get_contid(tsk));
> > > > > > > + audit_log_end(ab);
> > > > > > > + return 0;
> > > > > > > +}
> > > > > > > +EXPORT_SYMBOL(audit_log_contid);
> > > > > >
> > > > > > As discussed in the previous iteration of the patch, I prefer
> > > > > > AUDIT_CONTAINER_ID here over AUDIT_CONTAINER. If you feel
> > > > > > strongly about keeping it as-is with AUDIT_CONTAINER I suppose
> > > > > > I could live with that, but it is isn't my first choice.
> > > > >
> > > > > I don't have a strong opinion on this one, mildly preferring the
> > > > > shorter one only because it is shorter.
> > > >
> > > > We already have multiple AUDIT_CONTAINER* record types, so it seems
> > > > as though we should use "AUDIT_CONTAINER" as a prefix of sorts,
> > > > rather than a type itself.
> > >
> > > I'm fine with that. I'd still like to hear Steve's input. He had
> > > stronger opinions than me.
> >
> > The creation event should be separate and distinct from the continuing
> > use when its used as a supplemental record. IOW, binding the ID to a
> > container is part of the lifecycle and needs to be kept distinct.
>
> Steve's comment is pretty ambiguous when it comes to AUDIT_CONTAINER
> vs AUDIT_CONTAINER_ID, but one could argue that AUDIT_CONTAINER_ID
> helps distinguish the audit container id marking record and gets to
> what I believe is the spirit of Steve's comment. Taking this in
> context with my previous remarks, let's switch to using
> AUDIT_CONTAINER_ID.
I suspect Steve is mixing up AUDIT_CONTAINER_OP with AUDIT_CONTAINER_ID,
confusing the fact that they are two seperate records. As a summary,
the suggested records are:
CONTAINER_OP audit container identifier creation
CONTAINER audit container identifier aux record to an event
and what Paul is suggesting (which is fine by me) is:
CONTAINER_OP audit container identifier creation event
CONTAINER_ID audit container identifier aux record to an event
Steve, please indicate you are fine with this.
> paul moore
- RGB
--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635
^ permalink raw reply
* Re: [PATCH ghak90 (was ghak32) V4 03/10] audit: log container info of syscalls
From: Richard Guy Briggs @ 2018-10-25 12:22 UTC (permalink / raw)
To: Paul Moore
Cc: containers, linux-api, linux-audit, linux-fsdevel, linux-kernel,
netdev, netfilter-devel, ebiederm, luto, carlos, dhowells, viro,
simo, Eric Paris, Serge Hallyn
In-Reply-To: <166a9dae538.280e.85c95baa4474aabc7814e68940a78392@paul-moore.com>
On 2018-10-25 07:13, Paul Moore wrote:
> On October 25, 2018 1:43:16 AM Richard Guy Briggs <rgb@redhat.com> wrote:
> > On 2018-10-24 16:55, Paul Moore wrote:
> >> On Wed, Oct 24, 2018 at 11:15 AM Richard Guy Briggs <rgb@redhat.com> wrote:
> >>> On 2018-10-19 19:16, Paul Moore wrote:
> >>>> On Sun, Aug 5, 2018 at 4:32 AM Richard Guy Briggs <rgb@redhat.com> wrote:
> >
>
> ...
>
> >
> >>>> However, I do care about the "op" field in this record. It just
> >>>> doesn't make any sense; the way you are using it it is more of a
> >>>> context field than an operations field, and even then why is the
> >>>> context important from a logging and/or security perspective? Drop it
> >>>> please.
> >>>
> >>> I'll rename it to whatever you like. I'd suggest "ref=". The reason I
> >>> think it is important is there are multiple sources that aren't always
> >>> obvious from the other records to which it is associated. In the case
> >>> of ptrace and signals, there can be many target tasks listed (OBJ_PID)
> >>> with no other way to distinguish the matching audit container identifier
> >>> records all for one event. This is in addition to the default syscall
> >>> container identifier record. I'm not currently happy with the text
> >>> content to link the two, but that should be solvable (most obvious is
> >>> taret PID). Throwing away this information seems shortsighted.
> >>
> >> It would be helpful if you could generate real audit events
> >> demonstrating the problems you are describing, as well as a more
> >> standard syscall event, so we can discuss some possible solutions.
> >
> > If the auditted process is in a container and it ptraces or signals
> > another process in a container, there will be two AUDIT_CONTAINER
> > records for the same event that won't be identified as to which record
> > belongs to which process or other record (SYSCALL vs 1+ OBJ_PID
> > records). There could be many signals recorded, each with their own
> > OBJ_PID record. The first is stored in the audit context and additional
> > ones are stored in a chained struct that can accommodate 16 entries each.
> >
> > (See audit_signal_info(), __audit_ptrace().)
> >
> > (As a side note, on code inspection it appears that a signal target
> > would get overwritten by a ptrace action if they were to happen in that
> > order.)
>
> As requested above, please respond with real audit events generated by
> this patchset so that we can discuss possible solutions.
Ok, then we should be developping a test to test ptrace and signal
auditting in general since we don't have current experience/evidence
that those even work (or rip them out if not).
> paul moore
- RGB
--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635
^ permalink raw reply
* Re: [PATCH ghak90 (was ghak32) V4 03/10] audit: log container info of syscalls
From: Paul Moore @ 2018-10-25 10:49 UTC (permalink / raw)
To: sgrubb
Cc: rgb, simo, carlos, linux-api, containers, linux-kernel, dhowells,
linux-audit, netfilter-devel, ebiederm, luto, netdev,
linux-fsdevel, Eric Paris, Serge Hallyn, viro
In-Reply-To: <20181025080638.771621a3@ivy-bridge>
On Thu, Oct 25, 2018 at 2:06 AM Steve Grubb <sgrubb@redhat.com> wrote:
> On Wed, 24 Oct 2018 20:42:55 -0400
> Richard Guy Briggs <rgb@redhat.com> wrote:
> > On 2018-10-24 16:55, Paul Moore wrote:
> > > On Wed, Oct 24, 2018 at 11:15 AM Richard Guy Briggs
> > > <rgb@redhat.com> wrote:
> > > > On 2018-10-19 19:16, Paul Moore wrote:
> > > > > On Sun, Aug 5, 2018 at 4:32 AM Richard Guy Briggs
> > > > > <rgb@redhat.com> wrote:
...
> > > > > > +/*
> > > > > > + * audit_log_contid - report container info
> > > > > > + * @tsk: task to be recorded
> > > > > > + * @context: task or local context for record
> > > > > > + * @op: contid string description
> > > > > > + */
> > > > > > +int audit_log_contid(struct task_struct *tsk,
> > > > > > + struct audit_context *context,
> > > > > > char *op) +{
> > > > > > + struct audit_buffer *ab;
> > > > > > +
> > > > > > + if (!audit_contid_set(tsk))
> > > > > > + return 0;
> > > > > > + /* Generate AUDIT_CONTAINER record with container ID
> > > > > > */
> > > > > > + ab = audit_log_start(context, GFP_KERNEL,
> > > > > > AUDIT_CONTAINER);
> > > > > > + if (!ab)
> > > > > > + return -ENOMEM;
> > > > > > + audit_log_format(ab, "op=%s contid=%llu",
> > > > > > + op, audit_get_contid(tsk));
> > > > > > + audit_log_end(ab);
> > > > > > + return 0;
> > > > > > +}
> > > > > > +EXPORT_SYMBOL(audit_log_contid);
> > > > >
> > > > > As discussed in the previous iteration of the patch, I prefer
> > > > > AUDIT_CONTAINER_ID here over AUDIT_CONTAINER. If you feel
> > > > > strongly about keeping it as-is with AUDIT_CONTAINER I suppose
> > > > > I could live with that, but it is isn't my first choice.
> > > >
> > > > I don't have a strong opinion on this one, mildly preferring the
> > > > shorter one only because it is shorter.
> > >
> > > We already have multiple AUDIT_CONTAINER* record types, so it seems
> > > as though we should use "AUDIT_CONTAINER" as a prefix of sorts,
> > > rather than a type itself.
> >
> > I'm fine with that. I'd still like to hear Steve's input. He had
> > stronger opinions than me.
>
> The creation event should be separate and distinct from the continuing
> use when its used as a supplemental record. IOW, binding the ID to a
> container is part of the lifecycle and needs to be kept distinct.
Steve's comment is pretty ambiguous when it comes to AUDIT_CONTAINER
vs AUDIT_CONTAINER_ID, but one could argue that AUDIT_CONTAINER_ID
helps distinguish the audit container id marking record and gets to
what I believe is the spirit of Steve's comment. Taking this in
context with my previous remarks, let's switch to using
AUDIT_CONTAINER_ID.
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [PATCH] seccomp: Add pkru into seccomp_data
From: Florian Weimer @ 2018-10-25 9:12 UTC (permalink / raw)
To: Michael Sammler
Cc: Will Drewry, Kees Cook, linux-api, Ram Pai, Andy Lutomirski,
linuxppc-dev
In-Reply-To: <11a706bd-060f-41de-118b-bababfd83b3d@mpi-sws.org>
* Michael Sammler:
> Thank you for the pointer about the POWER implementation. I am not
> familiar with POWER in general and its protection key feature at
> all. Would the AMR register be the correct register to expose here?
Yes, according to my notes, the register is called AMR (special purpose
register 13).
> I understand your concern about exposing the number of protection keys
> in the ABI. One idea would be to state, that the pkru field (which
> should probably be renamed) contains an architecture specific value,
> which could then be the PKRU on x86 and AMR (or another register) on
> POWER. This new field should probably be extended to __u64 and the
> reserved field removed.
POWER also has proper read/write bit separation, not PKEY_DISABLE_ACCESS
(disable read and write) and PKEY_DISABLE_WRITE like Intel. It's
currently translated by the kernel, but I really need a
PKEY_DISABLE_READ bit in glibc to implement pkey_get in case the memory
is write-only.
> Another idea would be to not add a field in the seccomp_data
> structure, but instead provide a new BPF instruction, which reads the
> value of a specified protection key.
I would prefer that if it's possible. We should make sure that the bits
are the same as those returned from pkey_get. I have an implementation
on POWER, but have yet to figure out the implications for 32-bit because
I do not know the AMR register size there.
Thanks,
Florian
^ permalink raw reply
* Re: [PATCH] seccomp: Add pkru into seccomp_data
From: Michael Sammler @ 2018-10-25 8:39 UTC (permalink / raw)
To: Florian Weimer
Cc: Will Drewry, Kees Cook, linux-api, Ram Pai, Andy Lutomirski,
linuxppc-dev
In-Reply-To: <87zhv3nrr0.fsf@oldenburg.str.redhat.com>
On 10/24/2018 08:06 PM, Florian Weimer wrote:
> * Michael Sammler:
>
>> Add the current value of the PKRU register to data available for
>> seccomp-bpf programs to work on. This allows filters based on the
>> currently enabled protection keys.
>> diff --git a/include/uapi/linux/seccomp.h b/include/uapi/linux/seccomp.h
>> index 9efc0e73..e8b9ecfc 100644
>> --- a/include/uapi/linux/seccomp.h
>> +++ b/include/uapi/linux/seccomp.h
>> @@ -52,12 +52,16 @@
>> * @instruction_pointer: at the time of the system call.
>> * @args: up to 6 system call arguments always stored as 64-bit values
>> * regardless of the architecture.
>> + * @pkru: value of the pkru register
>> + * @reserved: pad the structure to a multiple of eight bytes
>> */
>> struct seccomp_data {
>> int nr;
>> __u32 arch;
>> __u64 instruction_pointer;
>> __u64 args[6];
>> + __u32 pkru;
>> + __u32 reserved;
>> };
> This doesn't cover the POWER implementation. Adding Cc:s.
>
> And I think the kernel shouldn't expose the number of protection keys in
> the ABI.
>
> Thanks,
> Florian
Thank you for the pointer about the POWER implementation. I am not
familiar with POWER in general and its protection key feature at all.
Would the AMR register be the correct register to expose here?
I understand your concern about exposing the number of protection keys
in the ABI. One idea would be to state, that the pkru field (which
should probably be renamed) contains an architecture specific value,
which could then be the PKRU on x86 and AMR (or another register) on
POWER. This new field should probably be extended to __u64 and the
reserved field removed.
Another idea would be to not add a field in the seccomp_data structure,
but instead provide a new BPF instruction, which reads the value of a
specified protection key.
- Michael
^ permalink raw reply
* Re: [PATCH ghak90 (was ghak32) V4 03/10] audit: log container info of syscalls
From: Paul Moore @ 2018-10-25 6:13 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: containers, linux-api, linux-audit, linux-fsdevel, linux-kernel,
netdev, netfilter-devel, ebiederm, luto, carlos, dhowells, viro,
simo, Eric Paris, Serge Hallyn
In-Reply-To: <20181025004255.zl7p7j6gztouh2hh@madcap2.tricolour.ca>
On October 25, 2018 1:43:16 AM Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2018-10-24 16:55, Paul Moore wrote:
>> On Wed, Oct 24, 2018 at 11:15 AM Richard Guy Briggs <rgb@redhat.com> wrote:
>>> On 2018-10-19 19:16, Paul Moore wrote:
>>>> On Sun, Aug 5, 2018 at 4:32 AM Richard Guy Briggs <rgb@redhat.com> wrote:
>
...
>
>>>> However, I do care about the "op" field in this record. It just
>>>> doesn't make any sense; the way you are using it it is more of a
>>>> context field than an operations field, and even then why is the
>>>> context important from a logging and/or security perspective? Drop it
>>>> please.
>>>
>>> I'll rename it to whatever you like. I'd suggest "ref=". The reason I
>>> think it is important is there are multiple sources that aren't always
>>> obvious from the other records to which it is associated. In the case
>>> of ptrace and signals, there can be many target tasks listed (OBJ_PID)
>>> with no other way to distinguish the matching audit container identifier
>>> records all for one event. This is in addition to the default syscall
>>> container identifier record. I'm not currently happy with the text
>>> content to link the two, but that should be solvable (most obvious is
>>> taret PID). Throwing away this information seems shortsighted.
>>
>> It would be helpful if you could generate real audit events
>> demonstrating the problems you are describing, as well as a more
>> standard syscall event, so we can discuss some possible solutions.
>
> If the auditted process is in a container and it ptraces or signals
> another process in a container, there will be two AUDIT_CONTAINER
> records for the same event that won't be identified as to which record
> belongs to which process or other record (SYSCALL vs 1+ OBJ_PID
> records). There could be many signals recorded, each with their own
> OBJ_PID record. The first is stored in the audit context and additional
> ones are stored in a chained struct that can accommodate 16 entries each.
>
> (See audit_signal_info(), __audit_ptrace().)
>
> (As a side note, on code inspection it appears that a signal target
> would get overwritten by a ptrace action if they were to happen in that
> order.)
As requested above, please respond with real audit events generated by this patchset so that we can discuss possible solutions.
^ permalink raw reply
* Re: [PATCH ghak90 (was ghak32) V4 03/10] audit: log container info of syscalls
From: Steve Grubb @ 2018-10-25 6:06 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: Paul Moore, simo, carlos, linux-api, containers, linux-kernel,
dhowells, linux-audit, netfilter-devel, ebiederm, luto, netdev,
linux-fsdevel, Eric Paris, Serge Hallyn, viro
In-Reply-To: <20181025004255.zl7p7j6gztouh2hh@madcap2.tricolour.ca>
On Wed, 24 Oct 2018 20:42:55 -0400
Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2018-10-24 16:55, Paul Moore wrote:
> > On Wed, Oct 24, 2018 at 11:15 AM Richard Guy Briggs
> > <rgb@redhat.com> wrote:
> > > On 2018-10-19 19:16, Paul Moore wrote:
> > > > On Sun, Aug 5, 2018 at 4:32 AM Richard Guy Briggs
> > > > <rgb@redhat.com> wrote:
> > > > > Create a new audit record AUDIT_CONTAINER to document the
> > > > > audit container identifier of a process if it is present.
> > > > >
> > > > > Called from audit_log_exit(), syscalls are covered.
> > > > >
> > > > > A sample raw event:
> > > > > type=SYSCALL msg=audit(1519924845.499:257): arch=c000003e
> > > > > syscall=257 success=yes exit=3 a0=ffffff9c a1=56374e1cef30
> > > > > a2=241 a3=1b6 items=2 ppid=606 pid=635 auid=0 uid=0 gid=0
> > > > > euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0 tty=pts0 ses=3
> > > > > comm="bash" exe="/usr/bin/bash"
> > > > > subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023
> > > > > key="tmpcontainerid" type=CWD msg=audit(1519924845.499:257):
> > > > > cwd="/root" type=PATH msg=audit(1519924845.499:257): item=0
> > > > > name="/tmp/" inode=13863 dev=00:27 mode=041777 ouid=0 ogid=0
> > > > > rdev=00:00 obj=system_u:object_r:tmp_t:s0 nametype= PARENT
> > > > > cap_fp=0000000000000000 cap_fi=0000000000000000 cap_fe=0
> > > > > cap_fver=0 type=PATH msg=audit(1519924845.499:257): item=1
> > > > > name="/tmp/tmpcontainerid" inode=17729 dev=00:27 mode=0100644
> > > > > ouid=0 ogid=0 rdev=00:00
> > > > > obj=unconfined_u:object_r:user_tmp_t:s0 nametype=CREATE
> > > > > cap_fp=0000000000000000 cap_fi=0000000000000000 cap_fe=0
> > > > > cap_fver=0 type=PROCTITLE msg=audit(1519924845.499:257):
> > > > > proctitle=62617368002D6300736C65657020313B206563686F2074657374203E202F746D702F746D70636F6E7461696E65726964
> > > > > type=CONTAINER msg=audit(1519924845.499:257): op=task
> > > > > contid=123458
> > > > >
> > > > > See: https://github.com/linux-audit/audit-kernel/issues/90
> > > > > See: https://github.com/linux-audit/audit-userspace/issues/51
> > > > > See: https://github.com/linux-audit/audit-testsuite/issues/64
> > > > > See:
> > > > > https://github.com/linux-audit/audit-kernel/wiki/RFE-Audit-Container-ID
> > > > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com> Acked-by:
> > > > > Serge Hallyn <serge@hallyn.com> Acked-by: Steve Grubb
> > > > > <sgrubb@redhat.com> ---
> > > > > include/linux/audit.h | 7 +++++++
> > > > > include/uapi/linux/audit.h | 1 +
> > > > > kernel/audit.c | 24 ++++++++++++++++++++++++
> > > > > kernel/auditsc.c | 3 +++
> > > > > 4 files changed, 35 insertions(+)
> > > >
> > > > ...
> > > >
> > > > > @@ -2045,6 +2045,30 @@ void audit_log_session_info(struct
> > > > > audit_buffer *ab) audit_log_format(ab, " auid=%u ses=%u",
> > > > > auid, sessionid); }
> > > > >
> > > > > +/*
> > > > > + * audit_log_contid - report container info
> > > > > + * @tsk: task to be recorded
> > > > > + * @context: task or local context for record
> > > > > + * @op: contid string description
> > > > > + */
> > > > > +int audit_log_contid(struct task_struct *tsk,
> > > > > + struct audit_context *context,
> > > > > char *op) +{
> > > > > + struct audit_buffer *ab;
> > > > > +
> > > > > + if (!audit_contid_set(tsk))
> > > > > + return 0;
> > > > > + /* Generate AUDIT_CONTAINER record with container ID
> > > > > */
> > > > > + ab = audit_log_start(context, GFP_KERNEL,
> > > > > AUDIT_CONTAINER);
> > > > > + if (!ab)
> > > > > + return -ENOMEM;
> > > > > + audit_log_format(ab, "op=%s contid=%llu",
> > > > > + op, audit_get_contid(tsk));
> > > > > + audit_log_end(ab);
> > > > > + return 0;
> > > > > +}
> > > > > +EXPORT_SYMBOL(audit_log_contid);
> > > >
> > > > As discussed in the previous iteration of the patch, I prefer
> > > > AUDIT_CONTAINER_ID here over AUDIT_CONTAINER. If you feel
> > > > strongly about keeping it as-is with AUDIT_CONTAINER I suppose
> > > > I could live with that, but it is isn't my first choice.
> > >
> > > I don't have a strong opinion on this one, mildly preferring the
> > > shorter one only because it is shorter.
> >
> > We already have multiple AUDIT_CONTAINER* record types, so it seems
> > as though we should use "AUDIT_CONTAINER" as a prefix of sorts,
> > rather than a type itself.
>
> I'm fine with that. I'd still like to hear Steve's input. He had
> stronger opinions than me.
The creation event should be separate and distinct from the continuing
use when its used as a supplemental record. IOW, binding the ID to a
container is part of the lifecycle and needs to be kept distinct.
-Steve
> > > > However, I do care about the "op" field in this record. It just
> > > > doesn't make any sense; the way you are using it it is more of a
> > > > context field than an operations field, and even then why is the
> > > > context important from a logging and/or security perspective?
> > > > Drop it please.
> > >
> > > I'll rename it to whatever you like. I'd suggest "ref=". The
> > > reason I think it is important is there are multiple sources that
> > > aren't always obvious from the other records to which it is
> > > associated. In the case of ptrace and signals, there can be many
> > > target tasks listed (OBJ_PID) with no other way to distinguish
> > > the matching audit container identifier records all for one
> > > event. This is in addition to the default syscall container
> > > identifier record. I'm not currently happy with the text content
> > > to link the two, but that should be solvable (most obvious is
> > > taret PID). Throwing away this information seems shortsighted.
> >
> > It would be helpful if you could generate real audit events
> > demonstrating the problems you are describing, as well as a more
> > standard syscall event, so we can discuss some possible solutions.
>
> If the auditted process is in a container and it ptraces or signals
> another process in a container, there will be two AUDIT_CONTAINER
> records for the same event that won't be identified as to which record
> belongs to which process or other record (SYSCALL vs 1+ OBJ_PID
> records). There could be many signals recorded, each with their own
> OBJ_PID record. The first is stored in the audit context and
> additional ones are stored in a chained struct that can accommodate
> 16 entries each.
>
> (See audit_signal_info(), __audit_ptrace().)
>
> (As a side note, on code inspection it appears that a signal target
> would get overwritten by a ptrace action if they were to happen in
> that order.)
>
> > paul moore
>
> - RGB
>
> --
> Richard Guy Briggs <rgb@redhat.com>
> Sr. S/W Engineer, Kernel Security, Base Operating Systems
> Remote, Ottawa, Red Hat Canada
> IRC: rgb, SunRaycer
> Voice: +1.647.777.2635, Internal: (81) 32635
>
> --
> Linux-audit mailing list
> Linux-audit@redhat.com
> https://www.redhat.com/mailman/listinfo/linux-audit
^ permalink raw reply
* Re: [PATCH ghak90 (was ghak32) V4 03/10] audit: log container info of syscalls
From: Richard Guy Briggs @ 2018-10-25 0:42 UTC (permalink / raw)
To: Paul Moore
Cc: containers, linux-api, linux-audit, linux-fsdevel, linux-kernel,
netdev, netfilter-devel, ebiederm, luto, carlos, dhowells, viro,
simo, Eric Paris, Serge Hallyn
In-Reply-To: <CAHC9VhRF1vsxM-k0Lw-9NqS9b9rgXRRBu0tq4ajdFpMqUxiH4A@mail.gmail.com>
On 2018-10-24 16:55, Paul Moore wrote:
> On Wed, Oct 24, 2018 at 11:15 AM Richard Guy Briggs <rgb@redhat.com> wrote:
> > On 2018-10-19 19:16, Paul Moore wrote:
> > > On Sun, Aug 5, 2018 at 4:32 AM Richard Guy Briggs <rgb@redhat.com> wrote:
> > > > Create a new audit record AUDIT_CONTAINER to document the audit
> > > > container identifier of a process if it is present.
> > > >
> > > > Called from audit_log_exit(), syscalls are covered.
> > > >
> > > > A sample raw event:
> > > > type=SYSCALL msg=audit(1519924845.499:257): arch=c000003e syscall=257 success=yes exit=3 a0=ffffff9c a1=56374e1cef30 a2=241 a3=1b6 items=2 ppid=606 pid=635 auid=0 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0 tty=pts0 ses=3 comm="bash" exe="/usr/bin/bash" subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 key="tmpcontainerid"
> > > > type=CWD msg=audit(1519924845.499:257): cwd="/root"
> > > > type=PATH msg=audit(1519924845.499:257): item=0 name="/tmp/" inode=13863 dev=00:27 mode=041777 ouid=0 ogid=0 rdev=00:00 obj=system_u:object_r:tmp_t:s0 nametype= PARENT cap_fp=0000000000000000 cap_fi=0000000000000000 cap_fe=0 cap_fver=0
> > > > type=PATH msg=audit(1519924845.499:257): item=1 name="/tmp/tmpcontainerid" inode=17729 dev=00:27 mode=0100644 ouid=0 ogid=0 rdev=00:00 obj=unconfined_u:object_r:user_tmp_t:s0 nametype=CREATE cap_fp=0000000000000000 cap_fi=0000000000000000 cap_fe=0 cap_fver=0
> > > > type=PROCTITLE msg=audit(1519924845.499:257): proctitle=62617368002D6300736C65657020313B206563686F2074657374203E202F746D702F746D70636F6E7461696E65726964
> > > > type=CONTAINER msg=audit(1519924845.499:257): op=task contid=123458
> > > >
> > > > See: https://github.com/linux-audit/audit-kernel/issues/90
> > > > See: https://github.com/linux-audit/audit-userspace/issues/51
> > > > See: https://github.com/linux-audit/audit-testsuite/issues/64
> > > > See: https://github.com/linux-audit/audit-kernel/wiki/RFE-Audit-Container-ID
> > > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > > > Acked-by: Serge Hallyn <serge@hallyn.com>
> > > > Acked-by: Steve Grubb <sgrubb@redhat.com>
> > > > ---
> > > > include/linux/audit.h | 7 +++++++
> > > > include/uapi/linux/audit.h | 1 +
> > > > kernel/audit.c | 24 ++++++++++++++++++++++++
> > > > kernel/auditsc.c | 3 +++
> > > > 4 files changed, 35 insertions(+)
> > >
> > > ...
> > >
> > > > @@ -2045,6 +2045,30 @@ void audit_log_session_info(struct audit_buffer *ab)
> > > > audit_log_format(ab, " auid=%u ses=%u", auid, sessionid);
> > > > }
> > > >
> > > > +/*
> > > > + * audit_log_contid - report container info
> > > > + * @tsk: task to be recorded
> > > > + * @context: task or local context for record
> > > > + * @op: contid string description
> > > > + */
> > > > +int audit_log_contid(struct task_struct *tsk,
> > > > + struct audit_context *context, char *op)
> > > > +{
> > > > + struct audit_buffer *ab;
> > > > +
> > > > + if (!audit_contid_set(tsk))
> > > > + return 0;
> > > > + /* Generate AUDIT_CONTAINER record with container ID */
> > > > + ab = audit_log_start(context, GFP_KERNEL, AUDIT_CONTAINER);
> > > > + if (!ab)
> > > > + return -ENOMEM;
> > > > + audit_log_format(ab, "op=%s contid=%llu",
> > > > + op, audit_get_contid(tsk));
> > > > + audit_log_end(ab);
> > > > + return 0;
> > > > +}
> > > > +EXPORT_SYMBOL(audit_log_contid);
> > >
> > > As discussed in the previous iteration of the patch, I prefer
> > > AUDIT_CONTAINER_ID here over AUDIT_CONTAINER. If you feel strongly
> > > about keeping it as-is with AUDIT_CONTAINER I suppose I could live
> > > with that, but it is isn't my first choice.
> >
> > I don't have a strong opinion on this one, mildly preferring the shorter
> > one only because it is shorter.
>
> We already have multiple AUDIT_CONTAINER* record types, so it seems as
> though we should use "AUDIT_CONTAINER" as a prefix of sorts, rather
> than a type itself.
I'm fine with that. I'd still like to hear Steve's input. He had
stronger opinions than me.
> > > However, I do care about the "op" field in this record. It just
> > > doesn't make any sense; the way you are using it it is more of a
> > > context field than an operations field, and even then why is the
> > > context important from a logging and/or security perspective? Drop it
> > > please.
> >
> > I'll rename it to whatever you like. I'd suggest "ref=". The reason I
> > think it is important is there are multiple sources that aren't always
> > obvious from the other records to which it is associated. In the case
> > of ptrace and signals, there can be many target tasks listed (OBJ_PID)
> > with no other way to distinguish the matching audit container identifier
> > records all for one event. This is in addition to the default syscall
> > container identifier record. I'm not currently happy with the text
> > content to link the two, but that should be solvable (most obvious is
> > taret PID). Throwing away this information seems shortsighted.
>
> It would be helpful if you could generate real audit events
> demonstrating the problems you are describing, as well as a more
> standard syscall event, so we can discuss some possible solutions.
If the auditted process is in a container and it ptraces or signals
another process in a container, there will be two AUDIT_CONTAINER
records for the same event that won't be identified as to which record
belongs to which process or other record (SYSCALL vs 1+ OBJ_PID
records). There could be many signals recorded, each with their own
OBJ_PID record. The first is stored in the audit context and additional
ones are stored in a chained struct that can accommodate 16 entries each.
(See audit_signal_info(), __audit_ptrace().)
(As a side note, on code inspection it appears that a signal target
would get overwritten by a ptrace action if they were to happen in that
order.)
> paul moore
- RGB
--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635
^ permalink raw reply
* Re: [PATCH ghak90 (was ghak32) V4 03/10] audit: log container info of syscalls
From: Paul Moore @ 2018-10-24 20:55 UTC (permalink / raw)
To: rgb
Cc: containers, linux-api, linux-audit, linux-fsdevel, linux-kernel,
netdev, netfilter-devel, ebiederm, luto, carlos, dhowells, viro,
simo, Eric Paris, Serge Hallyn
In-Reply-To: <20181024151439.lavhanabsyxdrdvo@madcap2.tricolour.ca>
On Wed, Oct 24, 2018 at 11:15 AM Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2018-10-19 19:16, Paul Moore wrote:
> > On Sun, Aug 5, 2018 at 4:32 AM Richard Guy Briggs <rgb@redhat.com> wrote:
> > > Create a new audit record AUDIT_CONTAINER to document the audit
> > > container identifier of a process if it is present.
> > >
> > > Called from audit_log_exit(), syscalls are covered.
> > >
> > > A sample raw event:
> > > type=SYSCALL msg=audit(1519924845.499:257): arch=c000003e syscall=257 success=yes exit=3 a0=ffffff9c a1=56374e1cef30 a2=241 a3=1b6 items=2 ppid=606 pid=635 auid=0 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0 tty=pts0 ses=3 comm="bash" exe="/usr/bin/bash" subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 key="tmpcontainerid"
> > > type=CWD msg=audit(1519924845.499:257): cwd="/root"
> > > type=PATH msg=audit(1519924845.499:257): item=0 name="/tmp/" inode=13863 dev=00:27 mode=041777 ouid=0 ogid=0 rdev=00:00 obj=system_u:object_r:tmp_t:s0 nametype= PARENT cap_fp=0000000000000000 cap_fi=0000000000000000 cap_fe=0 cap_fver=0
> > > type=PATH msg=audit(1519924845.499:257): item=1 name="/tmp/tmpcontainerid" inode=17729 dev=00:27 mode=0100644 ouid=0 ogid=0 rdev=00:00 obj=unconfined_u:object_r:user_tmp_t:s0 nametype=CREATE cap_fp=0000000000000000 cap_fi=0000000000000000 cap_fe=0 cap_fver=0
> > > type=PROCTITLE msg=audit(1519924845.499:257): proctitle=62617368002D6300736C65657020313B206563686F2074657374203E202F746D702F746D70636F6E7461696E65726964
> > > type=CONTAINER msg=audit(1519924845.499:257): op=task contid=123458
> > >
> > > See: https://github.com/linux-audit/audit-kernel/issues/90
> > > See: https://github.com/linux-audit/audit-userspace/issues/51
> > > See: https://github.com/linux-audit/audit-testsuite/issues/64
> > > See: https://github.com/linux-audit/audit-kernel/wiki/RFE-Audit-Container-ID
> > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > > Acked-by: Serge Hallyn <serge@hallyn.com>
> > > Acked-by: Steve Grubb <sgrubb@redhat.com>
> > > ---
> > > include/linux/audit.h | 7 +++++++
> > > include/uapi/linux/audit.h | 1 +
> > > kernel/audit.c | 24 ++++++++++++++++++++++++
> > > kernel/auditsc.c | 3 +++
> > > 4 files changed, 35 insertions(+)
> >
> > ...
> >
> > > @@ -2045,6 +2045,30 @@ void audit_log_session_info(struct audit_buffer *ab)
> > > audit_log_format(ab, " auid=%u ses=%u", auid, sessionid);
> > > }
> > >
> > > +/*
> > > + * audit_log_contid - report container info
> > > + * @tsk: task to be recorded
> > > + * @context: task or local context for record
> > > + * @op: contid string description
> > > + */
> > > +int audit_log_contid(struct task_struct *tsk,
> > > + struct audit_context *context, char *op)
> > > +{
> > > + struct audit_buffer *ab;
> > > +
> > > + if (!audit_contid_set(tsk))
> > > + return 0;
> > > + /* Generate AUDIT_CONTAINER record with container ID */
> > > + ab = audit_log_start(context, GFP_KERNEL, AUDIT_CONTAINER);
> > > + if (!ab)
> > > + return -ENOMEM;
> > > + audit_log_format(ab, "op=%s contid=%llu",
> > > + op, audit_get_contid(tsk));
> > > + audit_log_end(ab);
> > > + return 0;
> > > +}
> > > +EXPORT_SYMBOL(audit_log_contid);
> >
> > As discussed in the previous iteration of the patch, I prefer
> > AUDIT_CONTAINER_ID here over AUDIT_CONTAINER. If you feel strongly
> > about keeping it as-is with AUDIT_CONTAINER I suppose I could live
> > with that, but it is isn't my first choice.
>
> I don't have a strong opinion on this one, mildly preferring the shorter
> one only because it is shorter.
We already have multiple AUDIT_CONTAINER* record types, so it seems as
though we should use "AUDIT_CONTAINER" as a prefix of sorts, rather
than a type itself.
> > However, I do care about the "op" field in this record. It just
> > doesn't make any sense; the way you are using it it is more of a
> > context field than an operations field, and even then why is the
> > context important from a logging and/or security perspective? Drop it
> > please.
>
> I'll rename it to whatever you like. I'd suggest "ref=". The reason I
> think it is important is there are multiple sources that aren't always
> obvious from the other records to which it is associated. In the case
> of ptrace and signals, there can be many target tasks listed (OBJ_PID)
> with no other way to distinguish the matching audit container identifier
> records all for one event. This is in addition to the default syscall
> container identifier record. I'm not currently happy with the text
> content to link the two, but that should be solvable (most obvious is
> taret PID). Throwing away this information seems shortsighted.
It would be helpful if you could generate real audit events
demonstrating the problems you are describing, as well as a more
standard syscall event, so we can discuss some possible solutions.
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [PATCH] seccomp: Add pkru into seccomp_data
From: Florian Weimer @ 2018-10-24 18:06 UTC (permalink / raw)
To: Michael Sammler
Cc: Will Drewry, Kees Cook, linux-api, Ram Pai, Andy Lutomirski,
linuxppc-dev
In-Reply-To: <20181024153523.10974-1-msammler@mpi-sws.org>
* Michael Sammler:
> Add the current value of the PKRU register to data available for
> seccomp-bpf programs to work on. This allows filters based on the
> currently enabled protection keys.
> diff --git a/include/uapi/linux/seccomp.h b/include/uapi/linux/seccomp.h
> index 9efc0e73..e8b9ecfc 100644
> --- a/include/uapi/linux/seccomp.h
> +++ b/include/uapi/linux/seccomp.h
> @@ -52,12 +52,16 @@
> * @instruction_pointer: at the time of the system call.
> * @args: up to 6 system call arguments always stored as 64-bit values
> * regardless of the architecture.
> + * @pkru: value of the pkru register
> + * @reserved: pad the structure to a multiple of eight bytes
> */
> struct seccomp_data {
> int nr;
> __u32 arch;
> __u64 instruction_pointer;
> __u64 args[6];
> + __u32 pkru;
> + __u32 reserved;
> };
This doesn't cover the POWER implementation. Adding Cc:s.
And I think the kernel shouldn't expose the number of protection keys in
the ABI.
Thanks,
Florian
^ permalink raw reply
* Re: [PATCH v6 1/1] ns: add binfmt_misc to the user namespace
From: Laurent Vivier @ 2018-10-24 17:15 UTC (permalink / raw)
To: Andrei Vagin
Cc: linux-kernel, Jann Horn, James Bottomley, linux-api,
linux-fsdevel, Alexander Viro, Eric Biederman, containers,
Dmitry Safonov
In-Reply-To: <20181016162245.GA7697@gmail.com>
On 16/10/2018 17:22, Andrei Vagin wrote:
> On Wed, Oct 10, 2018 at 06:14:30PM +0200, Laurent Vivier wrote:
>> This patch allows to have a different binfmt_misc configuration
>> for each new user namespace. By default, the binfmt_misc configuration
>> is the one of the previous level, but if the binfmt_misc filesystem is
>> mounted in the new namespace a new empty binfmt instance is created and
>> used in this namespace.
>>
>> For instance, using "unshare" we can start a chroot of another
>> architecture and configure the binfmt_misc interpreter without being root
>> to run the binaries in this chroot.
>>
>> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
>
> Acked-by: Andrei Vagin <avagin@gmail.com>
>
> Thanks,
> Andrei
>
I don't konw who is the maintainer for this part, but is there any
chance to have this merged in 4.20?
Thanks,
Laurent
^ permalink raw reply
* Re: [PATCH ghak90 (was ghak32) V4 03/10] audit: log container info of syscalls
From: Richard Guy Briggs @ 2018-10-24 15:14 UTC (permalink / raw)
To: Paul Moore
Cc: containers, linux-api, linux-audit, linux-fsdevel, linux-kernel,
netdev, netfilter-devel, ebiederm, luto, carlos, dhowells, viro,
simo, Eric Paris, Serge Hallyn
In-Reply-To: <CAHC9VhSv4hWGHiaaOfuBrRQKdp2YG5RtqXQbFXg7j7LNEhO2XQ@mail.gmail.com>
On 2018-10-19 19:16, Paul Moore wrote:
> On Sun, Aug 5, 2018 at 4:32 AM Richard Guy Briggs <rgb@redhat.com> wrote:
> > Create a new audit record AUDIT_CONTAINER to document the audit
> > container identifier of a process if it is present.
> >
> > Called from audit_log_exit(), syscalls are covered.
> >
> > A sample raw event:
> > type=SYSCALL msg=audit(1519924845.499:257): arch=c000003e syscall=257 success=yes exit=3 a0=ffffff9c a1=56374e1cef30 a2=241 a3=1b6 items=2 ppid=606 pid=635 auid=0 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0 tty=pts0 ses=3 comm="bash" exe="/usr/bin/bash" subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 key="tmpcontainerid"
> > type=CWD msg=audit(1519924845.499:257): cwd="/root"
> > type=PATH msg=audit(1519924845.499:257): item=0 name="/tmp/" inode=13863 dev=00:27 mode=041777 ouid=0 ogid=0 rdev=00:00 obj=system_u:object_r:tmp_t:s0 nametype= PARENT cap_fp=0000000000000000 cap_fi=0000000000000000 cap_fe=0 cap_fver=0
> > type=PATH msg=audit(1519924845.499:257): item=1 name="/tmp/tmpcontainerid" inode=17729 dev=00:27 mode=0100644 ouid=0 ogid=0 rdev=00:00 obj=unconfined_u:object_r:user_tmp_t:s0 nametype=CREATE cap_fp=0000000000000000 cap_fi=0000000000000000 cap_fe=0 cap_fver=0
> > type=PROCTITLE msg=audit(1519924845.499:257): proctitle=62617368002D6300736C65657020313B206563686F2074657374203E202F746D702F746D70636F6E7461696E65726964
> > type=CONTAINER msg=audit(1519924845.499:257): op=task contid=123458
> >
> > See: https://github.com/linux-audit/audit-kernel/issues/90
> > See: https://github.com/linux-audit/audit-userspace/issues/51
> > See: https://github.com/linux-audit/audit-testsuite/issues/64
> > See: https://github.com/linux-audit/audit-kernel/wiki/RFE-Audit-Container-ID
> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > Acked-by: Serge Hallyn <serge@hallyn.com>
> > Acked-by: Steve Grubb <sgrubb@redhat.com>
> > ---
> > include/linux/audit.h | 7 +++++++
> > include/uapi/linux/audit.h | 1 +
> > kernel/audit.c | 24 ++++++++++++++++++++++++
> > kernel/auditsc.c | 3 +++
> > 4 files changed, 35 insertions(+)
>
> ...
>
> > @@ -2045,6 +2045,30 @@ void audit_log_session_info(struct audit_buffer *ab)
> > audit_log_format(ab, " auid=%u ses=%u", auid, sessionid);
> > }
> >
> > +/*
> > + * audit_log_contid - report container info
> > + * @tsk: task to be recorded
> > + * @context: task or local context for record
> > + * @op: contid string description
> > + */
> > +int audit_log_contid(struct task_struct *tsk,
> > + struct audit_context *context, char *op)
> > +{
> > + struct audit_buffer *ab;
> > +
> > + if (!audit_contid_set(tsk))
> > + return 0;
> > + /* Generate AUDIT_CONTAINER record with container ID */
> > + ab = audit_log_start(context, GFP_KERNEL, AUDIT_CONTAINER);
> > + if (!ab)
> > + return -ENOMEM;
> > + audit_log_format(ab, "op=%s contid=%llu",
> > + op, audit_get_contid(tsk));
> > + audit_log_end(ab);
> > + return 0;
> > +}
> > +EXPORT_SYMBOL(audit_log_contid);
>
> As discussed in the previous iteration of the patch, I prefer
> AUDIT_CONTAINER_ID here over AUDIT_CONTAINER. If you feel strongly
> about keeping it as-is with AUDIT_CONTAINER I suppose I could live
> with that, but it is isn't my first choice.
I don't have a strong opinion on this one, mildly preferring the shorter
one only because it is shorter.
Steve? Can you comment on this one way or the other?
> However, I do care about the "op" field in this record. It just
> doesn't make any sense; the way you are using it it is more of a
> context field than an operations field, and even then why is the
> context important from a logging and/or security perspective? Drop it
> please.
I'll rename it to whatever you like. I'd suggest "ref=". The reason I
think it is important is there are multiple sources that aren't always
obvious from the other records to which it is associated. In the case
of ptrace and signals, there can be many target tasks listed (OBJ_PID)
with no other way to distinguish the matching audit container identifier
records all for one event. This is in addition to the default syscall
container identifier record. I'm not currently happy with the text
content to link the two, but that should be solvable (most obvious is
taret PID). Throwing away this information seems shortsighted.
> paul moore
- RGB
--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635
^ permalink raw reply
* Re: [GIT PULL] siginfo updates for 4.20-rc1
From: Linus Torvalds @ 2018-10-24 10:26 UTC (permalink / raw)
To: ebiederm; +Cc: Linux Kernel Mailing List, linux-api
In-Reply-To: <87h8hdzajh.fsf@xmission.com>
On Tue, Oct 23, 2018 at 3:01 AM Eric W. Biederman <ebiederm@xmission.com> wrote:
>
> Please pull the siginfo-linus branch from the git tree:
Pulled.
Linus
^ permalink raw reply
* Re: [RFC PATCH for 4.21 01/16] rseq/selftests: Add reference counter to coexist with glibc
From: Mathieu Desnoyers @ 2018-10-23 14:59 UTC (permalink / raw)
To: Szabolcs Nagy
Cc: nd, Peter Zijlstra, Paul E. McKenney, Boqun Feng, linux-kernel,
linux-api, Thomas Gleixner, Andy Lutomirski, Dave Watson,
Paul Turner, Andrew Morton, Russell King, Ingo Molnar,
H. Peter Anvin, Andi Kleen, Chris Lameter, Ben Maurer, rostedt,
Josh Triplett, Linus Torvalds, Catalin Marinas
In-Reply-To: <0dcd1e46-7007-3a99-4ada-f09ab66ab2d5@arm.com>
----- On Oct 12, 2018, at 10:59 AM, Szabolcs Nagy szabolcs.nagy@arm.com wrote:
> On 11/10/18 20:42, Mathieu Desnoyers wrote:
>> ----- On Oct 11, 2018, at 1:04 PM, Szabolcs Nagy Szabolcs.Nagy@arm.com wrote:
>>
>>> On 11/10/18 17:37, Mathieu Desnoyers wrote:
>>>> ----- On Oct 11, 2018, at 12:20 PM, Szabolcs Nagy Szabolcs.Nagy@arm.com wrote:
>>>>> On 11/10/18 16:13, Mathieu Desnoyers wrote:
>>>>>> ----- On Oct 11, 2018, at 6:37 AM, Szabolcs Nagy Szabolcs.Nagy@arm.com wrote:
>>>>>>> On 10/10/18 20:19, Mathieu Desnoyers wrote:
>>>>>>>> +__attribute__((visibility("hidden"))) __thread
>>>>>>>> +volatile struct libc_rseq __lib_rseq_abi = {
>>>>>>> ...
>>>>> but it's in a magic struct that's called "abi" which is confusing,
>>>>> the counter is not abi, it's in a hidden object.
>>>>
>>>> No, it is really an ABI between user-space apps/libs. It's not meant to be
>>>> hidden. glibc implements its own register/unregister functions (it does not
>>>> link against librseq). librseq exposes register/unregister functions as public
>>>> APIs. Those also use the refcount. I also plan to have existing libraries, e.g.
>>>> liblttng-ust and possibly liburcu flavors, implement the
>>>> registration/unregistration and refcount handling on their own, so we don't
>>>> have to add a requirement on additional linking on librseq for pre-existing
>>>> libraries.
>>>>
>>>> So that refcount is not an ABI between kernel and user-space, but it's a
>>>> user-space ABI nevertheless (between program and shared objects).
>>>>
>>>
>>> if that's what you want, then your declaration is wrong.
>>> the object should not have hidden visibility.
>>
>> Actually, if we look closer into my patch, it defines two symbols,
>> one of which is an alias:
>>
>> __attribute__((visibility("hidden"))) __thread
>> volatile struct libc_rseq __lib_rseq_abi = {
>> .cpu_id = RSEQ_CPU_ID_UNINITIALIZED,
>> };
>>
>> extern __attribute__((weak, alias("__lib_rseq_abi"))) __thread
>> volatile struct rseq __rseq_abi;
>>
>> Note that the public __rseq_abi symbol is weak but does not have
>> hidden visibility. I do this to ensure I don't get prototype
>> mismatch for __rseq_abi between rseq.c and rseq.h (it is required
>> to be a struct rseq by rseq.h), but I want the space to hold the
>> extra refcount field present in struct libc_rseq.
>>
>
I notice this email has been sitting in my inbox for a while, sorry
for the delayed reply.
> but that's wrong: the weak symbol might get resolved to
> a different object in another module, while you increment
> a local refcounter, so there is no coordination between
> userspace components.
Hrm, good point. I should not use the __lib_rseq_abi symbol at all
here.
>
> this was the reason for my first question in my original mail,
> as soon as i saw the local counter i suspected this is broken.
Good catch, yes. I think I should not use the alias approach then.
>
> and "assume there is an extra counter field" is not
> acceptable as user space abi, if the counter is relevant
> across modules then expose the entire struct.
The question that arises here is whether I should update
uapi/linux/rseq.h and add the refcount field directly in
there, even though the kernel does not care about it per se ?
>
>>> either the struct should be public abi (extern tls
>>> symbol) or the register/unregister functions should
>>> be public abi (so when multiple implementations are
>>> present in the same process only one of them will
>>> provide definition for the public abi symbol and
>>> thus there will be one refcounter).
>>
>> Those are two possible solutions, indeed. Considering that
>> we already need to expose the __rseq_abi symbol as a public
>> ABI in a way that ensures that multiple implementations
>> in a same process end up only using one of them, it seems
>> straightforward to simply extend that structure and hold the
>> refcount there, rather than having two extra ABI symbols
>> (register/unregister functions).
>>
>> One very appropriate question here is whether we want to
>> expose the layout of struct libc_rseq (which includes the
>> refcount) in a public header file, and if so, which project
>> should hold it ? Or do we just want to document the layout
>> of this ABI so projects can define the structure layout
>> internally ? As my implementation currently stands, I have
>> the following structure duplicated into rseq selftests,
>> librseq, and glibc:
>>
>
> "not exposed" and "the counter is abi" together is not
> useful, either you want coordination in user-space or
> not, that decision should imply the userspace abi/api
> (e.g. adding a counter to the user-space struct).
I'm inclined to add the refcount to struct rseq directly,
unless anyone objects. It seems much simpler.
>
> it is true that only modules that implement registration
> need to know about the counter and normal users don't,
> but if you want any coordination then the layout must
> be fixed and that should be exposed somewhere to avoid
> breakage.
Yep. Exposing this in uapi/linux/rseq.h is the main
location that seems to make sense to me.
>
> (i think ideally the api would be controlled by functions
> and not object symbols with magic layout, but the rseq
> design is already full of such magic. and i think it's
> better to do the registration in libc only without
> coordination but that might not be practical if users
> want it now)
Yes, early adopters is my concern here.
>
>> /*
>> * linux/rseq.h defines struct rseq as aligned on 32 bytes. The kernel ABI
>> * size is 20 bytes. For support of multiple rseq users within a process,
>> * user-space defines an extra 4 bytes field as a reference count, for a
>> * total of 24 bytes.
>> */
>> struct libc_rseq {
>> /* kernel-userspace ABI. */
>> __u32 cpu_id_start;
>> __u32 cpu_id;
>> __u64 rseq_cs;
>> __u32 flags;
>> /* user-space ABI. */
>> __u32 refcount;
>> } __attribute__((aligned(4 * sizeof(__u64))));
>>
>> That duplicated structure only needs to be present in early-adopter
>> applications/libraries. Those linking on librseq or relying on newer
>> glibc to register rseq don't need to know about this extended layout:
>> all they need to care about is the layout of struct rseq (without the
>> added refcount).
>
> please decide if you want multiple libraries to
> be able to register rseq and coordinate or not
> and document that decision in the public api.
Yes, I'll try this out and see how this goes.
Thanks for the feedback!
Mathieu
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply
* [GIT PULL] siginfo updates for 4.20-rc1
From: Eric W. Biederman @ 2018-10-23 2:01 UTC (permalink / raw)
To: Linus Torvalds; +Cc: linux-kernel, linux-api
Linus,
Please pull the siginfo-linus branch from the git tree:
git://git.kernel.org/pub/scm/linux/kernel/git/ebiederm/user-namespace.git siginfo-linus
HEAD: a36700589b85443e28170be59fa11c8a104130a5 signal: Guard against negative signal numbers in copy_siginfo_from_user32
I have been slowly sorting out siginfo and this is the culmination of that work.
The primary result is in several ways the signal infrastructure has been made
less error prone. The code has been updated so that manually specifying
SEND_SIG_FORCED is never necessary. The conversion to the new siginfo sending
functions is now complete, which makes it difficult to send a signal without
filling in the proper siginfo fields.
At the tail end of the patchset comes the optimization of decreasing the size of
struct siginfo in the kernel from 128 bytes to about 48 bytes on 64bit. The
fundamental observation that enables this is by definition none of the known
ways to use struct siginfo uses the extra bytes.
This comes at the cost of a small user space observable difference. For the
rare case of siginfo being injected into the kernel only what can be copied
into kernel_siginfo is delivered to the destination, the rest of the bytes are
set to 0. For cases where the signal and the si_code are known this is safe,
because we know thos bytes are not used. For cases where the signal and si_code
combination is unknown the bits that won't fit into struct kernel_siginfo are
tested to verify they are zero, and the send fails if they are not.
I made an extensive search through userspace code and I could not find anything
that would break because of the above change. If it turns out I did break
something it will take just the revert of a single change to restore
kernel_siginfo to the same size as userspace siginfo.
Testing did reveal dependencies on preferring the signo passed to sigqueueinfo
over si->signo, so bit the bullet and added the complexity necessary to handle
that case.
Testing also revealed bad things can happen if a negative signal number is
passed into the system calls. Something no sane application will do but
something a malicious program or a fuzzer might do. So I have fixed the code
that performs the bounds checks to ensure negative signal numbers are handled.
There are minor conflicts between this tree and several other trees.
- The x86 tree
- The y2038 tree
- The arm64 tree
- The x86 tip tree
I think only the resolution of the x86 tip tree is at all difficult. None of
the conflicts are fundamental. They are all from changes to other parts of the
code that are just close enough to have context conflicts. The x86 tip tree
conflict actually involves a conflict from removing a unnecessary pkey parameter
on the siginfo side and a some small refactoring on the x86 side.
Eric W. Biederman (80):
signal: Always ignore SIGKILL and SIGSTOP sent to the global init
signal: Properly deliver SIGILL from uprobes
signal: Properly deliver SIGSEGV from x86 uprobes
signal: Always deliver the kernel's SIGKILL and SIGSTOP to a pid namespace init
signal: send_sig_all no longer needs SEND_SIG_FORCED
signal: Remove the siginfo paramater from kernel_dqueue_signal
signal: Don't send siginfo to kthreads.
signal: Never allocate siginfo for SIGKILL or SIGSTOP
signal: Use SEND_SIG_PRIV not SEND_SIG_FORCED with SIGKILL and SIGSTOP
signal: Remove SEND_SIG_FORCED
signal/GenWQE: Fix sending of SIGKILL
tty_io: Use group_send_sig_info in __do_SACK to note it is a session being killed
signal: Use group_send_sig_info to kill all processes in a pid namespace
signal: Remove specific_send_sig_info
signal: Pair exports with their functions
signal: Simplify tracehook_report_syscall_exit
signal/x86: Inline fill_sigtrap_info in it's only caller send_sigtrap
signal/x86: Move MCE error reporting out of force_sig_info_fault
signal/x86: Use send_sig_mceerr as apropriate
signal/x86: In trace_mpx_bounds_register_exception add __user annotations
signal/x86: Move mpx siginfo generation into do_bounds
signal/x86/traps: Factor out show_signal
signal/x86/traps: Move more code into do_trap_no_signal so it can be reused
signal/x86/traps: Use force_sig_bnderr
signal/x86/traps: Use force_sig instead of open coding it.
signal/x86/traps: Simplify trap generation
signal/x86: Remove pkey parameter from bad_area_nosemaphore
signal/x86: Remove the pkey parameter from do_sigbus
signal/x86: Remove pkey parameter from mm_fault_error
signal/x86: Don't compute pkey in __do_page_fault
signal/x86: Pass pkey not vma into __bad_area
signal/x86: Call force_sig_pkuerr from __bad_area_nosemaphore
signal/x86: Replace force_sig_info_fault with force_sig_fault
signal/x86: Pass pkey by value
signal/x86: Use force_sig_fault where appropriate
signal/powerpc: Use force_sig_mceerr as appropriate
signal/powerpc: Remove pkey parameter from __bad_area
signal/powerpc: Call _exception_pkey directly from bad_key_fault_exception
signal/powerpc: Remove pkey parameter from __bad_area_nosemaphore
signal/powerpc: Factor the common exception code into exception_common
signal/powerpc: Call force_sig_fault from _exception
signal/powerpc: Specialize _exception_pkey for handling pkey exceptions
signal/powerpc: Simplify _exception_pkey by using force_sig_pkuerr
signal/powerpc: Use force_sig_fault where appropriate
signal/arm64: Push siginfo generation into arm64_notify_die
signal/arm64: Remove unneeded tsk parameter from arm64_force_sig_info
signal/arm64: Factor out arm64_show_signal from arm64_force_sig_info
signal/arm64: Factor set_thread_esr out of __do_user_fault
signal/arm64: Consolidate the two hwpoison cases in do_page_fault
signal/arm64: For clarity separate the 3 signal sending cases in do_page_fault
signal/arm64: Expand __do_user_fault and remove it
signal/arm64: Only perform one esr_to_fault_info call in do_page_fault
signal/arm64: Only call set_thread_esr once in do_page_fault
signal/arm64: Add and use arm64_force_sig_fault where appropriate
signal/arm64: Add and use arm64_force_sig_mceerr as appropriate
signal/arm64: Remove arm64_force_sig_info
signal/arm64: In ptrace_hbptriggered name the signal description string
signal/arm64: Add and use arm64_force_sig_ptrace_errno_trap
signal/arm64: Use send_sig_fault where appropriate
signal/arm: Push siginfo generation into arm_notify_die
signal/arm: Use force_sig_fault where appropriate
signal/arm: Use send_sig_fault where appropriate
signal/arm/kvm: Use send_sig_mceerr
signal/ia64: Use the generic force_sigsegv in setup_frame
signal/ia64: Use the force_sig(SIGSEGV,...) in ia64_rt_sigreturn
signal/ia64: Use force_sig_fault where appropriate
signal/arc: Push siginfo generation into unhandled_exception
signal/arc: Use force_sig_fault where appropriate
signal/unicore32: Use send_sig_fault where appropriate
signal/unicore32: Generate siginfo in ucs32_notify_die
signal/unicore32: Use force_sig_fault where appropriate
signal/sparc: Move EMT_TAGOVF into the generic siginfo.h
signal: Fail sigqueueinfo if si_signo != sig
signal: Remove the need for __ARCH_SI_PREABLE_SIZE and SI_PAD_SIZE
signal: Introduce copy_siginfo_from_user and use it's return value
signal: Distinguish between kernel_siginfo and siginfo
signal: Use a smaller struct siginfo in the kernel
signal: In sigqueueinfo prefer sig not si_signo
signal: Guard against negative signal numbers in copy_siginfo_from_user
signal: Guard against negative signal numbers in copy_siginfo_from_user32
arch/alpha/include/uapi/asm/siginfo.h | 1 -
arch/arc/kernel/traps.c | 22 +-
arch/arc/mm/fault.c | 20 +-
arch/arm/include/asm/bug.h | 4 +-
arch/arm/kernel/ptrace.c | 11 +-
arch/arm/kernel/swp_emulate.c | 16 +-
arch/arm/kernel/traps.c | 63 ++--
arch/arm/mm/alignment.c | 10 +-
arch/arm/mm/fault.c | 28 +-
arch/arm/vfp/vfpmodule.c | 11 +-
arch/arm64/include/asm/system_misc.h | 3 +-
arch/arm64/include/asm/traps.h | 5 +-
arch/arm64/include/uapi/asm/Kbuild | 1 +
arch/arm64/include/uapi/asm/siginfo.h | 24 --
arch/arm64/kernel/debug-monitors.c | 11 +-
arch/arm64/kernel/fpsimd.c | 10 +-
arch/arm64/kernel/ptrace.c | 16 +-
arch/arm64/kernel/sys_compat.c | 13 +-
arch/arm64/kernel/traps.c | 67 ++---
arch/arm64/mm/fault.c | 94 +++---
arch/ia64/include/uapi/asm/siginfo.h | 2 -
arch/ia64/kernel/brl_emu.c | 31 +-
arch/ia64/kernel/signal.c | 60 +---
arch/ia64/kernel/traps.c | 144 +++------
arch/ia64/kernel/unaligned.c | 12 +-
arch/ia64/mm/fault.c | 12 +-
arch/mips/include/uapi/asm/siginfo.h | 11 -
arch/parisc/include/uapi/asm/Kbuild | 1 +
arch/parisc/include/uapi/asm/siginfo.h | 11 -
arch/powerpc/include/asm/bug.h | 2 +-
arch/powerpc/include/asm/ptrace.h | 2 +-
arch/powerpc/include/uapi/asm/Kbuild | 1 +
arch/powerpc/include/uapi/asm/siginfo.h | 18 --
arch/powerpc/kernel/process.c | 9 +-
arch/powerpc/kernel/traps.c | 34 +--
arch/powerpc/mm/fault.c | 55 ++--
arch/powerpc/platforms/cell/spu_base.c | 4 +-
arch/powerpc/platforms/cell/spufs/fault.c | 26 +-
arch/riscv/include/uapi/asm/Kbuild | 1 +
arch/riscv/include/uapi/asm/siginfo.h | 24 --
arch/s390/include/uapi/asm/Kbuild | 1 +
arch/s390/include/uapi/asm/siginfo.h | 17 --
arch/sparc/include/uapi/asm/siginfo.h | 7 -
arch/unicore32/include/asm/bug.h | 3 +-
arch/unicore32/kernel/fpu-ucf64.c | 12 +-
arch/unicore32/kernel/traps.c | 5 +-
arch/unicore32/mm/fault.c | 27 +-
arch/x86/entry/vsyscall/vsyscall_64.c | 9 +-
arch/x86/include/asm/compat.h | 2 +-
arch/x86/include/asm/mpx.h | 12 +-
arch/x86/include/asm/ptrace.h | 2 +-
arch/x86/include/asm/trace/mpx.h | 4 +-
arch/x86/include/uapi/asm/siginfo.h | 2 -
arch/x86/kernel/ptrace.c | 29 +-
arch/x86/kernel/traps.c | 176 +++++------
arch/x86/kernel/umip.c | 8 +-
arch/x86/kernel/uprobes.c | 2 +-
arch/x86/kvm/mmu.c | 11 +-
arch/x86/mm/fault.c | 167 ++++-------
arch/x86/mm/mpx.c | 30 +-
drivers/misc/genwqe/card_base.h | 2 +-
drivers/misc/genwqe/card_dev.c | 9 +-
drivers/tty/sysrq.c | 2 +-
drivers/tty/tty_io.c | 6 +-
drivers/usb/core/devio.c | 4 +-
drivers/usb/gadget/function/f_mass_storage.c | 4 +-
fs/binfmt_elf.c | 6 +-
fs/coredump.c | 2 +-
fs/fcntl.c | 2 +-
fs/jffs2/background.c | 2 +-
fs/signalfd.c | 6 +-
include/linux/binfmts.h | 2 +-
include/linux/compat.h | 4 +-
include/linux/coredump.h | 4 +-
include/linux/lsm_hooks.h | 4 +-
include/linux/posix-timers.h | 2 +-
include/linux/ptrace.h | 17 +-
include/linux/sched.h | 2 +-
include/linux/sched/signal.h | 23 +-
include/linux/security.h | 6 +-
include/linux/signal.h | 18 +-
include/linux/signal_types.h | 8 +-
include/linux/tracehook.h | 13 +-
include/trace/events/signal.h | 7 +-
include/uapi/asm-generic/siginfo.h | 193 ++++++------
ipc/mqueue.c | 2 +-
kernel/events/uprobes.c | 4 +-
kernel/pid_namespace.c | 2 +-
kernel/ptrace.c | 26 +-
kernel/seccomp.c | 6 +-
kernel/signal.c | 421 +++++++++++++++++----------
kernel/time/posix-timers.c | 2 +-
mm/oom_kill.c | 4 +-
security/apparmor/lsm.c | 2 +-
security/security.c | 2 +-
security/selinux/hooks.c | 2 +-
security/smack/smack_lsm.c | 2 +-
virt/kvm/arm/mmu.c | 14 +-
98 files changed, 925 insertions(+), 1328 deletions(-)
^ permalink raw reply
* Re: [PATCH v7 1/6] seccomp: add a return code to trap to userspace
From: Christian Brauner @ 2018-10-22 9:42 UTC (permalink / raw)
To: Tycho Andersen
Cc: Kees Cook, Jann Horn, Linux API, Linux Containers, Akihiro Suda,
Oleg Nesterov, LKML, Eric W . Biederman,
linux-fsdevel@vger.kernel.org, Christian Brauner, Andy Lutomirski
In-Reply-To: <20181021160437.GB25202@cisco>
On Sun, Oct 21, 2018 at 05:04:37PM +0100, Tycho Andersen wrote:
> On Wed, Oct 17, 2018 at 03:21:02PM -0700, Kees Cook wrote:
> > On Wed, Oct 17, 2018 at 1:29 PM, Tycho Andersen <tycho@tycho.ws> wrote:
> > > On Thu, Sep 27, 2018 at 02:31:24PM -0700, Kees Cook wrote:
> > >> On Thu, Sep 27, 2018 at 8:11 AM, Tycho Andersen <tycho@tycho.ws> wrote:
> > >> > @@ -60,4 +62,29 @@ struct seccomp_data {
> > >> > __u64 args[6];
> > >> > };
> > >> >
> > >> > +struct seccomp_notif {
> > >> > + __u16 len;
> > >> > + __u64 id;
> > >> > + __u32 pid;
> > >> > + __u8 signaled;
> > >> > + struct seccomp_data data;
> > >> > +};
> > >> > +
> > >> > +struct seccomp_notif_resp {
> > >> > + __u16 len;
> > >> > + __u64 id;
> > >> > + __s32 error;
> > >> > + __s64 val;
> > >> > +};
> > >>
> > >> So, len has to come first, for versioning. However, since it's ahead
> > >> of a u64, this leaves a struct padding hole. pahole output:
> > >>
> > >> struct seccomp_notif {
> > >> __u16 len; /* 0 2 */
> > >>
> > >> /* XXX 6 bytes hole, try to pack */
> > >>
> > >> __u64 id; /* 8 8 */
> > >> __u32 pid; /* 16 4 */
> > >> __u8 signaled; /* 20 1 */
> > >>
> > >> /* XXX 3 bytes hole, try to pack */
> > >>
> > >> struct seccomp_data data; /* 24 64 */
> > >> /* --- cacheline 1 boundary (64 bytes) was 24 bytes ago --- */
> > >>
> > >> /* size: 88, cachelines: 2, members: 5 */
> > >> /* sum members: 79, holes: 2, sum holes: 9 */
> > >> /* last cacheline: 24 bytes */
> > >> };
> > >> struct seccomp_notif_resp {
> > >> __u16 len; /* 0 2 */
> > >>
> > >> /* XXX 6 bytes hole, try to pack */
> > >>
> > >> __u64 id; /* 8 8 */
> > >> __s32 error; /* 16 4 */
> > >>
> > >> /* XXX 4 bytes hole, try to pack */
> > >>
> > >> __s64 val; /* 24 8 */
> > >>
> > >> /* size: 32, cachelines: 1, members: 4 */
> > >> /* sum members: 22, holes: 2, sum holes: 10 */
> > >> /* last cacheline: 32 bytes */
> > >> };
> > >>
> > >> How about making len u32, and moving pid and error above "id"? This
> > >> leaves a hole after signaled, so changing "len" won't be sufficient
> > >> for versioning here. Perhaps move it after data?
> > >
> > > Just to confirm my understanding; I've got these as:
> > >
> > > struct seccomp_notif {
> > > __u32 len; /* 0 4 */
> > > __u32 pid; /* 4 4 */
> > > __u64 id; /* 8 8 */
> > > __u8 signaled; /* 16 1 */
> > >
> > > /* XXX 7 bytes hole, try to pack */
> > >
> > > struct seccomp_data data; /* 24 64 */
> > > /* --- cacheline 1 boundary (64 bytes) was 24 bytes ago --- */
> > >
> > > /* size: 88, cachelines: 2, members: 5 */
> > > /* sum members: 81, holes: 1, sum holes: 7 */
> > > /* last cacheline: 24 bytes */
> > > };
> > > struct seccomp_notif_resp {
> > > __u32 len; /* 0 4 */
> > > __s32 error; /* 4 4 */
> > > __u64 id; /* 8 8 */
> > > __s64 val; /* 16 8 */
> > >
> > > /* size: 24, cachelines: 1, members: 4 */
> > > /* last cacheline: 24 bytes */
> > > };
> > >
> > > in the next version. Since the structure has no padding at the end of
> > > it, I think the Right Thing will happen. Note that this is slightly
> > > different than what Kees suggested, if I add signaled after data, then
> > > I end up with:
> > >
> > > struct seccomp_notif {
> > > __u32 len; /* 0 4 */
> > > __u32 pid; /* 4 4 */
> > > __u64 id; /* 8 8 */
> > > struct seccomp_data data; /* 16 64 */
> > > /* --- cacheline 1 boundary (64 bytes) was 16 bytes ago --- */
> > > __u8 signaled; /* 80 1 */
> > >
> > > /* size: 88, cachelines: 2, members: 5 */
> > > /* padding: 7 */
> > > /* last cacheline: 24 bytes */
> > > };
> > >
> > > which I think will have the versioning problem if the next member
> > > introduces is < 7 bytes.
> >
> > It'll be a problem in either place. What I was thinking was that
> > specific versioning is required instead of just length.
>
> Euh, so I implemented this, and it sucks :). It's ugly, and generally
> feels bad.
>
> What if instead we just get rid of versioning all together, and
> instead introduce a u32 flags? We could have one flag right now
> (SECCOMP_NOTIF_FLAG_SIGNALED), and use introduce others as we add more
> information to the response. Then we can add
> SECCOMP_NOTIF_FLAG_EXTRA_FOO, and add another SECCOMP_IOCTL_GET_FOO to
> grab the info?
>
> FWIW, it's not really clear to me that we'll ever add anything to the
> response since hopefully we'll land PUT_FD, so maybe this is all moot
> anyway.
I guess the only argument against a flag would be that you run out of
bits quickly if your interface grows (cf. mount, netlink etc.). But this
is likely not a concern here.
I actually think that the way vfs capabilities are done is pretty
nice. By accident or design they allow transparent translation between
old and new formats in-kernel. So would be cool if we can have the same
guarantee for this interface.
Christian
^ permalink raw reply
* Re: [PATCH 1/3] printk: Introduce per-console loglevel setting
From: Sergey Senozhatsky @ 2018-10-22 2:37 UTC (permalink / raw)
To: Calvin Owens
Cc: Sergey Senozhatsky, Petr Mladek, Sergey Senozhatsky,
Steven Rostedt, linux-api@vger.kernel.org,
linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org,
Kernel Team
In-Reply-To: <20181019220314.GA8164@Haydn>
On (10/19/18 22:03), Calvin Owens wrote:
> Hi Sergey,
>
> It's in-progress, I'm sorry it hasn't happened sooner.
Great!
> By embedding the kobject in the console struct, we end up needing to refactor
> the console drivers to use the kobject refcount instead of simply calling
> kfree(), which is what I'm working on. It ends up being tedious but not
> particularly complicated, my goal is to have this up soon :)
OK :)
-ss
^ 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