| Message ID | 230e91cd3e50a3d8015daac135c24c4c58cf0a21.1568834524.git.rgb@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | audit: implement container identifier | expand |
On Wed, Sep 18, 2019 at 09:22:23PM -0400, Richard Guy Briggs wrote: > Set an arbitrary limit on the number of audit container identifiers to > limit abuse. > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com> > --- > kernel/audit.c | 8 ++++++++ > kernel/audit.h | 4 ++++ > 2 files changed, 12 insertions(+) > > diff --git a/kernel/audit.c b/kernel/audit.c > index 53d13d638c63..329916534dd2 100644 > --- a/kernel/audit.c > +++ b/kernel/audit.c > @@ -139,6 +139,7 @@ struct audit_net { > struct list_head audit_inode_hash[AUDIT_INODE_BUCKETS]; > /* Hash for contid-based rules */ > struct list_head audit_contid_hash[AUDIT_CONTID_BUCKETS]; > +int audit_contid_count = 0; > > static struct kmem_cache *audit_buffer_cache; > > @@ -2384,6 +2385,7 @@ void audit_cont_put(struct audit_cont *cont) > put_task_struct(cont->owner); > list_del_rcu(&cont->list); > kfree_rcu(cont, rcu); > + audit_contid_count--; > } > } > > @@ -2456,6 +2458,11 @@ int audit_set_contid(struct task_struct *task, u64 contid) > goto conterror; > } > } > + /* Set max contids */ > + if (audit_contid_count > AUDIT_CONTID_COUNT) { > + rc = -ENOSPC; > + goto conterror; > + } You should check for audit_contid_count == AUDIT_CONTID_COUNT here, no? or at least >=, since you increment it below. Otherwise its possible that you will exceed it by one in the full condition. > if (!newcont) { > newcont = kmalloc(sizeof(struct audit_cont), GFP_ATOMIC); > if (newcont) { > @@ -2465,6 +2472,7 @@ int audit_set_contid(struct task_struct *task, u64 contid) > newcont->owner = current; > refcount_set(&newcont->refcount, 1); > list_add_rcu(&newcont->list, &audit_contid_hash[h]); > + audit_contid_count++; > } else { > rc = -ENOMEM; > goto conterror; > diff --git a/kernel/audit.h b/kernel/audit.h > index 162de8366b32..543f1334ba47 100644 > --- a/kernel/audit.h > +++ b/kernel/audit.h > @@ -219,6 +219,10 @@ static inline int audit_hash_contid(u64 contid) > return (contid & (AUDIT_CONTID_BUCKETS-1)); > } > > +extern int audit_contid_count; > + > +#define AUDIT_CONTID_COUNT 1 << 16 > + Just to ask the question, since it wasn't clear in the changelog, what abuse are you avoiding here? Ostensibly you should be able to create as many container ids as you have space for, and the simple creation of container ids doesn't seem like the resource strain I would be concerned about here, given that an orchestrator can still create as many containers as the system will otherwise allow, which will consume significantly more ram/disk/etc. > /* Indicates that audit should log the full pathname. */ > #define AUDIT_NAME_FULL -1 > > -- > 1.8.3.1 > >
On Fri, Sep 27, 2019 at 8:52 AM Neil Horman <nhorman@tuxdriver.com> wrote: > On Wed, Sep 18, 2019 at 09:22:23PM -0400, Richard Guy Briggs wrote: > > Set an arbitrary limit on the number of audit container identifiers to > > limit abuse. > > > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com> > > --- > > kernel/audit.c | 8 ++++++++ > > kernel/audit.h | 4 ++++ > > 2 files changed, 12 insertions(+) > > > > diff --git a/kernel/audit.c b/kernel/audit.c > > index 53d13d638c63..329916534dd2 100644 > > --- a/kernel/audit.c > > +++ b/kernel/audit.c ... > > @@ -2465,6 +2472,7 @@ int audit_set_contid(struct task_struct *task, u64 contid) > > newcont->owner = current; > > refcount_set(&newcont->refcount, 1); > > list_add_rcu(&newcont->list, &audit_contid_hash[h]); > > + audit_contid_count++; > > } else { > > rc = -ENOMEM; > > goto conterror; > > diff --git a/kernel/audit.h b/kernel/audit.h > > index 162de8366b32..543f1334ba47 100644 > > --- a/kernel/audit.h > > +++ b/kernel/audit.h > > @@ -219,6 +219,10 @@ static inline int audit_hash_contid(u64 contid) > > return (contid & (AUDIT_CONTID_BUCKETS-1)); > > } > > > > +extern int audit_contid_count; > > + > > +#define AUDIT_CONTID_COUNT 1 << 16 > > + > > Just to ask the question, since it wasn't clear in the changelog, what > abuse are you avoiding here? Ostensibly you should be able to create as > many container ids as you have space for, and the simple creation of > container ids doesn't seem like the resource strain I would be concerned > about here, given that an orchestrator can still create as many > containers as the system will otherwise allow, which will consume > significantly more ram/disk/etc. I've got a similar question. Up to this point in the patchset, there is a potential issue of hash bucket chain lengths and traversing them with a spinlock held, but it seems like we shouldn't be putting an arbitrary limit on audit container IDs unless we have a good reason for it. If for some reason we do want to enforce a limit, it should probably be a tunable value like a sysctl, or similar. -- paul moore www.paul-moore.com
On 2019-10-10 20:38, Paul Moore wrote: > On Fri, Sep 27, 2019 at 8:52 AM Neil Horman <nhorman@tuxdriver.com> wrote: > > On Wed, Sep 18, 2019 at 09:22:23PM -0400, Richard Guy Briggs wrote: > > > Set an arbitrary limit on the number of audit container identifiers to > > > limit abuse. > > > > > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com> > > > --- > > > kernel/audit.c | 8 ++++++++ > > > kernel/audit.h | 4 ++++ > > > 2 files changed, 12 insertions(+) > > > > > > diff --git a/kernel/audit.c b/kernel/audit.c > > > index 53d13d638c63..329916534dd2 100644 > > > --- a/kernel/audit.c > > > +++ b/kernel/audit.c > > ... > > > > @@ -2465,6 +2472,7 @@ int audit_set_contid(struct task_struct *task, u64 contid) > > > newcont->owner = current; > > > refcount_set(&newcont->refcount, 1); > > > list_add_rcu(&newcont->list, &audit_contid_hash[h]); > > > + audit_contid_count++; > > > } else { > > > rc = -ENOMEM; > > > goto conterror; > > > diff --git a/kernel/audit.h b/kernel/audit.h > > > index 162de8366b32..543f1334ba47 100644 > > > --- a/kernel/audit.h > > > +++ b/kernel/audit.h > > > @@ -219,6 +219,10 @@ static inline int audit_hash_contid(u64 contid) > > > return (contid & (AUDIT_CONTID_BUCKETS-1)); > > > } > > > > > > +extern int audit_contid_count; > > > + > > > +#define AUDIT_CONTID_COUNT 1 << 16 > > > + > > > > Just to ask the question, since it wasn't clear in the changelog, what > > abuse are you avoiding here? Ostensibly you should be able to create as > > many container ids as you have space for, and the simple creation of > > container ids doesn't seem like the resource strain I would be concerned > > about here, given that an orchestrator can still create as many > > containers as the system will otherwise allow, which will consume > > significantly more ram/disk/etc. > > I've got a similar question. Up to this point in the patchset, there > is a potential issue of hash bucket chain lengths and traversing them > with a spinlock held, but it seems like we shouldn't be putting an > arbitrary limit on audit container IDs unless we have a good reason > for it. If for some reason we do want to enforce a limit, it should > probably be a tunable value like a sysctl, or similar. Can you separate and clarify the concerns here? I plan to move this patch to the end of the patchset and make it optional, possibly adding a tuning mechanism. Like the migration from /proc to netlink for loginuid/sessionid/contid/capcontid, this was Eric Biederman's concern and suggested mitigation. As for the first issue of the bucket chain length traversal while holding the list spin-lock, would you prefer to use the rcu lock to traverse the list and then only hold the spin-lock when modifying the list, and possibly even make the spin-lock more fine-grained per list? > 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
On 2019-09-27 08:51, Neil Horman wrote: > On Wed, Sep 18, 2019 at 09:22:23PM -0400, Richard Guy Briggs wrote: > > Set an arbitrary limit on the number of audit container identifiers to > > limit abuse. > > > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com> > > --- > > kernel/audit.c | 8 ++++++++ > > kernel/audit.h | 4 ++++ > > 2 files changed, 12 insertions(+) > > > > diff --git a/kernel/audit.c b/kernel/audit.c > > index 53d13d638c63..329916534dd2 100644 > > --- a/kernel/audit.c > > +++ b/kernel/audit.c > > @@ -139,6 +139,7 @@ struct audit_net { > > struct list_head audit_inode_hash[AUDIT_INODE_BUCKETS]; > > /* Hash for contid-based rules */ > > struct list_head audit_contid_hash[AUDIT_CONTID_BUCKETS]; > > +int audit_contid_count = 0; > > > > static struct kmem_cache *audit_buffer_cache; > > > > @@ -2384,6 +2385,7 @@ void audit_cont_put(struct audit_cont *cont) > > put_task_struct(cont->owner); > > list_del_rcu(&cont->list); > > kfree_rcu(cont, rcu); > > + audit_contid_count--; > > } > > } > > > > @@ -2456,6 +2458,11 @@ int audit_set_contid(struct task_struct *task, u64 contid) > > goto conterror; > > } > > } > > + /* Set max contids */ > > + if (audit_contid_count > AUDIT_CONTID_COUNT) { > > + rc = -ENOSPC; > > + goto conterror; > > + } > You should check for audit_contid_count == AUDIT_CONTID_COUNT here, no? > or at least >=, since you increment it below. Otherwise its possible > that you will exceed it by one in the full condition. Yes, agreed. > > if (!newcont) { > > newcont = kmalloc(sizeof(struct audit_cont), GFP_ATOMIC); > > if (newcont) { > > @@ -2465,6 +2472,7 @@ int audit_set_contid(struct task_struct *task, u64 contid) > > newcont->owner = current; > > refcount_set(&newcont->refcount, 1); > > list_add_rcu(&newcont->list, &audit_contid_hash[h]); > > + audit_contid_count++; > > } else { > > rc = -ENOMEM; > > goto conterror; > > diff --git a/kernel/audit.h b/kernel/audit.h > > index 162de8366b32..543f1334ba47 100644 > > --- a/kernel/audit.h > > +++ b/kernel/audit.h > > @@ -219,6 +219,10 @@ static inline int audit_hash_contid(u64 contid) > > return (contid & (AUDIT_CONTID_BUCKETS-1)); > > } > > > > +extern int audit_contid_count; > > + > > +#define AUDIT_CONTID_COUNT 1 << 16 > > + > Just to ask the question, since it wasn't clear in the changelog, what > abuse are you avoiding here? Ostensibly you should be able to create as > many container ids as you have space for, and the simple creation of > container ids doesn't seem like the resource strain I would be concerned > about here, given that an orchestrator can still create as many > containers as the system will otherwise allow, which will consume > significantly more ram/disk/etc. Agreed. I'm not a huge fan of this, but included it as an optional patch to address resource abuse concerns of Eric Beiderman. I'll push it to the end of the patchset and make it clear it is optional unless I hear a compelling reason to keep it. A similar argument was used to make the audit queue length tunable parameter unlimited. > > /* Indicates that audit should log the full pathname. */ > > #define AUDIT_NAME_FULL -1 > > > > -- > > 1.8.3.1 - 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
On Thu, Oct 24, 2019 at 5:23 PM Richard Guy Briggs <rgb@redhat.com> wrote: > On 2019-10-10 20:38, Paul Moore wrote: > > On Fri, Sep 27, 2019 at 8:52 AM Neil Horman <nhorman@tuxdriver.com> wrote: > > > On Wed, Sep 18, 2019 at 09:22:23PM -0400, Richard Guy Briggs wrote: > > > > Set an arbitrary limit on the number of audit container identifiers to > > > > limit abuse. > > > > > > > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com> > > > > --- > > > > kernel/audit.c | 8 ++++++++ > > > > kernel/audit.h | 4 ++++ > > > > 2 files changed, 12 insertions(+) > > > > > > > > diff --git a/kernel/audit.c b/kernel/audit.c > > > > index 53d13d638c63..329916534dd2 100644 > > > > --- a/kernel/audit.c > > > > +++ b/kernel/audit.c > > > > ... > > > > > > @@ -2465,6 +2472,7 @@ int audit_set_contid(struct task_struct *task, u64 contid) > > > > newcont->owner = current; > > > > refcount_set(&newcont->refcount, 1); > > > > list_add_rcu(&newcont->list, &audit_contid_hash[h]); > > > > + audit_contid_count++; > > > > } else { > > > > rc = -ENOMEM; > > > > goto conterror; > > > > diff --git a/kernel/audit.h b/kernel/audit.h > > > > index 162de8366b32..543f1334ba47 100644 > > > > --- a/kernel/audit.h > > > > +++ b/kernel/audit.h > > > > @@ -219,6 +219,10 @@ static inline int audit_hash_contid(u64 contid) > > > > return (contid & (AUDIT_CONTID_BUCKETS-1)); > > > > } > > > > > > > > +extern int audit_contid_count; > > > > + > > > > +#define AUDIT_CONTID_COUNT 1 << 16 > > > > + > > > > > > Just to ask the question, since it wasn't clear in the changelog, what > > > abuse are you avoiding here? Ostensibly you should be able to create as > > > many container ids as you have space for, and the simple creation of > > > container ids doesn't seem like the resource strain I would be concerned > > > about here, given that an orchestrator can still create as many > > > containers as the system will otherwise allow, which will consume > > > significantly more ram/disk/etc. > > > > I've got a similar question. Up to this point in the patchset, there > > is a potential issue of hash bucket chain lengths and traversing them > > with a spinlock held, but it seems like we shouldn't be putting an > > arbitrary limit on audit container IDs unless we have a good reason > > for it. If for some reason we do want to enforce a limit, it should > > probably be a tunable value like a sysctl, or similar. > > Can you separate and clarify the concerns here? "Why are you doing this?" is about as simple as I can pose the question. > I plan to move this patch to the end of the patchset and make it > optional, possibly adding a tuning mechanism. Like the migration from > /proc to netlink for loginuid/sessionid/contid/capcontid, this was Eric > Biederman's concern and suggested mitigation. Okay, let's just drop it. I *really* don't like this approach of tossing questionable stuff at the end of the patchset; I get why you are doing it, but I think we really need to focus on keeping this changeset small. If the number of ACIDs (heh) become unwieldy the right solution is to improve the algorithms/structures, if we can't do that for some reason, *then* we can fall back to a limiting knob in a latter release. > As for the first issue of the bucket chain length traversal while > holding the list spin-lock, would you prefer to use the rcu lock to > traverse the list and then only hold the spin-lock when modifying the > list, and possibly even make the spin-lock more fine-grained per list? Until we have a better idea of how this is going to be used, I think it's okay for now. It's also internal to the kernel so we can change it at any time. My comments about the locking/structs was only to try and think of some reason why one might want to limit the number of ACIDs since neither you or Eric provided any reasoning that I could see.
On 2019-11-08 12:49, Paul Moore wrote: > On Thu, Oct 24, 2019 at 5:23 PM Richard Guy Briggs <rgb@redhat.com> wrote: > > On 2019-10-10 20:38, Paul Moore wrote: > > > On Fri, Sep 27, 2019 at 8:52 AM Neil Horman <nhorman@tuxdriver.com> wrote: > > > > On Wed, Sep 18, 2019 at 09:22:23PM -0400, Richard Guy Briggs wrote: > > > > > Set an arbitrary limit on the number of audit container identifiers to > > > > > limit abuse. > > > > > > > > > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com> > > > > > --- > > > > > kernel/audit.c | 8 ++++++++ > > > > > kernel/audit.h | 4 ++++ > > > > > 2 files changed, 12 insertions(+) > > > > > > > > > > diff --git a/kernel/audit.c b/kernel/audit.c > > > > > index 53d13d638c63..329916534dd2 100644 > > > > > --- a/kernel/audit.c > > > > > +++ b/kernel/audit.c > > > > > > ... > > > > > > > > @@ -2465,6 +2472,7 @@ int audit_set_contid(struct task_struct *task, u64 contid) > > > > > newcont->owner = current; > > > > > refcount_set(&newcont->refcount, 1); > > > > > list_add_rcu(&newcont->list, &audit_contid_hash[h]); > > > > > + audit_contid_count++; > > > > > } else { > > > > > rc = -ENOMEM; > > > > > goto conterror; > > > > > diff --git a/kernel/audit.h b/kernel/audit.h > > > > > index 162de8366b32..543f1334ba47 100644 > > > > > --- a/kernel/audit.h > > > > > +++ b/kernel/audit.h > > > > > @@ -219,6 +219,10 @@ static inline int audit_hash_contid(u64 contid) > > > > > return (contid & (AUDIT_CONTID_BUCKETS-1)); > > > > > } > > > > > > > > > > +extern int audit_contid_count; > > > > > + > > > > > +#define AUDIT_CONTID_COUNT 1 << 16 > > > > > + > > > > > > > > Just to ask the question, since it wasn't clear in the changelog, what > > > > abuse are you avoiding here? Ostensibly you should be able to create as > > > > many container ids as you have space for, and the simple creation of > > > > container ids doesn't seem like the resource strain I would be concerned > > > > about here, given that an orchestrator can still create as many > > > > containers as the system will otherwise allow, which will consume > > > > significantly more ram/disk/etc. > > > > > > I've got a similar question. Up to this point in the patchset, there > > > is a potential issue of hash bucket chain lengths and traversing them > > > with a spinlock held, but it seems like we shouldn't be putting an > > > arbitrary limit on audit container IDs unless we have a good reason > > > for it. If for some reason we do want to enforce a limit, it should > > > probably be a tunable value like a sysctl, or similar. > > > > Can you separate and clarify the concerns here? > > "Why are you doing this?" is about as simple as I can pose the question. It was more of a concern for total system resources, primarily memory, but this is self-limiting and an arbitrary concern. The other limit of depth of nesting has different concerns that arise depending on how reporting is done. > > I plan to move this patch to the end of the patchset and make it > > optional, possibly adding a tuning mechanism. Like the migration from > > /proc to netlink for loginuid/sessionid/contid/capcontid, this was Eric > > Biederman's concern and suggested mitigation. > > Okay, let's just drop it. I *really* don't like this approach of > tossing questionable stuff at the end of the patchset; I get why you > are doing it, but I think we really need to focus on keeping this > changeset small. If the number of ACIDs (heh) become unwieldy the > right solution is to improve the algorithms/structures, if we can't do > that for some reason, *then* we can fall back to a limiting knob in a > latter release. Ok, I've dropped it. There are mitigations in place for large numbers of contids and it can be limited later without breaking anything. > > As for the first issue of the bucket chain length traversal while > > holding the list spin-lock, would you prefer to use the rcu lock to > > traverse the list and then only hold the spin-lock when modifying the > > list, and possibly even make the spin-lock more fine-grained per list? > > Until we have a better idea of how this is going to be used, I think > it's okay for now. It's also internal to the kernel so we can change > it at any time. My comments about the locking/structs was only to try > and think of some reason why one might want to limit the number of > ACIDs since neither you or Eric provided any reasoning that I could > see. I've switched to using an rcu read lock on the list traversal and spin-lock on list update. > 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
On Tuesday, December 17, 2019 1:45:41 PM EST Richard Guy Briggs wrote: > On 2019-11-08 12:49, Paul Moore wrote: > > On Thu, Oct 24, 2019 at 5:23 PM Richard Guy Briggs <rgb@redhat.com> wrote: > > > On 2019-10-10 20:38, Paul Moore wrote: > > > > On Fri, Sep 27, 2019 at 8:52 AM Neil Horman <nhorman@tuxdriver.com> wrote: > > > > > On Wed, Sep 18, 2019 at 09:22:23PM -0400, Richard Guy Briggs wrote: > > > > > > Set an arbitrary limit on the number of audit container > > > > > > identifiers to > > > > > > limit abuse. > > > > > > > > > > > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com> > > > > > > --- > > > > > > kernel/audit.c | 8 ++++++++ > > > > > > kernel/audit.h | 4 ++++ > > > > > > 2 files changed, 12 insertions(+) > > > > > > > > > > > > diff --git a/kernel/audit.c b/kernel/audit.c > > > > > > index 53d13d638c63..329916534dd2 100644 > > > > > > --- a/kernel/audit.c > > > > > > +++ b/kernel/audit.c > > > > > > > > ... > > > > > > > > > > @@ -2465,6 +2472,7 @@ int audit_set_contid(struct task_struct > > > > > > *task, u64 contid) newcont->owner = current; > > > > > > refcount_set(&newcont->refcount, 1); > > > > > > list_add_rcu(&newcont->list, &audit_contid_hash[h]); > > > > > > + audit_contid_count++; > > > > > > } else { > > > > > > rc = -ENOMEM; > > > > > > goto conterror; > > > > > > diff --git a/kernel/audit.h b/kernel/audit.h > > > > > > index 162de8366b32..543f1334ba47 100644 > > > > > > --- a/kernel/audit.h > > > > > > +++ b/kernel/audit.h > > > > > > @@ -219,6 +219,10 @@ static inline int audit_hash_contid(u64 > > > > > > contid) > > > > > > return (contid & (AUDIT_CONTID_BUCKETS-1)); > > > > > > } > > > > > > > > > > > > +extern int audit_contid_count; > > > > > > + > > > > > > +#define AUDIT_CONTID_COUNT 1 << 16 > > > > > > + > > > > > > > > > > Just to ask the question, since it wasn't clear in the changelog, > > > > > what > > > > > abuse are you avoiding here? Ostensibly you should be able to > > > > > create as > > > > > many container ids as you have space for, and the simple creation > > > > > of > > > > > container ids doesn't seem like the resource strain I would be > > > > > concerned > > > > > about here, given that an orchestrator can still create as many > > > > > containers as the system will otherwise allow, which will consume > > > > > significantly more ram/disk/etc. > > > > > > > > I've got a similar question. Up to this point in the patchset, there > > > > is a potential issue of hash bucket chain lengths and traversing them > > > > with a spinlock held, but it seems like we shouldn't be putting an > > > > arbitrary limit on audit container IDs unless we have a good reason > > > > for it. If for some reason we do want to enforce a limit, it should > > > > probably be a tunable value like a sysctl, or similar. > > > > > > Can you separate and clarify the concerns here? > > > > "Why are you doing this?" is about as simple as I can pose the question. > > It was more of a concern for total system resources, primarily memory, > but this is self-limiting and an arbitrary concern. > > The other limit of depth of nesting has different concerns that arise > depending on how reporting is done. Well, there is a limit on the audit record size. So, whatever is being sent in the record plus the size of the timestamp deducted from MAX_AUDIT_MESSAGE_LENGTH (8970) is the limit. That can be divided by however many ID's fit in that space and you have the real limit. -Steve -Steve
On 2019-12-17 14:25, Steve Grubb wrote: > On Tuesday, December 17, 2019 1:45:41 PM EST Richard Guy Briggs wrote: > > On 2019-11-08 12:49, Paul Moore wrote: > > > On Thu, Oct 24, 2019 at 5:23 PM Richard Guy Briggs <rgb@redhat.com> > wrote: > > > > On 2019-10-10 20:38, Paul Moore wrote: > > > > > On Fri, Sep 27, 2019 at 8:52 AM Neil Horman <nhorman@tuxdriver.com> > wrote: > > > > > > On Wed, Sep 18, 2019 at 09:22:23PM -0400, Richard Guy Briggs wrote: > > > > > > > Set an arbitrary limit on the number of audit container > > > > > > > identifiers to > > > > > > > limit abuse. > > > > > > > > > > > > > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com> > > > > > > > --- > > > > > > > kernel/audit.c | 8 ++++++++ > > > > > > > kernel/audit.h | 4 ++++ > > > > > > > 2 files changed, 12 insertions(+) > > > > > > > > > > > > > > diff --git a/kernel/audit.c b/kernel/audit.c > > > > > > > index 53d13d638c63..329916534dd2 100644 > > > > > > > --- a/kernel/audit.c > > > > > > > +++ b/kernel/audit.c > > > > > > > > > > ... > > > > > > > > > > > > @@ -2465,6 +2472,7 @@ int audit_set_contid(struct task_struct > > > > > > > *task, u64 contid) newcont->owner = current; > > > > > > > refcount_set(&newcont->refcount, 1); > > > > > > > list_add_rcu(&newcont->list, &audit_contid_hash[h]); > > > > > > > + audit_contid_count++; > > > > > > > } else { > > > > > > > rc = -ENOMEM; > > > > > > > goto conterror; > > > > > > > diff --git a/kernel/audit.h b/kernel/audit.h > > > > > > > index 162de8366b32..543f1334ba47 100644 > > > > > > > --- a/kernel/audit.h > > > > > > > +++ b/kernel/audit.h > > > > > > > @@ -219,6 +219,10 @@ static inline int audit_hash_contid(u64 > > > > > > > contid) > > > > > > > return (contid & (AUDIT_CONTID_BUCKETS-1)); > > > > > > > } > > > > > > > > > > > > > > +extern int audit_contid_count; > > > > > > > + > > > > > > > +#define AUDIT_CONTID_COUNT 1 << 16 > > > > > > > + > > > > > > > > > > > > Just to ask the question, since it wasn't clear in the changelog, > > > > > > what > > > > > > abuse are you avoiding here? Ostensibly you should be able to > > > > > > create as > > > > > > many container ids as you have space for, and the simple creation > > > > > > of > > > > > > container ids doesn't seem like the resource strain I would be > > > > > > concerned > > > > > > about here, given that an orchestrator can still create as many > > > > > > containers as the system will otherwise allow, which will consume > > > > > > significantly more ram/disk/etc. > > > > > > > > > > I've got a similar question. Up to this point in the patchset, there > > > > > is a potential issue of hash bucket chain lengths and traversing them > > > > > with a spinlock held, but it seems like we shouldn't be putting an > > > > > arbitrary limit on audit container IDs unless we have a good reason > > > > > for it. If for some reason we do want to enforce a limit, it should > > > > > probably be a tunable value like a sysctl, or similar. > > > > > > > > Can you separate and clarify the concerns here? > > > > > > "Why are you doing this?" is about as simple as I can pose the question. > > > > It was more of a concern for total system resources, primarily memory, > > but this is self-limiting and an arbitrary concern. > > > > The other limit of depth of nesting has different concerns that arise > > depending on how reporting is done. > > Well, there is a limit on the audit record size. So, whatever is being sent > in the record plus the size of the timestamp deducted from > MAX_AUDIT_MESSAGE_LENGTH (8970) is the limit. That can be divided by however > many ID's fit in that space and you have the real limit. This will be addressed in the v8 patch set. > -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
diff --git a/kernel/audit.c b/kernel/audit.c index 53d13d638c63..329916534dd2 100644 --- a/kernel/audit.c +++ b/kernel/audit.c @@ -139,6 +139,7 @@ struct audit_net { struct list_head audit_inode_hash[AUDIT_INODE_BUCKETS]; /* Hash for contid-based rules */ struct list_head audit_contid_hash[AUDIT_CONTID_BUCKETS]; +int audit_contid_count = 0; static struct kmem_cache *audit_buffer_cache; @@ -2384,6 +2385,7 @@ void audit_cont_put(struct audit_cont *cont) put_task_struct(cont->owner); list_del_rcu(&cont->list); kfree_rcu(cont, rcu); + audit_contid_count--; } } @@ -2456,6 +2458,11 @@ int audit_set_contid(struct task_struct *task, u64 contid) goto conterror; } } + /* Set max contids */ + if (audit_contid_count > AUDIT_CONTID_COUNT) { + rc = -ENOSPC; + goto conterror; + } if (!newcont) { newcont = kmalloc(sizeof(struct audit_cont), GFP_ATOMIC); if (newcont) { @@ -2465,6 +2472,7 @@ int audit_set_contid(struct task_struct *task, u64 contid) newcont->owner = current; refcount_set(&newcont->refcount, 1); list_add_rcu(&newcont->list, &audit_contid_hash[h]); + audit_contid_count++; } else { rc = -ENOMEM; goto conterror; diff --git a/kernel/audit.h b/kernel/audit.h index 162de8366b32..543f1334ba47 100644 --- a/kernel/audit.h +++ b/kernel/audit.h @@ -219,6 +219,10 @@ static inline int audit_hash_contid(u64 contid) return (contid & (AUDIT_CONTID_BUCKETS-1)); } +extern int audit_contid_count; + +#define AUDIT_CONTID_COUNT 1 << 16 + /* Indicates that audit should log the full pathname. */ #define AUDIT_NAME_FULL -1
Set an arbitrary limit on the number of audit container identifiers to limit abuse. Signed-off-by: Richard Guy Briggs <rgb@redhat.com> --- kernel/audit.c | 8 ++++++++ kernel/audit.h | 4 ++++ 2 files changed, 12 insertions(+)