[PATCH v3] audit: log AUDIT_TIME_* records only from rules

[Richard Guy Briggs]

On 2022-01-25 22:24, Richard Guy Briggs wrote:
> AUDIT_TIME_* events are generated when there are syscall rules present that are
> not related to time keeping.  This will produce noisy log entries that could
> flood the logs and hide events we really care about.
> 
> Rather than immediately produce the AUDIT_TIME_* records, store the data in the
> context and log it at syscall exit time respecting the filter rules.
> 
> Please see https://bugzilla.redhat.com/show_bug.cgi?id=1991919
> 
> Fixes: 7e8eda734d30 ("ntp: Audit NTP parameters adjustment")
> Fixes: 2d87a0674bd6 ("timekeeping: Audit clock adjustments")
> Signed-off-by: Richard Guy Briggs <rgb at redhat.com>
> ---
> Changelog:
> v2:
> - rename __audit_ntp_log_ to audit_log_ntp
> - pre-check ntp before storing
> - move tk out of the context union and move ntp logging to the bottom of audit_show_special()
> - restructure logging of ntp to use ab and allocate more only if more
> - add Fixes lines
> v3
> - move tk into union
> - rename audit_log_ntp() to audit_log_time() and add tk
> - key off both AUDIT_TIME_* but favour NTP
> 
>  kernel/audit.h   |  4 +++
>  kernel/auditsc.c | 86 +++++++++++++++++++++++++++++++++++++-----------
>  2 files changed, 70 insertions(+), 20 deletions(-)
> 
> diff --git a/kernel/audit.h b/kernel/audit.h
> index c4498090a5bd..58b66543b4d5 100644
> --- a/kernel/audit.h
> +++ b/kernel/audit.h
> @@ -201,6 +201,10 @@ struct audit_context {
>  		struct {
>  			char			*name;
>  		} module;
> +		struct {
> +			struct audit_ntp_data	ntp_data;
> +			struct timespec64	tk_injoffset;
> +		} time;
>  	};
>  	int fds[2];
>  	struct audit_proctitle proctitle;
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index b517947bfa48..9c6c55a81fdb 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -1331,6 +1331,55 @@ static void audit_log_fcaps(struct audit_buffer *ab, struct audit_names *name)
>  			 from_kuid(&init_user_ns, name->fcap.rootid));
>  }
>  
> +void audit_log_time(struct audit_context *context, struct audit_buffer **ab)
> +{
> +	const struct audit_ntp_data *ntp = &context->time.ntp_data;
> +	const struct timespec64 *tk = &context->time.tk_injoffset;
> +	const char *ntp_name[] = {
> +		"offset",
> +		"freq",
> +		"status",
> +		"tai",
> +		"tick",
> +		"adjust",
> +	};
> +	int type, first = 1;
> +
> +	if (context->type == AUDIT_TIME_INJOFFSET)
> +		goto tk;
> +
> +	/* use up allocated ab from show_special before new one */
> +	for (type = 0; type < AUDIT_NTP_NVALS; type++) {
> +		if (ntp->vals[type].newval != ntp->vals[type].oldval) {
> +			if (first) {
> +				first = 0;
> +			} else {
> +				audit_log_end(*ab);
> +				*ab = audit_log_start(context, GFP_KERNEL,
> +						      AUDIT_TIME_ADJNTPVAL);
> +				if (!*ab)
> +					return;
> +			}
> +			audit_log_format(*ab, "op=%s old=%lli new=%lli",
> +					 ntp_name[type], ntp->vals[type].oldval,
> +					 ntp->vals[type].newval);
> +		}
> +	}
> +
> +tk:
> +	if (tk->tv_sec != 0 || tk->tv_nsec != 0) {
> +		if (!first) {
> +			audit_log_end(*ab);
> +			*ab = audit_log_start(context, GFP_KERNEL,
> +					      AUDIT_TIME_INJOFFSET);
> +			if (!*ab)
> +				return;
> +		}

It occurs to me that a slight improvement could be made to move the
"tk:" label here.  And better yet, change the tk condition above to:

	if (!tk->tv_sec && !tk->tv_nsec)
		return;

> +		audit_log_format(*ab, "sec=%lli nsec=%li",
> +				 (long long)tk->tv_sec, tk->tv_nsec);
> +	}
> +}
> +
>  static void show_special(struct audit_context *context, int *call_panic)
>  {
>  	struct audit_buffer *ab;
> @@ -1445,6 +1494,10 @@ static void show_special(struct audit_context *context, int *call_panic)
>  			audit_log_format(ab, "(null)");
>  
>  		break;
> +	case AUDIT_TIME_ADJNTPVAL:
> +	case AUDIT_TIME_INJOFFSET:
> +		audit_log_time(context, &ab);
> +		break;
>  	}
>  	audit_log_end(ab);
>  }
> @@ -2840,31 +2893,24 @@ void __audit_fanotify(unsigned int response)
>  
>  void __audit_tk_injoffset(struct timespec64 offset)
>  {
> -	audit_log(audit_context(), GFP_KERNEL, AUDIT_TIME_INJOFFSET,
> -		  "sec=%lli nsec=%li",
> -		  (long long)offset.tv_sec, offset.tv_nsec);
> -}
> -
> -static void audit_log_ntp_val(const struct audit_ntp_data *ad,
> -			      const char *op, enum audit_ntp_type type)
> -{
> -	const struct audit_ntp_val *val = &ad->vals[type];
> -
> -	if (val->newval == val->oldval)
> -		return;
> +	struct audit_context *context = audit_context();
>  
> -	audit_log(audit_context(), GFP_KERNEL, AUDIT_TIME_ADJNTPVAL,
> -		  "op=%s old=%lli new=%lli", op, val->oldval, val->newval);
> +	if (!context->type)
> +		context->type = AUDIT_TIME_INJOFFSET;
> +	memcpy(&context->time.tk_injoffset, &offset, sizeof(offset));
>  }
>  
>  void __audit_ntp_log(const struct audit_ntp_data *ad)
>  {
> -	audit_log_ntp_val(ad, "offset",	AUDIT_NTP_OFFSET);
> -	audit_log_ntp_val(ad, "freq",	AUDIT_NTP_FREQ);
> -	audit_log_ntp_val(ad, "status",	AUDIT_NTP_STATUS);
> -	audit_log_ntp_val(ad, "tai",	AUDIT_NTP_TAI);
> -	audit_log_ntp_val(ad, "tick",	AUDIT_NTP_TICK);
> -	audit_log_ntp_val(ad, "adjust",	AUDIT_NTP_ADJUST);
> +	struct audit_context *context = audit_context();
> +	int type;
> +
> +	for (type = 0; type < AUDIT_NTP_NVALS; type++)
> +		if (ad->vals[type].newval != ad->vals[type].oldval) {
> +			context->type = AUDIT_TIME_ADJNTPVAL;
> +			memcpy(&context->time.ntp_data, ad, sizeof(*ad));
> +			break;
> +		}
>  }
>  
>  void __audit_log_nfcfg(const char *name, u8 af, unsigned int nentries,
> -- 
> 2.27.0
> 

- RGB

--
Richard Guy Briggs <rgb at 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