[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [PATCH] Audit filter rule operators (2/2)



Hi Dustin,

Just a few comments below...

On Fri, Oct 21, 2005 at 06:24:31PM -0500, Dustin Kirkland wrote:
> diff -urpbBN linux-2.6.14-rc4/include/linux/audit.h
> linux-2.6.14-rc4-audit_ops/include/linux/audit.h
> --- linux-2.6.14-rc4/include/linux/audit.h	2005-10-19 09:40:27.000000000 -0500
> +++ linux-2.6.14-rc4-audit_ops/include/linux/audit.h	2005-10-21 18:05:43.000000000 -0500
> @@ -128,8 +128,29 @@
>  #define AUDIT_ARG2      (AUDIT_ARG0+2)
>  #define AUDIT_ARG3      (AUDIT_ARG0+3)
>  
> -#define AUDIT_NEGATE    0x80000000
> +/* These are the supported operators.
> +        4  2  1
> +        >  <  =
> +        -------
> +        0  0  0         0       undef
> +        0  0  1         1       =
> +        0  1  0         2       <
> +        0  1  1         3       <=
> +        1  0  0         4       >
> +        1  0  1         5       >=
> +        1  1  0         6       !=
> +        1  1  1         7       range
> + */
> +#define AUDIT_OPERATORS			0xF0000000
> +#define AUDIT_EQUAL			0x10000000
> +#define AUDIT_LESS_THAN			0x20000000
> +#define AUDIT_LESS_THAN_OR_EQUAL	0x30000000

Here's another way to specify this that may be more readable and
common in the kernel:

#define AUDIT_LESS_THAN_OR_EQUAL        AUDIT_EQUAL|AUDIT_LESS_THAN

> +#define AUDIT_GREATER_THAN		0x40000000
> +#define AUDIT_GREATER_THAN_OR_EQUAL	0x50000000
> +#define AUDIT_NOT_EQUAL			0x60000000
> +#define AUDIT_RANGE			0x70000000

AUDIT_RANGE isn't needed at all, as you already have an implementation
that can express that.

audit_filter_rules() treats the fields as implicit ANDs, so the
following would express a uid range 10..20

field[0] = AUDIT_UID;
value[0] = 10 | AUDIT_GREATER_THAN_OR_EQUAL;

field[1] = AUDIT_UID;
value[1] = 20 | AUDIT_LESS_THAN_OR_EQUAL;

> +#define AUDIT_NEGATE			0x80000000
>  
>  /* Status symbols */
>  				/* Mask values */
> diff -urpbBN linux-2.6.14-rc4/kernel/auditsc.c linux-2.6.14-rc4-audit_ops/kernel/auditsc.c
> --- linux-2.6.14-rc4/kernel/auditsc.c	2005-10-19 09:40:29.000000000 -0500
> +++ linux-2.6.14-rc4-audit_ops/kernel/auditsc.c	2005-10-21 18:08:32.000000000 -0500
> @@ -385,6 +385,36 @@ int audit_receive_filter(int type, int p
>  	return err;
>  }
>  
> +static int audit_comparator(const u32 left, const u32 operator, const u32 right)
> +{
> +	int rc;
> +	switch (operator) {
> +		case AUDIT_LESS_THAN:
> +			rc = (left < right);
> +			break;
> +		case AUDIT_LESS_THAN_OR_EQUAL:
> +			rc = (left <= right);
> +			break;
> +		case AUDIT_GREATER_THAN:
> +			rc = (left > right);
> +			break;
> +		case AUDIT_GREATER_THAN_OR_EQUAL:
> +			rc = (left >= right);
> +			break;
> +		case AUDIT_NOT_EQUAL:
> +			rc = (left != right);
> +			break;
> +		case AUDIT_EQUAL:
> +		default:
> +			rc = (left == right);
> +			break;
> +	}

Or how about this?

        int rc = 0;

        if ((operator & AUDIT_EQUAL) && (left == right))
                rc = 1;
        else if ((operator & AUDIT_LESS_THAN) && (left < right))
                rc = 1;
        else if ((operator & AUDIT_GREATER_THAN) && (left > right))
                rc = 1;

        return rc;

> +	if ( operator & AUDIT_NEGATE )
> +		return !rc;
> +	else 
> +		return rc;

I think this may have been alluded to in another email, but we need to
do comaptibility checking on rule insert, not during rule evaluation.
So if AUDIT_NEGATE is set, clear it and set AUDIT_NOT_EQUAL.  If no
bits are set, set AUDIT_EQUAL.  Doing so eliminates these two branches
in fast-path code.

> +}
> +
>  /* Compare a task_struct with an audit_rule.  Return 1 on match, 0
>   * otherwise. */
>  static int audit_filter_rules(struct task_struct *tsk,
> @@ -395,62 +425,63 @@ static int audit_filter_rules(struct tas
>  	int i, j;
>  
>  	for (i = 0; i < rule->field_count; i++) {
> -		u32 field  = rule->fields[i] & ~AUDIT_NEGATE;
> +		u32 field  = rule->fields[i] & ~AUDIT_OPERATORS;
> +		u32 op  = rule->fields[i] & AUDIT_OPERATORS;
>  		u32 value  = rule->values[i];
>  		int result = 0;
>  
>  		switch (field) {
>  		case AUDIT_PID:
> -			result = (tsk->pid == value);
> +				result = audit_comparator(tsk->pid, op, value);
>  			break;
>  		case AUDIT_UID:
> -			result = (tsk->uid == value);
> +				result = audit_comparator(tsk->uid, op, value);
>  			break;
>  		case AUDIT_EUID:
> -			result = (tsk->euid == value);
> +				result = audit_comparator(tsk->euid, op, value);
>  			break;
>  		case AUDIT_SUID:
> -			result = (tsk->suid == value);
> +				result = audit_comparator(tsk->suid, op, value);
>  			break;
>  		case AUDIT_FSUID:
> -			result = (tsk->fsuid == value);
> +				result = audit_comparator(tsk->fsuid, op, value);
>  			break;
>  		case AUDIT_GID:
> -			result = (tsk->gid == value);
> +				result = audit_comparator(tsk->gid, op, value);
>  			break;
>  		case AUDIT_EGID:
> -			result = (tsk->egid == value);
> +				result = audit_comparator(tsk->egid, op, value);
>  			break;
>  		case AUDIT_SGID:
> -			result = (tsk->sgid == value);
> +				result = audit_comparator(tsk->sgid, op, value);
>  			break;
>  		case AUDIT_FSGID:
> -			result = (tsk->fsgid == value);
> +				result = audit_comparator(tsk->fsgid, op, value);
>  			break;
>  		case AUDIT_PERS:
> -			result = (tsk->personality == value);
> +				result = audit_comparator(tsk->personality, op, value);

Many lines in this function are way over 80 chars -- see
Documentation/CodingStyle Chapter 2.

>  			break;
>  		case AUDIT_ARCH:
>  			if (ctx) 
> -				result = (ctx->arch == value);
> +					result = audit_comparator(ctx->arch, op, value);
>  			break;
>  
>  		case AUDIT_EXIT:
>  			if (ctx && ctx->return_valid)
> -				result = (ctx->return_code == value);
> +					result = audit_comparator(ctx->return_code, op, value);
>  			break;
>  		case AUDIT_SUCCESS:
>  			if (ctx && ctx->return_valid) {
>  				if (value)
> -					result = (ctx->return_valid == AUDITSC_SUCCESS);
> +						result = audit_comparator(ctx->return_valid, op, AUDITSC_SUCCESS);
>  				else
> -					result = (ctx->return_valid == AUDITSC_FAILURE);
> +						result = audit_comparator(ctx->return_valid, op, AUDITSC_FAILURE);
>  			}
>  			break;
>  		case AUDIT_DEVMAJOR:
>  			if (ctx) {
>  				for (j = 0; j < ctx->name_count; j++) {
> -					if (MAJOR(ctx->names[j].dev)==value) {
> +						if ( audit_comparator(MAJOR(ctx->names[j].dev), op, value) ) {
>  						++result;
>  						break;
>  					}
> @@ -460,7 +491,7 @@ static int audit_filter_rules(struct tas
>  		case AUDIT_DEVMINOR:
>  			if (ctx) {
>  				for (j = 0; j < ctx->name_count; j++) {
> -					if (MINOR(ctx->names[j].dev)==value) {
> +						if ( audit_comparator(MINOR(ctx->names[j].dev), op, value) ) {
>  						++result;
>  						break;
>  					}
> @@ -470,7 +501,7 @@ static int audit_filter_rules(struct tas
>  		case AUDIT_INODE:
>  			if (ctx) {
>  				for (j = 0; j < ctx->name_count; j++) {
> -					if (ctx->names[j].ino == value) {
> +						if ( audit_comparator(ctx->names[j].ino, op, value) ) {
>  						++result;
>  						break;
>  					}
> @@ -480,19 +511,17 @@ static int audit_filter_rules(struct tas
>  		case AUDIT_LOGINUID:
>  			result = 0;
>  			if (ctx)
> -				result = (ctx->loginuid == value);
> +					result = audit_comparator(ctx->loginuid, op, value);
>  			break;
>  		case AUDIT_ARG0:
>  		case AUDIT_ARG1:
>  		case AUDIT_ARG2:
>  		case AUDIT_ARG3:
>  			if (ctx)
> -				result = (ctx->argv[field-AUDIT_ARG0]==value);
> +					result = audit_comparator(ctx->argv[field-AUDIT_ARG0], op, value);
>  			break;
>  		}
>  
> -		if (rule->fields[i] & AUDIT_NEGATE)
> -			result = !result;
>  		if (!result)
>  			return 0;
>  	}
> 
> 



> --
> Linux-audit mailing list
> Linux-audit redhat com
> https://www.redhat.com/mailman/listinfo/linux-audit


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]