Discussion:
[PATCH 1/2] Revert "In auditctl, when resetting lost request status output afterwards"
(too old to reply)
Richard Guy Briggs
2017-11-23 00:00:56 UTC
Permalink
This reverts commit 56a708761347ba49ccdc2378d31133f01129f4f2.

Conflicts:
ChangeLog
---
ChangeLog | 1 +
src/auditctl.c | 7 ++++---
2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index de9ae56..4e9ca3d 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -4,6 +4,7 @@
- In auditctl, when resetting lost request status output afterwards
- AVC report from aureport was missing result column header (#1511606)
- Add SOFTWARE_UPDATE event
+- REVERT: In auditctl, when resetting lost request status output afterwards

2.8.1
- Fix NULL ptr dereference in audispd plugin_dir parser
diff --git a/src/auditctl.c b/src/auditctl.c
index 0b301c1..a1c05b5 100644
--- a/src/auditctl.c
+++ b/src/auditctl.c
@@ -1064,9 +1064,10 @@ process_keys:
#endif
break;
case 3:
- if ((rc = audit_reset_lost(fd)) >= 0)
- audit_request_status(fd);
- else {
+ if ((rc = audit_reset_lost(fd)) >= 0) {
+ audit_msg(LOG_INFO, "lost: %u", rc);
+ return -2;
+ } else {
audit_number_to_errmsg(rc, long_opts[lidx].name);
retval = -1;
}
--
1.8.3.1
Richard Guy Briggs
2017-11-23 00:00:57 UTC
Permalink
The kernel always returns negative values on error, so zero and anything
positive is valid success. Lost_reset returned a positive value at the
time of reset, including zero that got interpreted as success and
replaced with the packet sequence number "2".

Rename audit_send() to __audit_send() and pass the sequence number back
via a parameter rather than return value.

Have a new stub audit_send() call __audit_send() and mimic the previous
behaviour of audit_send().

There are legacy functions that actually use a sequence number:
audit_request_rules_list_data()
delete_all_rules()
audit_request_signal_info()
src/auditd.c:get_reply()
A number of others don't appear to need it, but expose it in libaudit:
audit_send_user_message()
audit_log_user_comm_message()
audit_log_acct_message()
audit_log_user_avc_message()
audit_log_semanage_message()
audit_log_user_command()
audit_request_status()
audit_set_enabled()
audit_set_failure()
audit_set_rate_limit()
audit_set_backlog_limit()
audit_set_backlog_wait_time()
audit_add_rule_data()
audit_delete_rule_data()

Passes all audit-testsuite tests.

See: https://github.com/linux-audit/audit-userspace/issues/31

Signed-off-by: Richard Guy Briggs <***@redhat.com>
---
lib/libaudit.c | 3 ++-
lib/netlink.c | 28 ++++++++++++++++++++--------
lib/private.h | 1 +
3 files changed, 23 insertions(+), 9 deletions(-)

diff --git a/lib/libaudit.c b/lib/libaudit.c
index a9ba575..aa8258c 100644
--- a/lib/libaudit.c
+++ b/lib/libaudit.c
@@ -519,6 +519,7 @@ int audit_set_backlog_wait_time(int fd, uint32_t bwt)
int audit_reset_lost(int fd)
{
int rc;
+ int seq;
struct audit_status s;

if ((audit_get_features() & AUDIT_FEATURE_BITMAP_LOST_RESET) == 0)
@@ -527,7 +528,7 @@ int audit_reset_lost(int fd)
memset(&s, 0, sizeof(s));
s.mask = AUDIT_STATUS_LOST;
s.lost = 0;
- rc = audit_send(fd, AUDIT_SET, &s, sizeof(s));
+ rc = __audit_send(fd, AUDIT_SET, &s, sizeof(s), &seq);
if (rc < 0)
audit_msg(audit_priority(errno),
"Error sending lost reset request (%s)",
diff --git a/lib/netlink.c b/lib/netlink.c
index 6e23883..5b2028f 100644
--- a/lib/netlink.c
+++ b/lib/netlink.c
@@ -203,7 +203,7 @@ static int adjust_reply(struct audit_reply *rep, int len)
* error: -errno
* short: 0
*/
-int audit_send(int fd, int type, const void *data, unsigned int size)
+int __audit_send(int fd, int type, const void *data, unsigned int size, int *seq)
{
static int sequence = 0;
struct audit_message req;
@@ -224,6 +224,7 @@ int audit_send(int fd, int type, const void *data, unsigned int size)

if (++sequence < 0)
sequence = 1;
+ *seq = sequence;

memset(&req, 0, sizeof(req));
req.nlh.nlmsg_len = NLMSG_SPACE(size);
@@ -241,18 +242,29 @@ int audit_send(int fd, int type, const void *data, unsigned int size)
retval = sendto(fd, &req, req.nlh.nlmsg_len, 0,
(struct sockaddr*)&addr, sizeof(addr));
} while (retval < 0 && errno == EINTR);
- if (retval == (int)req.nlh.nlmsg_len) {
- if ((retval = check_ack(fd)) == 0)
- return sequence;
- else
- return retval;
- }
- if (retval < 0)
+ if (retval == (int)req.nlh.nlmsg_len)
+ return check_ack(fd);
+ if (retval < 0) {
return -errno;
+ } else if (retval > 0) {
+ errno = EINVAL;
+ return -errno;
+ }

return 0;
}

+int audit_send(int fd, int type, const void *data, unsigned int size)
+{
+ int rc;
+ int seq;
+
+ rc = __audit_send(fd, type, data, size, &seq);
+ if (rc == 0)
+ rc = seq;
+ return rc;
+}
+
/*
* This function will take a peek into the next packet and see if there's
* an error. If so, the error is returned and its non-zero. Otherwise a
diff --git a/lib/private.h b/lib/private.h
index dbe0f74..560740f 100644
--- a/lib/private.h
+++ b/lib/private.h
@@ -121,6 +121,7 @@ void audit_msg(int priority, const char *fmt, ...)
#endif

extern int audit_send(int fd, int type, const void *data, unsigned int size);
+extern int __audit_send(int fd, int type, const void *data, unsigned int size, int *seq);

AUDIT_HIDDEN_START
--
1.8.3.1
Steve Grubb
2017-11-27 22:16:53 UTC
Permalink
Post by Richard Guy Briggs
The kernel always returns negative values on error, so zero and anything
positive is valid success. Lost_reset returned a positive value at the
time of reset, including zero that got interpreted as success and
replaced with the packet sequence number "2".
Rename audit_send() to __audit_send() and pass the sequence number back
via a parameter rather than return value.
Have a new stub audit_send() call __audit_send() and mimic the previous
behaviour of audit_send()
Applied,

Thanks,
-Steve
Steve Grubb
2017-11-27 22:16:19 UTC
Permalink
Post by Richard Guy Briggs
This reverts commit 56a708761347ba49ccdc2378d31133f01129f4f2.
ChangeLog
---
ChangeLog | 1 +
src/auditctl.c | 7 ++++---
2 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/ChangeLog b/ChangeLog
index de9ae56..4e9ca3d 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -4,6 +4,7 @@
- In auditctl, when resetting lost request status output afterwards
- AVC report from aureport was missing result column header (#1511606)
- Add SOFTWARE_UPDATE event
+- REVERT: In auditctl, when resetting lost request status output afterwards
Applied. But its best not to mess with the Changelog because there is no
guarantee this patch goes in before other work. As it turns out, it failed to
apply and then I deleted this chunk and everything was OK.

-Steve
Post by Richard Guy Briggs
2.8.1
- Fix NULL ptr dereference in audispd plugin_dir parser
diff --git a/src/auditctl.c b/src/auditctl.c
index 0b301c1..a1c05b5 100644
--- a/src/auditctl.c
+++ b/src/auditctl.c
#endif
break;
- if ((rc = audit_reset_lost(fd)) >= 0)
- audit_request_status(fd);
- else {
+ if ((rc = audit_reset_lost(fd)) >= 0) {
+ audit_msg(LOG_INFO, "lost: %u", rc);
+ return -2;
+ } else {
audit_number_to_errmsg(rc, long_opts[lidx].name);
retval = -1;
}
Richard Guy Briggs
2017-11-27 22:41:32 UTC
Permalink
Post by Steve Grubb
Post by Richard Guy Briggs
This reverts commit 56a708761347ba49ccdc2378d31133f01129f4f2.
ChangeLog
---
ChangeLog | 1 +
src/auditctl.c | 7 ++++---
2 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/ChangeLog b/ChangeLog
index de9ae56..4e9ca3d 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -4,6 +4,7 @@
- In auditctl, when resetting lost request status output afterwards
- AVC report from aureport was missing result column header (#1511606)
- Add SOFTWARE_UPDATE event
+- REVERT: In auditctl, when resetting lost request status output afterwards
Applied. But its best not to mess with the Changelog because there is no
guarantee this patch goes in before other work. As it turns out, it failed to
apply and then I deleted this chunk and everything was OK.
Ok, so I should have just added a Changelog entry to quote the previous
one, reverting it?
Post by Steve Grubb
-Steve
Post by Richard Guy Briggs
2.8.1
- Fix NULL ptr dereference in audispd plugin_dir parser
diff --git a/src/auditctl.c b/src/auditctl.c
index 0b301c1..a1c05b5 100644
--- a/src/auditctl.c
+++ b/src/auditctl.c
#endif
break;
- if ((rc = audit_reset_lost(fd)) >= 0)
- audit_request_status(fd);
- else {
+ if ((rc = audit_reset_lost(fd)) >= 0) {
+ audit_msg(LOG_INFO, "lost: %u", rc);
+ return -2;
+ } else {
audit_number_to_errmsg(rc, long_opts[lidx].name);
retval = -1;
}
- RGB

--
Richard Guy Briggs <***@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
2017-11-27 23:06:29 UTC
Permalink
Post by Richard Guy Briggs
Post by Steve Grubb
Post by Richard Guy Briggs
This reverts commit 56a708761347ba49ccdc2378d31133f01129f4f2.
ChangeLog
---
ChangeLog | 1 +
src/auditctl.c | 7 ++++---
2 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/ChangeLog b/ChangeLog
index de9ae56..4e9ca3d 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -4,6 +4,7 @@
- In auditctl, when resetting lost request status output afterwards
- AVC report from aureport was missing result column header (#1511606)
- Add SOFTWARE_UPDATE event
+- REVERT: In auditctl, when resetting lost request status output
afterwards>
Applied. But its best not to mess with the Changelog because there is no
guarantee this patch goes in before other work. As it turns out, it
failed to apply and then I deleted this chunk and everything was OK.
Ok, so I should have just added a Changelog entry to quote the previous
one, reverting it?
No. Just don't modify the changelog file. It can cause things not to apply
because other things went in first. I'll keep the Changelog file updated
separate to any patches submitted.

-Steve

Loading...