[PATCH 0/2] lost_reset: alt fix restoring designed behaviour

[PATCH 0/2] lost_reset: alt fix restoring designed behaviour

[Richard Guy Briggs]

Provide an alternate fix that restores the designed synchronous
behaviour.

Richard Guy Briggs (2):
  Revert "In auditctl, when resetting lost request status output
    afterwards"
  lost_reset: return value rather than sequence number when zero

 ChangeLog      |  1 +
 lib/libaudit.c |  3 ++-
 lib/netlink.c  | 28 ++++++++++++++++++++--------
 lib/private.h  |  1 +
 src/auditctl.c |  7 ++++---
 5 files changed, 28 insertions(+), 12 deletions(-)

-- 
1.8.3.1

[PATCH 1/2] Revert "In auditctl, when resetting lost request status output afterwards"

[Richard Guy Briggs]

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

[PATCH 2/2] lost_reset: return value rather than sequence number when zero

[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().

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 <rgb@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

Re: [PATCH 1/2] Revert "In auditctl, when resetting lost request status output afterwards"

[Steve Grubb]

On Wednesday, November 22, 2017 7:00:56 PM EST Richard Guy Briggs wrote:
> 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

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


>  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;
>  		}

Re: [PATCH 2/2] lost_reset: return value rather than sequence number when zero

[Steve Grubb]

On Wednesday, November 22, 2017 7:00:57 PM EST Richard Guy Briggs wrote:
> 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

Re: [PATCH 1/2] Revert "In auditctl, when resetting lost request status output afterwards"

[Richard Guy Briggs]

On 2017-11-27 17:16, Steve Grubb wrote:
> On Wednesday, November 22, 2017 7:00:56 PM EST Richard Guy Briggs wrote:
> > 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
> 
> 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?

> -Steve
> 
> >  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;
> >  		}

- 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

Re: [PATCH 1/2] Revert "In auditctl, when resetting lost request status output afterwards"

[Steve Grubb]

On Monday, November 27, 2017 5:41:32 PM EST Richard Guy Briggs wrote:
> On 2017-11-27 17:16, Steve Grubb wrote:
> > On Wednesday, November 22, 2017 7:00:56 PM EST Richard Guy Briggs wrote:
> > > 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> 
> > 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