2:06 p.m.
Re-named confusing local variable names (status_set and status_get didn't agree
with their command type name) and reduced their scope.
Future-proof API changes by not depending on the exact size of the audit_status
struct.
Signed-off-by: Richard Guy Briggs <rgb(a)redhat.com>
---
kernel/audit.c | 51 +++++++++++++++++++++++++++------------------------
1 files changed, 27 insertions(+), 24 deletions(-)
diff --git a/kernel/audit.c b/kernel/audit.c
index acfa7a9..3d17670 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -635,7 +635,6 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr
*nlh)
{
u32 seq;
void *data;
- struct audit_status *status_get, status_set;
int err;
struct audit_buffer *ab;
u16 msg_type = nlh->nlmsg_type;
@@ -661,47 +660,51 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr
*nlh)
data = nlmsg_data(nlh);
switch (msg_type) {
- case AUDIT_GET:
- status_set.enabled = audit_enabled;
- status_set.failure = audit_failure;
- status_set.pid = audit_pid;
- status_set.rate_limit = audit_rate_limit;
- status_set.backlog_limit = audit_backlog_limit;
- status_set.lost = atomic_read(&audit_lost);
- status_set.backlog = skb_queue_len(&audit_skb_queue);
+ case AUDIT_GET: {
+ struct audit_status s;
+ s.enabled = audit_enabled;
+ s.failure = audit_failure;
+ s.pid = audit_pid;
+ s.rate_limit = audit_rate_limit;
+ s.backlog_limit = audit_backlog_limit;
+ s.lost = atomic_read(&audit_lost);
+ s.backlog = skb_queue_len(&audit_skb_queue);
audit_send_reply(NETLINK_CB(skb).portid, seq, AUDIT_GET, 0, 0,
- &status_set, sizeof(status_set));
+ &s, sizeof(s));
break;
- case AUDIT_SET:
- if (nlh->nlmsg_len < sizeof(struct audit_status))
- return -EINVAL;
- status_get = (struct audit_status *)data;
- if (status_get->mask & AUDIT_STATUS_ENABLED) {
- err = audit_set_enabled(status_get->enabled);
+ }
+ case AUDIT_SET: {
+ struct audit_status s;
+ memset(&s, 0, sizeof(s));
+ /* guard against past and future API changes */
+ memcpy(&s, data, min(sizeof(s), (size_t)nlh->nlmsg_len));
+ if (s.mask & AUDIT_STATUS_ENABLED) {
+ err = audit_set_enabled(s.enabled);
if (err < 0)
return err;
}
- if (status_get->mask & AUDIT_STATUS_FAILURE) {
- err = audit_set_failure(status_get->failure);
+ if (s.mask & AUDIT_STATUS_FAILURE) {
+ err = audit_set_failure(s.failure);
if (err < 0)
return err;
}
- if (status_get->mask & AUDIT_STATUS_PID) {
- int new_pid = status_get->pid;
+ if (s.mask & AUDIT_STATUS_PID) {
+ int new_pid = s.pid;
if (audit_enabled != AUDIT_OFF)
audit_log_config_change("audit_pid", new_pid, audit_pid, 1);
audit_pid = new_pid;
audit_nlk_portid = NETLINK_CB(skb).portid;
}
- if (status_get->mask & AUDIT_STATUS_RATE_LIMIT) {
- err = audit_set_rate_limit(status_get->rate_limit);
+ if (s.mask & AUDIT_STATUS_RATE_LIMIT) {
+ err = audit_set_rate_limit(s.rate_limit);
if (err < 0)
return err;
}
- if (status_get->mask & AUDIT_STATUS_BACKLOG_LIMIT)
- err = audit_set_backlog_limit(status_get->backlog_limit);
+ if (s.mask & AUDIT_STATUS_BACKLOG_LIMIT)
+ err = audit_set_backlog_limit(s.backlog_limit);
break;
+ }
case AUDIT_USER:
case AUDIT_FIRST_USER_MSG ... AUDIT_LAST_USER_MSG:
case AUDIT_FIRST_USER_MSG2 ... AUDIT_LAST_USER_MSG2:
--
1.7.1
4:18 p.m.
On Wednesday, September 18, 2013 03:06:52 PM Richard Guy Briggs wrote:
Re-named confusing local variable names (status_set and status_get
didn't
agree with their command type name) and reduced their scope.
Future-proof API changes by not depending on the exact size of the
audit_status struct.
I wished things like this were coordinated before being written. We had some
discussion of this back in July under a topic, "audit: implement generic
feature setting and retrieving". Maybe that API can be fixed so its not just
set/unset but can take a number as well.
Also, because there is no way to query the kernel to see what kind of things
it supports, we've typically defined a new message type and put into it exactly
what we need. In other words, if you want something expandable, the define a
new message type like AUDIT_GET_EXT and AUDIT_SET_EXT and build it to be
expandable.
Then in a future version of auditctl it will try to use the new command and
fall back to the old one if it gets EINVAL. Then some years later the old GET
and SET can be deprecated. But the audit code base has to support a wide
variety of kernels and suddenly making on resizable might break old code on
new kernel. A new message type is a safer migration path.
-Steve
Signed-off-by: Richard Guy Briggs <rgb(a)redhat.com>
---
kernel/audit.c | 51 +++++++++++++++++++++++++++------------------------
1 files changed, 27 insertions(+), 24 deletions(-)
diff --git a/kernel/audit.c b/kernel/audit.c
index acfa7a9..3d17670 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -635,7 +635,6 @@ static int audit_receive_msg(struct sk_buff *skb, struct
nlmsghdr *nlh) {
u32 seq;
void *data;
- struct audit_status *status_get, status_set;
int err;
struct audit_buffer *ab;
u16 msg_type = nlh->nlmsg_type;
@@ -661,47 +660,51 @@ static int audit_receive_msg(struct sk_buff *skb,
struct nlmsghdr *nlh) data = nlmsg_data(nlh);
switch (msg_type) {
- case AUDIT_GET:
- status_set.enabled = audit_enabled;
- status_set.failure = audit_failure;
- status_set.pid = audit_pid;
- status_set.rate_limit = audit_rate_limit;
- status_set.backlog_limit = audit_backlog_limit;
- status_set.lost = atomic_read(&audit_lost);
- status_set.backlog = skb_queue_len(&audit_skb_queue);
+ case AUDIT_GET: {
+ struct audit_status s;
+ s.enabled = audit_enabled;
+ s.failure = audit_failure;
+ s.pid = audit_pid;
+ s.rate_limit = audit_rate_limit;
+ s.backlog_limit = audit_backlog_limit;
+ s.lost = atomic_read(&audit_lost);
+ s.backlog = skb_queue_len(&audit_skb_queue);
audit_send_reply(NETLINK_CB(skb).portid, seq, AUDIT_GET, 0, 0,
- &status_set, sizeof(status_set));
+ &s, sizeof(s));
break;
- case AUDIT_SET:
- if (nlh->nlmsg_len < sizeof(struct audit_status))
- return -EINVAL;
- status_get = (struct audit_status *)data;
- if (status_get->mask & AUDIT_STATUS_ENABLED) {
- err = audit_set_enabled(status_get->enabled);
+ }
+ case AUDIT_SET: {
+ struct audit_status s;
+ memset(&s, 0, sizeof(s));
+ /* guard against past and future API changes */
+ memcpy(&s, data, min(sizeof(s), (size_t)nlh->nlmsg_len));
+ if (s.mask & AUDIT_STATUS_ENABLED) {
+ err = audit_set_enabled(s.enabled);
if (err < 0)
return err;
}
- if (status_get->mask & AUDIT_STATUS_FAILURE) {
- err = audit_set_failure(status_get->failure);
+ if (s.mask & AUDIT_STATUS_FAILURE) {
+ err = audit_set_failure(s.failure);
if (err < 0)
return err;
}
- if (status_get->mask & AUDIT_STATUS_PID) {
- int new_pid = status_get->pid;
+ if (s.mask & AUDIT_STATUS_PID) {
+ int new_pid = s.pid;
if (audit_enabled != AUDIT_OFF)
audit_log_config_change("audit_pid", new_pid, audit_pid,
1);
audit_pid = new_pid;
audit_nlk_portid = NETLINK_CB(skb).portid;
}
- if (status_get->mask & AUDIT_STATUS_RATE_LIMIT) {
- err = audit_set_rate_limit(status_get->rate_limit);
+ if (s.mask & AUDIT_STATUS_RATE_LIMIT) {
+ err = audit_set_rate_limit(s.rate_limit);
if (err < 0)
return err;
}
- if (status_get->mask & AUDIT_STATUS_BACKLOG_LIMIT)
- err = audit_set_backlog_limit(status_get->backlog_limit);
+ if (s.mask & AUDIT_STATUS_BACKLOG_LIMIT)
+ err = audit_set_backlog_limit(s.backlog_limit);
break;
+ }
case AUDIT_USER:
case AUDIT_FIRST_USER_MSG ... AUDIT_LAST_USER_MSG:
case AUDIT_FIRST_USER_MSG2 ... AUDIT_LAST_USER_MSG2:
9:47 a.m.
On Thu, 2013-09-19 at 17:18 -0400, Steve Grubb wrote:
On Wednesday, September 18, 2013 03:06:52 PM Richard Guy Briggs
wrote:
> Re-named confusing local variable names (status_set and status_get didn't
> agree with their command type name) and reduced their scope.
>
> Future-proof API changes by not depending on the exact size of the
> audit_status struct.
I wished things like this were coordinated before being written. We had some
discussion of this back in July under a topic, "audit: implement generic
feature setting and retrieving". Maybe that API can be fixed so its not just
set/unset but can take a number as well.
Your right, we did talk about it. Though it seems we talked past each
other. What was implemented was an on off extensible interface. The
status interface already fits for setting numbers. And because of how
it is used has been extended is is extensible for setting numbers.
Also, because there is no way to query the kernel to see what kind of things
it supports
I'll agree. Richard, can you please add a version field to the status?
Start at version 1. Any time we add a new audit feature we'll bump it.
, we've typically defined a new message type and put into it
exactly
what we need. In other words, if you want something expandable, the define a
new message type like AUDIT_GET_EXT and AUDIT_SET_EXT and build it to be
expandable.
There is no reason we can't use what we have. (As we've done it before)
11:38 a.m.
On Fri, Sep 20, 2013 at 10:47:50AM -0400, Eric Paris wrote:
On Thu, 2013-09-19 at 17:18 -0400, Steve Grubb wrote:
> On Wednesday, September 18, 2013 03:06:52 PM Richard Guy Briggs wrote:
> > Re-named confusing local variable names (status_set and status_get didn't
> > agree with their command type name) and reduced their scope.
> >
> > Future-proof API changes by not depending on the exact size of the
> > audit_status struct.
> Also, because there is no way to query the kernel to see what
kind of things
> it supports
I'll agree. Richard, can you please add a version field to the status?
Start at version 1. Any time we add a new audit feature we'll bump it.
Done. Adding the interface and version field is version 1, so without,
it is zero. Adding the backlog timeout is version 2.
- RGB
--
Richard Guy Briggs <rbriggs(a)redhat.com>
Senior Software Engineer
Kernel Security
AMER ENG Base Operating Systems
Remote, Ottawa, Canada
Voice: +1.647.777.2635
Internal: (81) 32635
Alt: +1.613.693.0684x3545
4056
days inactive
4061
days old
linux-audit@lists.linux-audit.osci.io
3 comments
3 participants
participants (3)
-
Eric Paris -
Richard Guy Briggs -
Steve Grubb