Re: [PATCH 1/7] audit: implement generic feature setting and retrieving

[Steve Grubb <sgrubb@redhat.com>]

Hello,

Just spent some time debugging auditctl, it was doing something I thought was 
weird. I tracked it down to this patch, see below for commentary...

On Friday, May 24, 2013 12:11:44 PM Eric Paris wrote:
> The audit_status structure was not designed with extensibility in mind.
> Define a new AUDIT_SET_FEATURE message type which takes a new structure
> of bits where things can be enabled/disabled/locked one at a time.  This
> structure should be able to grow in the future while maintaining forward
> and backward compatibility (based loosly on the ideas from capabilities
> and prctl)
> 
> This does not actually add any features, but is just infrastructure to
> allow new on/off types of audit system features.
> 
> Signed-off-by: Eric Paris <eparis@redhat.com>
> ---
>  include/linux/audit.h      |   2 +
>  include/uapi/linux/audit.h |  16 +++++++
>  kernel/audit.c             | 110
> ++++++++++++++++++++++++++++++++++++++++++++- 3 files changed, 127
> insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index 729a4d1..7b31bec 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -73,6 +73,8 @@ struct audit_field {
>  	void				*lsm_rule;
>  };
> 
> +extern int is_audit_feature_set(int which);
> +
>  extern int __init audit_register_class(int class, unsigned *list);
>  extern int audit_classify_syscall(int abi, unsigned syscall);
>  extern int audit_classify_arch(int arch);
> diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> index b7cb978..a053243 100644
> --- a/include/uapi/linux/audit.h
> +++ b/include/uapi/linux/audit.h
> @@ -68,6 +68,9 @@
>  #define AUDIT_MAKE_EQUIV	1015	/* Append to watched tree */
>  #define AUDIT_TTY_GET		1016	/* Get TTY auditing status */
>  #define AUDIT_TTY_SET		1017	/* Set TTY auditing status */
> +#define AUDIT_SET_FEATURE	1018	/* Turn an audit feature on or off */
> +#define AUDIT_GET_FEATURE	1019	/* Get which features are enabled */
> +#define AUDIT_FEATURE_CHANGE	1020	/* audit log listing feature changes 
*/
> 
>  #define AUDIT_FIRST_USER_MSG	1100	/* Userspace messages mostly
> uninteresting to kernel */ #define AUDIT_USER_AVC		1107	/* We 
filter this
> differently */
> @@ -369,6 +372,19 @@ struct audit_status {
>  	__u32		backlog;	/* messages waiting in queue */
>  };
> 
> +struct audit_features {
> +#define AUDIT_FEATURE_VERSION	1
> +	__u32	vers;
> +	__u32	mask;		/* which bits we are dealing with */
> +	__u32	features;	/* which feature to enable/disable */
> +	__u32	lock;		/* which features to lock */
> +};
> +
> +#define AUDIT_LAST_FEATURE	-1
> +
> +#define audit_feature_valid(x)		((x) >= 0 && (x) <= 
AUDIT_LAST_FEATURE)
> +#define AUDIT_FEATURE_TO_MASK(x)	(1 << ((x) & 31)) /* mask for __u32 */
> +
>  struct audit_tty_status {
>  	__u32		enabled;	/* 1 = enabled, 0 = disabled */
>  	__u32		log_passwd;	/* 1 = enabled, 0 = disabled */
> diff --git a/kernel/audit.c b/kernel/audit.c
> index f2f4666..3acbbc8 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -140,6 +140,15 @@ static struct task_struct *kauditd_task;
>  static DECLARE_WAIT_QUEUE_HEAD(kauditd_wait);
>  static DECLARE_WAIT_QUEUE_HEAD(audit_backlog_wait);
> 
> +static struct audit_features af = {.vers = AUDIT_FEATURE_VERSION,
> +				   .mask = -1,
> +				   .features = 0,
> +				   .lock = 0,};
> +
> +static char *audit_feature_names[0] = {
> +};
> +
> +
>  /* Serialize requests from userspace. */
>  DEFINE_MUTEX(audit_cmd_mutex);
> 
> @@ -584,6 +593,8 @@ static int audit_netlink_ok(struct sk_buff *skb, u16
> msg_type) return -EOPNOTSUPP;
>  	case AUDIT_GET:
>  	case AUDIT_SET:
> +	case AUDIT_GET_FEATURE:
> +	case AUDIT_SET_FEATURE:
>  	case AUDIT_LIST_RULES:
>  	case AUDIT_ADD_RULE:
>  	case AUDIT_DEL_RULE:
> @@ -628,6 +639,94 @@ static int audit_log_common_recv_msg(struct
> audit_buffer **ab, u16 msg_type) return rc;
>  }
> 
> +int is_audit_feature_set(int i)
> +{
> +	return af.features & AUDIT_FEATURE_TO_MASK(i);
> +}
> +
> +
> +static int audit_get_feature(struct sk_buff *skb)
> +{
> +	u32 seq;
> +
> +	seq = nlmsg_hdr(skb)->nlmsg_seq;
> +
> +	audit_send_reply(NETLINK_CB(skb).portid, seq, AUDIT_GET, 0, 0,
> +			 &af, sizeof(af));
> +
> +	return 0;
> +}

Isn't this broke? This returns the status (AUDIT_GET) instead of all the bits 
that got set via the set_feature command. It needs to build a struct 
audit_features and send it back using AUDIT_GET_FEATURE as the netlink msg 
type.

-Steve