[PATCH V4 (was V6)] audit: macros to replace unset inode and device values

[PATCH V4 (was V6)] audit: macros to replace unset inode and device values

[Richard Guy Briggs]

This is a patch to clean up a number of places were casted magic numbers are
used to represent unset inode and device numbers inpreparation for the audit by
executable path patch set.

Richard Guy Briggs (1):
  audit: use macros for unset inode and device values

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

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

[Richard Guy Briggs]

Signed-off-by: Richard Guy Briggs <rgb@redhat.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(-)

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);
 
-- 
1.7.1

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

[Paul Moore]

On Saturday, August 01, 2015 03:42:23 PM Richard Guy Briggs wrote:
> Signed-off-by: Richard Guy Briggs <rgb@redhat.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.

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

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

[Paul Moore]

On Saturday, August 01, 2015 03:42:23 PM Richard Guy Briggs wrote:
> Signed-off-by: Richard Guy Briggs <rgb@redhat.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.

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

Re: [PATCH V4 (was V6)] audit: macros to replace unset inode and device values

[Paul Moore]

On Saturday, August 01, 2015 03:42:22 PM Richard Guy Briggs wrote:
> This is a patch to clean up a number of places were casted magic numbers are
> used to represent unset inode and device numbers inpreparation for the
> audit by executable path patch set.
> 
> Richard Guy Briggs (1):
>   audit: use macros for unset inode and device values
> 
>  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(-)

Also, when there is only one patch in the patchset, no need to send a cover 
email, e.g. patch 0/1, just put the text in the patch description itself.

-- 
paul moore
security @ redhat

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

[Richard Guy Briggs]

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@redhat.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@redhat.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

Re: [PATCH V4 (was V6)] audit: macros to replace unset inode and device values

[Richard Guy Briggs]

On 15/08/04, Paul Moore wrote:
> On Saturday, August 01, 2015 03:42:22 PM Richard Guy Briggs wrote:
> > This is a patch to clean up a number of places were casted magic numbers are
> > used to represent unset inode and device numbers inpreparation for the
> > audit by executable path patch set.
> > 
> > Richard Guy Briggs (1):
> >   audit: use macros for unset inode and device values
> > 
> >  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(-)
> 
> Also, when there is only one patch in the patchset, no need to send a cover 
> email, e.g. patch 0/1, just put the text in the patch description itself.

Or drop it into a comment after the "---" demarcation...  Which would be
better for questions that turn out to need no further followup.

> paul moore

- RGB

--
Richard Guy Briggs <rbriggs@redhat.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

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

[Paul Moore]

On Wednesday, August 05, 2015 02:30:14 AM Richard Guy Briggs wrote:
> 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@redhat.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?

I have no problem with it in include/linux/audit.h, that is a kernel-only 
include that we can change at anytime.  My concern is putting it into a uapi 
header which makes it very hard to change.

I'm thinking we should just go ahead and put it in include/linux/audit.h for 
now as I can't think of a reason why userspace should be passing in an invalid 
dev/inode value, it just doesn't make sense.  If the invalid tokens prove to 
be valuable for userspace, we can always move the #defines.

-- 
paul moore
security @ redhat

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

[William Roberts]

