[ghak90,V7,06/21] audit: contid limit of 32k imposed to avoid DoS

Submitted by Richard Guy Briggs on Sept. 19, 2019, 1:22 a.m.

Details

Message ID 230e91cd3e50a3d8015daac135c24c4c58cf0a21.1568834524.git.rgb@redhat.com
State New
Series "audit: implement container identifier"
Headers show

Commit Message

Richard Guy Briggs Sept. 19, 2019, 1:22 a.m.
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(+)

Patch hide | download patch | download mbox

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
 

Comments

Neil Horman Sept. 27, 2019, 12:51 p.m.
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
> 
>
Paul Moore Oct. 11, 2019, 12:38 a.m.
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
Richard Guy Briggs Oct. 24, 2019, 9:23 p.m.
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
Richard Guy Briggs Oct. 25, 2019, 8:15 p.m.
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
Paul Moore Nov. 8, 2019, 5:49 p.m.
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.
Richard Guy Briggs Dec. 17, 2019, 6:45 p.m.
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
Steve Grubb Dec. 17, 2019, 7:25 p.m.
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
Richard Guy Briggs Dec. 17, 2019, 7:56 p.m.
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