linux-kernel - Re: [PATCH V4 (was V6)] audit: use macros for unset inode and device values

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20150805063014.GB32407@madcap2.tricolour.ca>
Date:	Wed, 5 Aug 2015 02:30:14 -0400
From:	Richard Guy Briggs <rgb@...hat.com>
To:	Paul Moore <pmoore@...hat.com>
Cc:	linux-audit@...hat.com, linux-kernel@...r.kernel.org,
	sgrubb@...hat.com, eparis@...hat.com
Subject: Re: [PATCH V4 (was V6)] audit: use macros for unset inode and device
 values

On 15/08/04, Paul Moore wrote:
> On Saturday, August 01, 2015 03:42:23 PM Richard Guy Briggs wrote:
> > Signed-off-by: Richard Guy Briggs <rgb@...hat.com>
> > ---
> >  include/uapi/linux/audit.h |    2 ++
> >  kernel/audit.c             |    2 +-
> >  kernel/audit_watch.c       |    8 ++++----
> >  kernel/auditsc.c           |    6 +++---
> >  4 files changed, 10 insertions(+), 8 deletions(-)
> 
> Yipee, less magic numbers!
> 
> However, one question for you ... are we ever going to see a device or inode 
> set to -1 in the userspace facing API?  In other words, should the new 
> #defines go in the uapi headers or simply in kernel/audit.h?  Unless it is 
> part of the API, let's leave it out of uapi as we have to be very careful 
> about that stuff and I'd prefer to keep it minimal.

This is a good point.  I did briefly thing about this at one point.
Perhaps Steve can answer this.  It would be trivial to move it back to
uapi if needed.  Would you be ok with it in include/linux/audit.h for
now?

> Also, if we can put the #defines in kernel/audit.h we can use the proper type 
> for AUDIT_DEV_UNSET which would make me happy.
> 
> > diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> > index d3475e1..971df22 100644
> > --- a/include/uapi/linux/audit.h
> > +++ b/include/uapi/linux/audit.h
> > @@ -440,6 +440,8 @@ struct audit_tty_status {
> >  };
> > 
> >  #define AUDIT_UID_UNSET (unsigned int)-1
> > +#define AUDIT_INO_UNSET (unsigned long)-1
> > +#define AUDIT_DEV_UNSET (unsigned)-1
> > 
> >  /* audit_rule_data supports filter rules with both integer and string
> >   * fields.  It corresponds with AUDIT_ADD_RULE, AUDIT_DEL_RULE and
> > diff --git a/kernel/audit.c b/kernel/audit.c
> > index 1c13e42..d546003 100644
> > --- a/kernel/audit.c
> > +++ b/kernel/audit.c
> > @@ -1761,7 +1761,7 @@ void audit_log_name(struct audit_context *context,
> > struct audit_names *n, } else
> >  		audit_log_format(ab, " name=(null)");
> > 
> > -	if (n->ino != (unsigned long)-1)
> > +	if (n->ino != AUDIT_INO_UNSET)
> >  		audit_log_format(ab, " inode=%lu"
> >  				 " dev=%02x:%02x mode=%#ho"
> >  				 " ouid=%u ogid=%u rdev=%02x:%02x",
> > diff --git a/kernel/audit_watch.c b/kernel/audit_watch.c
> > index 8f123d7..c668bfc 100644
> > --- a/kernel/audit_watch.c
> > +++ b/kernel/audit_watch.c
> > @@ -138,7 +138,7 @@ char *audit_watch_path(struct audit_watch *watch)
> > 
> >  int audit_watch_compare(struct audit_watch *watch, unsigned long ino, dev_t
> > dev) {
> > -	return (watch->ino != (unsigned long)-1) &&
> > +	return (watch->ino != AUDIT_INO_UNSET) &&
> >  		(watch->ino == ino) &&
> >  		(watch->dev == dev);
> >  }
> > @@ -179,8 +179,8 @@ static struct audit_watch *audit_init_watch(char *path)
> >  	INIT_LIST_HEAD(&watch->rules);
> >  	atomic_set(&watch->count, 1);
> >  	watch->path = path;
> > -	watch->dev = (dev_t)-1;
> > -	watch->ino = (unsigned long)-1;
> > +	watch->dev = AUDIT_DEV_UNSET;
> > +	watch->ino = AUDIT_INO_UNSET;
> > 
> >  	return watch;
> >  }
> > @@ -493,7 +493,7 @@ static int audit_watch_handle_event(struct
> > fsnotify_group *group, if (mask & (FS_CREATE|FS_MOVED_TO) && inode)
> >  		audit_update_watch(parent, dname, inode->i_sb->s_dev, inode->i_ino, 0);
> >  	else if (mask & (FS_DELETE|FS_MOVED_FROM))
> > -		audit_update_watch(parent, dname, (dev_t)-1, (unsigned long)-1, 1);
> > +		audit_update_watch(parent, dname, AUDIT_DEV_UNSET, AUDIT_INO_UNSET, 1);
> >  	else if (mask & (FS_DELETE_SELF|FS_UNMOUNT|FS_MOVE_SELF))
> >  		audit_remove_parent_watches(parent);
> > 
> > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > index 9fb9d1c..701ea5c 100644
> > --- a/kernel/auditsc.c
> > +++ b/kernel/auditsc.c
> > @@ -180,7 +180,7 @@ static int audit_match_filetype(struct audit_context
> > *ctx, int val) return 0;
> > 
> >  	list_for_each_entry(n, &ctx->names_list, list) {
> > -		if ((n->ino != -1) &&
> > +		if ((n->ino != AUDIT_INO_UNSET) &&
> >  		    ((n->mode & S_IFMT) == mode))
> >  			return 1;
> >  	}
> > @@ -1683,7 +1683,7 @@ static struct audit_names *audit_alloc_name(struct
> > audit_context *context, aname->should_free = true;
> >  	}
> > 
> > -	aname->ino = (unsigned long)-1;
> > +	aname->ino = AUDIT_INO_UNSET;
> >  	aname->type = type;
> >  	list_add_tail(&aname->list, &context->names_list);
> > 
> > @@ -1925,7 +1925,7 @@ void __audit_inode_child(const struct inode *parent,
> >  	if (inode)
> >  		audit_copy_inode(found_child, dentry, inode);
> >  	else
> > -		found_child->ino = (unsigned long)-1;
> > +		found_child->ino = AUDIT_INO_UNSET;
> >  }
> >  EXPORT_SYMBOL_GPL(__audit_inode_child);
> 
> -- 
> paul moore
> security @ redhat
> 

- RGB

--
Richard Guy Briggs <rbriggs@...hat.com>
Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/