12:02 p.m.
A regression was caused by commit 780a7654cee8:
audit: Make testing for a valid loginuid explicit.
(which in turn attempted to fix a regression caused by e1760bd)
When audit_krule_to_data() fills in the rules to get a listing, there was a
missing clause to convert back from AUDIT_LOGINUID_SET to AUDIT_LOGINUID.
This broke userspace by not returning the same information that was sent and
expected.
The rule:
auditctl -a exit,never -F auid=-1
gives:
auditctl -l
LIST_RULES: exit,never f24=0 syscall=all
when it should give:
LIST_RULES: exit,never auid=-1 (0xffffffff) syscall=all
Tag it so that it is reported the same way it was set. Create a new
private flags audit_krule field (pflags) to store it that won't interact with
the public one from the API.
Cc: stable(a)vger.kernel.org # v3.10-rc1+
Signed-off-by: Richard Guy Briggs <rgb(a)redhat.com>
---
include/linux/audit.h | 4 ++++
kernel/auditfilter.c | 10 ++++++++++
2 files changed, 14 insertions(+), 0 deletions(-)
diff --git a/include/linux/audit.h b/include/linux/audit.h
index eefc39a..b481779 100644
--- a/include/linux/audit.h
+++ b/include/linux/audit.h
@@ -46,6 +46,7 @@ struct audit_tree;
struct sk_buff;
struct audit_krule {
+ u32 pflags;
u32 flags;
u32 listnr;
u32 action;
@@ -63,6 +64,9 @@ struct audit_krule {
u64 prio;
};
+/* Flag to indicate legacy AUDIT_LOGINUID unset usage */
+#define AUDIT_LOGINUID_LEGACY 0x1
+
struct audit_field {
u32 type;
union {
diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
index fb4d2df..ecb05d3 100644
--- a/kernel/auditfilter.c
+++ b/kernel/auditfilter.c
@@ -441,6 +441,7 @@ static struct audit_entry *audit_data_to_entry(struct audit_rule_data
*data,
if ((f->type == AUDIT_LOGINUID) && (f->val == AUDIT_UID_UNSET)) {
f->type = AUDIT_LOGINUID_SET;
f->val = 0;
+ entry->rule.pflags |= AUDIT_LOGINUID_LEGACY;
}
if ((f->type == AUDIT_PID) || (f->type == AUDIT_PPID)) {
@@ -629,6 +630,13 @@ static struct audit_rule_data *audit_krule_to_data(struct audit_krule
*krule)
data->buflen += data->values[i] =
audit_pack_string(&bufp, krule->filterkey);
break;
+ case AUDIT_LOGINUID_SET:
+ if (krule->pflags & AUDIT_LOGINUID_LEGACY && !f->val) {
+ data->fields[i] = AUDIT_LOGINUID;
+ data->values[i] = AUDIT_UID_UNSET;
+ break;
+ }
+ /* fallthrough if set */
default:
data->values[i] = f->val;
}
@@ -645,6 +653,7 @@ static int audit_compare_rule(struct audit_krule *a, struct
audit_krule *b)
int i;
if (a->flags != b->flags ||
+ a->pflags != b->pflags ||
a->listnr != b->listnr ||
a->action != b->action ||
a->field_count != b->field_count)
@@ -762,6 +771,7 @@ struct audit_entry *audit_dupe_rule(struct audit_krule *old)
new = &entry->rule;
new->flags = old->flags;
+ new->pflags = old->pflags;
new->listnr = old->listnr;
new->action = old->action;
for (i = 0; i < AUDIT_BITMASK_SIZE; i++)
--
1.7.1
12:20 p.m.
New subject: [PATCH 2/2] audit: shrink audit_krule by using smaller bitfields
Replace five 32-bit fields with one. Move a nearby 32-bit field to enable
64-bit alignment.
Signed-off-by: Richard Guy Briggs <rgb(a)redhat.com>
---
include/linux/audit.h | 13 +++++++------
1 files changed, 7 insertions(+), 6 deletions(-)
diff --git a/include/linux/audit.h b/include/linux/audit.h
index b481779..bd06f92 100644
--- a/include/linux/audit.h
+++ b/include/linux/audit.h
@@ -46,13 +46,14 @@ struct audit_tree;
struct sk_buff;
struct audit_krule {
- u32 pflags;
- u32 flags;
- u32 listnr;
- u32 action;
- u32 mask[AUDIT_BITMASK_SIZE];
+ u32 listnr:4,
+ flags:5,
+ action:2,
+ pflags:1,
+ field_count:7,
+ reserved:13;
u32 buflen; /* for data alloc on list rules */
- u32 field_count;
+ u32 mask[AUDIT_BITMASK_SIZE];
char *filterkey; /* ties events to rules */
struct audit_field *fields;
struct audit_field *arch_f; /* quick access to arch field */
--
1.7.1
12:33 p.m.
New subject: [PATCH 2/2] audit: shrink audit_krule by using smaller bitfields
It's probably not a problem, but one needs to remember that all
updates to all bit fields need to be under the same lock/protection.
Bitfields + concurrent access is dangerous, but may well be
appropriate in this case.
-Eric
On Tue, Dec 23, 2014 at 1:20 PM, Richard Guy Briggs <rgb(a)redhat.com> wrote:
Replace five 32-bit fields with one. Move a nearby 32-bit field to
enable
64-bit alignment.
Signed-off-by: Richard Guy Briggs <rgb(a)redhat.com>
---
include/linux/audit.h | 13 +++++++------
1 files changed, 7 insertions(+), 6 deletions(-)
diff --git a/include/linux/audit.h b/include/linux/audit.h
index b481779..bd06f92 100644
--- a/include/linux/audit.h
+++ b/include/linux/audit.h
@@ -46,13 +46,14 @@ struct audit_tree;
struct sk_buff;
struct audit_krule {
- u32 pflags;
- u32 flags;
- u32 listnr;
- u32 action;
- u32 mask[AUDIT_BITMASK_SIZE];
+ u32 listnr:4,
+ flags:5,
+ action:2,
+ pflags:1,
+ field_count:7,
+ reserved:13;
u32 buflen; /* for data alloc on list rules */
- u32 field_count;
+ u32 mask[AUDIT_BITMASK_SIZE];
char *filterkey; /* ties events to rules */
struct audit_field *fields;
struct audit_field *arch_f; /* quick access to arch field */
--
1.7.1
2:42 p.m.
New subject: [PATCH 2/2] audit: shrink audit_krule by using smaller bitfields
On 14/12/23, Paris, Eric wrote:
It's probably not a problem, but one needs to remember that all
updates to all bit fields need to be under the same lock/protection.
Bitfields + concurrent access is dangerous, but may well be
appropriate in this case.
Good point. I'll go back and check for any issues, but as you point
out, in this case it may be appropriate. All these fields should only
change during a rule addition in a newly allocated struct (under
audit_cmd_mutex) before they are referenceable by other parts of the
code.
Thanks, Eric.
-Eric
On Tue, Dec 23, 2014 at 1:20 PM, Richard Guy Briggs <rgb(a)redhat.com> wrote:
> Replace five 32-bit fields with one. Move a nearby 32-bit field to enable
> 64-bit alignment.
>
> Signed-off-by: Richard Guy Briggs <rgb(a)redhat.com>
> ---
> include/linux/audit.h | 13 +++++++------
> 1 files changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index b481779..bd06f92 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -46,13 +46,14 @@ struct audit_tree;
> struct sk_buff;
>
> struct audit_krule {
> - u32 pflags;
> - u32 flags;
> - u32 listnr;
> - u32 action;
> - u32 mask[AUDIT_BITMASK_SIZE];
> + u32 listnr:4,
> + flags:5,
> + action:2,
> + pflags:1,
> + field_count:7,
> + reserved:13;
> u32 buflen; /* for data alloc on list rules */
> - u32 field_count;
> + u32 mask[AUDIT_BITMASK_SIZE];
> char *filterkey; /* ties events to rules */
> struct audit_field *fields;
> struct audit_field *arch_f; /* quick access to arch field */
> --
> 1.7.1
- RGB
--
Richard Guy Briggs <rbriggs(a)redhat.com>
Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545
1:04 p.m.
New subject: [PATCH 2/2] audit: shrink audit_krule by using smaller bitfields
On Tuesday, December 23, 2014 01:20:15 PM Richard Guy Briggs wrote:
Replace five 32-bit fields with one. Move a nearby 32-bit field to
enable
64-bit alignment.
Are there performance impacts from handling bit fields? The syscall audit rules
affect each syscall of each program running. Also, setting bitfields so small
kind of boxes in any future capability enhancements.
-Steve
Signed-off-by: Richard Guy Briggs <rgb(a)redhat.com>
---
include/linux/audit.h | 13 +++++++------
1 files changed, 7 insertions(+), 6 deletions(-)
diff --git a/include/linux/audit.h b/include/linux/audit.h
index b481779..bd06f92 100644
--- a/include/linux/audit.h
+++ b/include/linux/audit.h
@@ -46,13 +46,14 @@ struct audit_tree;
struct sk_buff;
struct audit_krule {
- u32 pflags;
- u32 flags;
- u32 listnr;
- u32 action;
- u32 mask[AUDIT_BITMASK_SIZE];
+ u32 listnr:4,
+ flags:5,
+ action:2,
+ pflags:1,
+ field_count:7,
+ reserved:13;
u32 buflen; /* for data alloc on list rules */
- u32 field_count;
+ u32 mask[AUDIT_BITMASK_SIZE];
char *filterkey; /* ties events to rules */
struct audit_field *fields;
struct audit_field *arch_f; /* quick access to arch field */
2:43 p.m.
New subject: [PATCH 2/2] audit: shrink audit_krule by using smaller bitfields
On 14/12/23, Steve Grubb wrote:
On Tuesday, December 23, 2014 01:20:15 PM Richard Guy Briggs wrote:
> Replace five 32-bit fields with one. Move a nearby 32-bit field to enable
> 64-bit alignment.
Are there performance impacts from handling bit fields? The syscall audit rules
affect each syscall of each program running.
Quite possibly. I honestly don't know. I agree testing this could be
quite insightful. Given the size of the mask field, this optimization
does seem minor.
Also, setting bitfields so small kind of boxes in any future
capability enhancements.
I don't see any problem there since there are 13 bits left over in the
reserved field from which we can pull should there be any future needs.
Since these are internal structures, enlarging them in the future should
present no special problems. listnr is already limited to 4 bits by the
prepend flag value of 0x10 passed in with it. field_count is already
taking 7 bits to represent 64 (rather than 6 bits for 63) so that one
could go to 127... action still has one value left before needing
expansion. flags could use the bottom 4 bits, but that seems
unnecessary since expanding it bitwidth is trivial.
-Steve
> Signed-off-by: Richard Guy Briggs <rgb(a)redhat.com>
> ---
> include/linux/audit.h | 13 +++++++------
> 1 files changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index b481779..bd06f92 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -46,13 +46,14 @@ struct audit_tree;
> struct sk_buff;
>
> struct audit_krule {
> - u32 pflags;
> - u32 flags;
> - u32 listnr;
> - u32 action;
> - u32 mask[AUDIT_BITMASK_SIZE];
> + u32 listnr:4,
> + flags:5,
> + action:2,
> + pflags:1,
> + field_count:7,
> + reserved:13;
> u32 buflen; /* for data alloc on list rules */
> - u32 field_count;
> + u32 mask[AUDIT_BITMASK_SIZE];
> char *filterkey; /* ties events to rules */
> struct audit_field *fields;
> struct audit_field *arch_f; /* quick access to arch field */
- RGB
--
Richard Guy Briggs <rbriggs(a)redhat.com>
Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545
3:29 p.m.
New subject: [PATCH 2/2] audit: shrink audit_krule by using smaller bitfields
On Tuesday, December 23, 2014 01:20:15 PM Richard Guy Briggs wrote:
Replace five 32-bit fields with one. Move a nearby 32-bit field to
enable
64-bit alignment.
Signed-off-by: Richard Guy Briggs <rgb(a)redhat.com>
---
include/linux/audit.h | 13 +++++++------
1 files changed, 7 insertions(+), 6 deletions(-)
Let's leave this alone for right now. I expect to be doing some work on
cleaning up audit in the near-ish future so let's hold off until we have a
better idea of how things are going to look.
--
paul moore
security @ redhat
3:26 p.m.
On Tuesday, December 23, 2014 01:02:04 PM Richard Guy Briggs wrote:
A regression was caused by commit 780a7654cee8:
audit: Make testing for a valid loginuid explicit.
(which in turn attempted to fix a regression caused by e1760bd)
When audit_krule_to_data() fills in the rules to get a listing, there was a
missing clause to convert back from AUDIT_LOGINUID_SET to AUDIT_LOGINUID.
This broke userspace by not returning the same information that was sent and
expected.
The rule:
auditctl -a exit,never -F auid=-1
gives:
auditctl -l
LIST_RULES: exit,never f24=0 syscall=all
when it should give:
LIST_RULES: exit,never auid=-1 (0xffffffff) syscall=all
Tag it so that it is reported the same way it was set. Create a new
private flags audit_krule field (pflags) to store it that won't interact
with the public one from the API.
Cc: stable(a)vger.kernel.org # v3.10-rc1+
Signed-off-by: Richard Guy Briggs <rgb(a)redhat.com>
---
include/linux/audit.h | 4 ++++
kernel/auditfilter.c | 10 ++++++++++
2 files changed, 14 insertions(+), 0 deletions(-)
Applied, thanks.
--
paul moore
security @ redhat
3:41 p.m.
Me likie much more.
On Tue, Dec 23, 2014 at 4:26 PM, Paul Moore <pmoore(a)redhat.com> wrote:
On Tuesday, December 23, 2014 01:02:04 PM Richard Guy Briggs wrote:
> A regression was caused by commit 780a7654cee8:
> audit: Make testing for a valid loginuid explicit.
> (which in turn attempted to fix a regression caused by e1760bd)
>
> When audit_krule_to_data() fills in the rules to get a listing, there was a
> missing clause to convert back from AUDIT_LOGINUID_SET to AUDIT_LOGINUID.
>
> This broke userspace by not returning the same information that was sent and
> expected.
>
> The rule:
> auditctl -a exit,never -F auid=-1
> gives:
> auditctl -l
> LIST_RULES: exit,never f24=0 syscall=all
> when it should give:
> LIST_RULES: exit,never auid=-1 (0xffffffff) syscall=all
>
> Tag it so that it is reported the same way it was set. Create a new
> private flags audit_krule field (pflags) to store it that won't interact
> with the public one from the API.
>
> Cc: stable(a)vger.kernel.org # v3.10-rc1+
> Signed-off-by: Richard Guy Briggs <rgb(a)redhat.com>
> ---
> include/linux/audit.h | 4 ++++
> kernel/auditfilter.c | 10 ++++++++++
> 2 files changed, 14 insertions(+), 0 deletions(-)
Applied, thanks.
--
paul moore
security @ redhat
3:46 p.m.
On Tuesday, December 23, 2014 04:41:42 PM Paris, Eric wrote:
Me likie much more.
Yeah, me too.
On Tue, Dec 23, 2014 at 4:26 PM, Paul Moore <pmoore(a)redhat.com>
wrote:
> On Tuesday, December 23, 2014 01:02:04 PM Richard Guy Briggs wrote:
>> A regression was caused by commit 780a7654cee8:
>> audit: Make testing for a valid loginuid explicit.
>>
>> (which in turn attempted to fix a regression caused by e1760bd)
>>
>> When audit_krule_to_data() fills in the rules to get a listing, there was
>> a
>> missing clause to convert back from AUDIT_LOGINUID_SET to AUDIT_LOGINUID.
>>
>> This broke userspace by not returning the same information that was sent
>> and expected.
>>
>> The rule:
>> auditctl -a exit,never -F auid=-1
>>
>> gives:
>> auditctl -l
>>
>> LIST_RULES: exit,never f24=0 syscall=all
>>
>> when it should give:
>> LIST_RULES: exit,never auid=-1 (0xffffffff) syscall=all
>>
>> Tag it so that it is reported the same way it was set. Create a new
>> private flags audit_krule field (pflags) to store it that won't interact
>> with the public one from the API.
>>
>> Cc: stable(a)vger.kernel.org # v3.10-rc1+
>> Signed-off-by: Richard Guy Briggs <rgb(a)redhat.com>
>> ---
>>
>> include/linux/audit.h | 4 ++++
>> kernel/auditfilter.c | 10 ++++++++++
>> 2 files changed, 14 insertions(+), 0 deletions(-)
>
> Applied, thanks.
>
> --
> paul moore
> security @ redhat
--
paul moore
security @ redhat
5:02 p.m.
On 14/12/23, Paul Moore wrote:
On Tuesday, December 23, 2014 04:41:42 PM Paris, Eric wrote:
> Me likie much more.
Yeah, me too.
Good, thanks for your patience guys.
> On Tue, Dec 23, 2014 at 4:26 PM, Paul Moore
<pmoore(a)redhat.com> wrote:
> > On Tuesday, December 23, 2014 01:02:04 PM Richard Guy Briggs wrote:
> >> A regression was caused by commit 780a7654cee8:
> >> audit: Make testing for a valid loginuid explicit.
> >>
> >> (which in turn attempted to fix a regression caused by e1760bd)
> >>
> >> When audit_krule_to_data() fills in the rules to get a listing, there was
> >> a
> >> missing clause to convert back from AUDIT_LOGINUID_SET to AUDIT_LOGINUID.
> >>
> >> This broke userspace by not returning the same information that was sent
> >> and expected.
> >>
> >> The rule:
> >> auditctl -a exit,never -F auid=-1
> >>
> >> gives:
> >> auditctl -l
> >>
> >> LIST_RULES: exit,never f24=0 syscall=all
> >>
> >> when it should give:
> >> LIST_RULES: exit,never auid=-1 (0xffffffff) syscall=all
> >>
> >> Tag it so that it is reported the same way it was set. Create a new
> >> private flags audit_krule field (pflags) to store it that won't
interact
> >> with the public one from the API.
> >>
> >> Cc: stable(a)vger.kernel.org # v3.10-rc1+
> >> Signed-off-by: Richard Guy Briggs <rgb(a)redhat.com>
> >> ---
> >>
> >> include/linux/audit.h | 4 ++++
> >> kernel/auditfilter.c | 10 ++++++++++
> >> 2 files changed, 14 insertions(+), 0 deletions(-)
> >
> > Applied, thanks.
> >
> > paul moore
paul moore
- RGB
--
Richard Guy Briggs <rbriggs(a)redhat.com>
Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545
3652
days inactive
3652
days old
linux-audit@lists.linux-audit.osci.io
10 comments
4 participants
participants (4)
-
Paris, Eric -
Paul Moore -
Richard Guy Briggs -
Steve Grubb