[-- Attachment #1.1: Type: text/plain, Size: 4245 bytes --]

On Aug 1, 2015 12:44 PM, "Richard Guy Briggs" <rgb@redhat.com> wrote:
>
> Signed-off-by: Richard Guy Briggs <rgb@redhat.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(-)
>
> 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

Why unsigned int in one but unsigned in the other?

>
>  /* 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);
>
> --
> 1.7.1
>
> --
> Linux-audit mailing list
> Linux-audit@redhat.com
> https://www.redhat.com/mailman/listinfo/linux-audit

[-- Attachment #1.2: Type: text/html, Size: 5882 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

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

[Richard Guy Briggs]

On 15/08/05, William Roberts wrote:
> On Aug 1, 2015 12:44 PM, "Richard Guy Briggs" <rgb@redhat.com> wrote:
> >
> > Signed-off-by: Richard Guy Briggs <rgb@redhat.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(-)
> >
> > 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
> 
> Why unsigned int in one but unsigned in the other?

It was based on one of the instances it was originally replacing (I
can't find it now).  It could be dev_t if it was moved out of uapi as
Paul preferred, but I prefer unsigned int now that you point it out.

> >  /* 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);
> >
> > --
> > 1.7.1
> >
> > --
> > Linux-audit mailing list
> > Linux-audit@redhat.com
> > https://www.redhat.com/mailman/listinfo/linux-audit

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


- RGB

--
Richard Guy Briggs <rbriggs@redhat.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

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

[Steve Grubb]

On Wednesday, August 05, 2015 03:16:58 PM Paul Moore wrote:
> On Wednesday, August 05, 2015 02:30:14 AM Richard Guy Briggs wrote:
> > 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@redhat.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?
> 
> I have no problem with it in include/linux/audit.h, that is a kernel-only
> include that we can change at anytime.  My concern is putting it into a uapi
> header which makes it very hard to change.
> 
> I'm thinking we should just go ahead and put it in include/linux/audit.h for
> now as I can't think of a reason why userspace should be passing in an
> invalid dev/inode value, it just doesn't make sense.  If the invalid tokens
> prove to be valuable for userspace, we can always move the #defines.

I can't imagine anyone auditing against a specific device or inode. Its like 
auditing a pid when you really want the program name. Its much more useful to 
audit by filename or directory and not inode/device. So, do whatever you want. 
The only unset value that people actually use is the auid because deamons have 
it unset.

-Steve

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

[Paul Moore]

On Wed, Aug 5, 2015 at 3:38 PM, Richard Guy Briggs <rgb@redhat.com> wrote:
> On 15/08/05, William Roberts wrote:
>> On Aug 1, 2015 12:44 PM, "Richard Guy Briggs" <rgb@redhat.com> wrote:
>> >
>> > Signed-off-by: Richard Guy Briggs <rgb@redhat.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(-)
>> >
>> > 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
>>
>> Why unsigned int in one but unsigned in the other?
>
> It was based on one of the instances it was originally replacing (I
> can't find it now).  It could be dev_t if it was moved out of uapi as
> Paul preferred, but I prefer unsigned int now that you point it out.

Once we move it out of the uapi header it should probably be (dev_t)-1.


-- 
paul moore
www.paul-moore.com

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

[Casey Schaufler]

On 8/5/2015 1:08 PM, Steve Grubb wrote:
> On Wednesday, August 05, 2015 03:16:58 PM Paul Moore wrote:
>> On Wednesday, August 05, 2015 02:30:14 AM Richard Guy Briggs wrote:
>>> 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@redhat.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?
>> I have no problem with it in include/linux/audit.h, that is a kernel-only
>> include that we can change at anytime.  My concern is putting it into a uapi
>> header which makes it very hard to change.
>>
>> I'm thinking we should just go ahead and put it in include/linux/audit.h for
>> now as I can't think of a reason why userspace should be passing in an
>> invalid dev/inode value, it just doesn't make sense.  If the invalid tokens
>> prove to be valuable for userspace, we can always move the #defines.
> I can't imagine anyone auditing against a specific device or inode. Its like 
> auditing a pid when you really want the program name. Its much more useful to 
> audit by filename or directory and not inode/device. So, do whatever you want. 
> The only unset value that people actually use is the auid because deamons have 
> it unset.

I remember the Orange Book days when we were *required* to audit by dev/inode
because it was the only true way to identify the object. Yes, it's analogous to
auditing the pid, but we had to audit by that, too. The dev/indode and pid are
the "true" names. Anything else is a hint at what you're looking at. I can easily
imaging someone who really cares about the audit data supplying the dev/inode and
pid. 

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

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

[Paul Moore]

On Thursday, August 06, 2015 02:31:57 PM Casey Schaufler wrote:
> I remember the Orange Book days when we were *required* to audit by
> dev/inode because it was the only true way to identify the object. Yes,
> it's analogous to auditing the pid, but we had to audit by that, too. The
> dev/indode and pid are the "true" names. Anything else is a hint at what
> you're looking at. I can easily imaging someone who really cares about the
> audit data supplying the dev/inode and pid.

Just to add a bit of clarity, my original question was if there was any value 
in exposing the unset/invalid device and inode values, e.g. -1.  While I agree 
that there is value in auditing by dev/inode, I can't think of a reasonable 
situation where the user would need to pass an unset/invalid device and/or 
inode value into the kernel as part of an audit configuration command.

-- 
paul moore
security @ redhat