Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1200304 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH V4 (was V6)] audit: use macros for unset inode and device values Paul Moore <pmoore@redhat.com> - 2015-08-05 00:40 +0200
    Re: [PATCH V4 (was V6)] audit: use macros for unset inode and device  values Richard Guy Briggs <rgb@redhat.com> - 2015-08-05 08:40 +0200
      Re: [PATCH V4 (was V6)] audit: use macros for unset inode and device values Paul Moore <pmoore@redhat.com> - 2015-08-05 21:20 +0200
        Re: [PATCH V4 (was V6)] audit: use macros for unset inode and device values Steve Grubb <sgrubb@redhat.com> - 2015-08-05 22:10 +0200
          Re: [PATCH V4 (was V6)] audit: use macros for unset inode and device  values Casey Schaufler <casey@schaufler-ca.com> - 2015-08-06 23:40 +0200
            Re: [PATCH V4 (was V6)] audit: use macros for unset inode and device values Paul Moore <pmoore@redhat.com> - 2015-08-07 16:30 +0200

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

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1200445 — 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@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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1201069

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1201090

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

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
>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1202735

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web