[PATCH] audit: convert status version to a feature bitmap

[PATCH] audit: convert status version to a feature bitmap

[Richard Guy Briggs]

The version field defined in the audit status structure was found to have
limitations in terms of its expressibility of features supported.  This is
distict from the get/set features call to be able to command those features
that are present.

Converting this field from a version number to a feature bitmap will allow
distributions to selectively backport and support certain features and will
allow upstream to be able to deprecate features in the future.  It will allow
userspace clients to first query the kernel for which features are actually
present and supported.  Currently, EINVAL is returned rather than EOPNOTSUP,
which isn't helpful in determining if there was an error in the command, or if
it simply isn't supported yet.  Past features are not represented by this
bitmap, but their use may be converted to EOPNOTSUP if needed in the future.

Since "version" is too generic to convert with a #define, use a union in the
struct status, introducing the member "feature_bitmap" unionized with
"version".

Convert existing AUDIT_VERSION_* macros over to AUDIT_FEATURE_BITMAP*
counterparts, leaving the former for backwards compatibility.

Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
 include/uapi/linux/audit.h |   17 +++++++++++++----
 kernel/audit.c             |    2 +-
 2 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
index 4d100c8..c9ac690 100644
--- a/include/uapi/linux/audit.h
+++ b/include/uapi/linux/audit.h
@@ -322,9 +322,15 @@ enum {
 #define AUDIT_STATUS_BACKLOG_LIMIT	0x0010
 #define AUDIT_STATUS_BACKLOG_WAIT_TIME	0x0020
 
-#define AUDIT_VERSION_BACKLOG_LIMIT	1
-#define AUDIT_VERSION_BACKLOG_WAIT_TIME	2
-#define AUDIT_VERSION_LATEST AUDIT_VERSION_BACKLOG_WAIT_TIME
+#define AUDIT_FEATURE_BITMAP_BACKLOG_LIMIT	0x00000001
+#define AUDIT_FEATURE_BITMAP_BACKLOG_WAIT_TIME	0x00000002
+#define AUDIT_FEATURE_BITMAP_ALL (AUDIT_FEATURE_BITMAP_BACKLOG_LIMIT     | \
+				  AUDIT_FEATURE_BITMAP_BACKLOG_WAIT_TIME   )
+
+/* deprecated: AUDIT_VERSION_* */
+#define AUDIT_VERSION_LATEST 		AUDIT_FEATURE_BITMAP_ALL
+#define AUDIT_VERSION_BACKLOG_LIMIT	AUDIT_FEATURE_BITMAP_BACKLOG_LIMIT
+#define AUDIT_VERSION_BACKLOG_WAIT_TIME	AUDIT_FEATURE_BITMAP_BACKLOG_WAIT_TIME
 
 				/* Failure-to-log actions */
 #define AUDIT_FAIL_SILENT	0
@@ -403,7 +409,10 @@ struct audit_status {
 	__u32		backlog_limit;	/* waiting messages limit */
 	__u32		lost;		/* messages lost */
 	__u32		backlog;	/* messages waiting in queue */
-	__u32		version;	/* audit api version number */
+	union {
+		__u32	version;	/* deprecated: audit api version num */
+		__u32	feature_bitmap;	/* bitmap of kernel audit features */
+	};
 	__u32		backlog_wait_time;/* message queue wait timeout */
 };
 
diff --git a/kernel/audit.c b/kernel/audit.c
index 8ee4508..f3a981d 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -842,7 +842,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
 		s.backlog_limit		= audit_backlog_limit;
 		s.lost			= atomic_read(&audit_lost);
 		s.backlog		= skb_queue_len(&audit_skb_queue);
-		s.version		= AUDIT_VERSION_LATEST;
+		s.feature_bitmap	= AUDIT_FEATURE_BITMAP_ALL;
 		s.backlog_wait_time	= audit_backlog_wait_time;
 		audit_send_reply(skb, seq, AUDIT_GET, 0, 0, &s, sizeof(s));
 		break;
-- 
1.7.1

Re: [PATCH] audit: convert status version to a feature bitmap

[Paul Moore]

On Monday, November 17, 2014 03:51:01 PM Richard Guy Briggs wrote:
> The version field defined in the audit status structure was found to have
> limitations in terms of its expressibility of features supported.  This is
> distict from the get/set features call to be able to command those features
> that are present.
> 
> Converting this field from a version number to a feature bitmap will allow
> distributions to selectively backport and support certain features and will
> allow upstream to be able to deprecate features in the future.  It will
> allow userspace clients to first query the kernel for which features are
> actually present and supported.  Currently, EINVAL is returned rather than
> EOPNOTSUP, which isn't helpful in determining if there was an error in the
> command, or if it simply isn't supported yet.  Past features are not
> represented by this bitmap, but their use may be converted to EOPNOTSUP if
> needed in the future.
> 
> Since "version" is too generic to convert with a #define, use a union in the
> struct status, introducing the member "feature_bitmap" unionized with
> "version".
> 
> Convert existing AUDIT_VERSION_* macros over to AUDIT_FEATURE_BITMAP*
> counterparts, leaving the former for backwards compatibility.
> 
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
>  include/uapi/linux/audit.h |   17 +++++++++++++----
>  kernel/audit.c             |    2 +-
>  2 files changed, 14 insertions(+), 5 deletions(-)

Applied to the audit next branch, thanks Richard.

 * git://git.infradead.org/users/pcmoore/audit next

-- 
paul moore
security and virtualization @ redhat

Re: [PATCH] audit: convert status version to a feature bitmap

[Paul Moore]

On Friday, November 14, 2014 10:32:51 PM Richard Guy Briggs wrote:
> On 14/11/13, Steve Grubb wrote:
> > On Thursday, November 13, 2014 08:08:52 PM Richard Guy Briggs wrote:
> > > > So what terrible things happen to userspace if
> > > > AUDIT_VERSION_BACKLOG_WAIT_TIME  becomes 0x03 instead of 0x02?
> > > 
> > > But it won't.  It gets the value of
> > > AUDIT_FEATURE_BITMAP_BACKLOG_WAIT_TIME, which is 0x00000002.
> > > 
> > > I think you meant to ask about AUDIT_VERSION_LATEST, which would become
> > > 3.
> > > 
> > > You *did* already ask that question in a previous thread, and there
> > > didn't seem to be a concern.  Steve Grubb could likely answer this
> > > question better than me.
> > 
> > The audit 2.4.1 package has been pushed to everything from F20 -> rawhide.
> > If you don't see any problems, then its safe. But check carefully around
> > the things that you did change. Right now, we only are caring about only
> > one kernel feature, --loginuid-immutable. Check that it still works,
> > auditctl -s.
> Here's my output, which I assume looks sane:
> 
> [root@f20 ~]# rpm -q audit
> audit-2.4.1-1.fc20.x86_64
> [root@f20 ~]# auditctl -s
> enabled 1
> flag 1
> pid 307
> rate_limit 0
> backlog_limit 320
> lost 0
> backlog 0
> backlog_wait_time 60000
> loginuid_immutable 0 unlocked

Looks like good output to me, Steve?

-- 
paul moore
security and virtualization @ redhat

Re: [PATCH] audit: convert status version to a feature bitmap

[Richard Guy Briggs]

On 14/11/13, Steve Grubb wrote:
> On Thursday, November 13, 2014 08:08:52 PM Richard Guy Briggs wrote:
> > > So what terrible things happen to userspace if
> > > AUDIT_VERSION_BACKLOG_WAIT_TIME  becomes 0x03 instead of 0x02?
> > 
> > But it won't.  It gets the value of
> > AUDIT_FEATURE_BITMAP_BACKLOG_WAIT_TIME, which is 0x00000002.
> > 
> > I think you meant to ask about AUDIT_VERSION_LATEST, which would become 3.
> > 
> > You *did* already ask that question in a previous thread, and there
> > didn't seem to be a concern.  Steve Grubb could likely answer this
> > question better than me.
> 
> The audit 2.4.1 package has been pushed to everything from F20 -> rawhide. If 
> you don't see any problems, then its safe. But check carefully around the 
> things that you did change. Right now, we only are caring about only one 
> kernel feature, --loginuid-immutable. Check that it still works, auditctl -s.

Here's my output, which I assume looks sane:

[root@f20 ~]# rpm -q audit
audit-2.4.1-1.fc20.x86_64
[root@f20 ~]# auditctl -s
enabled 1
flag 1
pid 307
rate_limit 0
backlog_limit 320
lost 0
backlog 0
backlog_wait_time 60000
loginuid_immutable 0 unlocked

> -Steve

- 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] audit: convert status version to a feature bitmap

[Paul Moore]

My apologies, yes I was concerned about LATEST. Try the test that Steve 
described and if that works I'm happy.
--
paul moore
www.paul-moore.com



On November 13, 2014 8:09:11 PM Richard Guy Briggs <rgb@redhat.com> wrote:

> On 14/11/13, Paul Moore wrote:
> > On Thursday, November 13, 2014 03:29:10 PM Richard Guy Briggs wrote:
> > > The version field defined in the audit status structure was found to have
> > > limitations in terms of its expressibility of features supported.  This is
> > > distict from the get/set features call to be able to command those features
> > > that are present.
> > >
> > > Converting this field from a version number to a feature bitmap will allow
> > > distributions to selectively backport and support certain features and will
> > > allow upstream to be able to deprecate features in the future.  It will
> > > allow userspace clients to first query the kernel for which features are
> > > actually present and supported.  Currently, EINVAL is returned rather than
> > > EOPNOTSUP, which isn't helpful in determining if there was an error in the
> > > command, or if it simply isn't supported yet.  Past features are not
> > > represented by this bitmap, but their use may be converted to EOPNOTSUP if
> > > needed in the future.
> > >
> > > Since "version" is too generic to convert with a #define, use a union 
> in the
> > > struct status, introducing the member "feature_bitmap" unionized with
> > > "version".
> > >
> > > Convert existing AUDIT_VERSION_* macros over to AUDIT_FEATURE_BITMAP*
> > > counterparts, leaving the former for backwards compatibility.
> > >
> > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > > ---
> > >  include/uapi/linux/audit.h |   17 +++++++++++++----
> > >  kernel/audit.c             |    2 +-
> > >  2 files changed, 14 insertions(+), 5 deletions(-)
> >
> > Looks good for the most part, just a naming nit pick and a question about the
> > deprecated AUDIT_VERSION_* defines; see below ...
> >
> > > diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> > > index 4d100c8..74aa584 100644
> > > --- a/include/uapi/linux/audit.h
> > > +++ b/include/uapi/linux/audit.h
> > > @@ -322,9 +322,15 @@ enum {
> > >  #define AUDIT_STATUS_BACKLOG_LIMIT	0x0010
> > >  #define AUDIT_STATUS_BACKLOG_WAIT_TIME	0x0020
> > >
> > > -#define AUDIT_VERSION_BACKLOG_LIMIT	1
> > > -#define AUDIT_VERSION_BACKLOG_WAIT_TIME	2
> > > -#define AUDIT_VERSION_LATEST AUDIT_VERSION_BACKLOG_WAIT_TIME
> > > +#define AUDIT_FEATURE_BITMAP_BACKLOG_LIMIT	0x00000001
> > > +#define AUDIT_FEATURE_BITMAP_BACKLOG_WAIT_TIME	0x00000002
> > > +#define AUDIT_FEATURE_BITMAP ( AUDIT_FEATURE_BITMAP_BACKLOG_LIMIT | \
> > > +				                AUDIT_FEATURE_BITMAP_BACKLOG_WAIT_TIME )
> >
> > How about AUDIT_FEATURE_BIMAP_ALL instead of just AUDIT_FEATURE_BITMAP?
>
> Sure, I'm fine with that.
>
> > > +/* deprecated: AUDIT_VERSION_* */
> > > +#define AUDIT_VERSION_LATEST 		AUDIT_FEATURE_BITMAP
> > > +#define AUDIT_VERSION_BACKLOG_LIMIT	AUDIT_FEATURE_BITMAP_BACKLOG_LIMIT
> > > +#define AUDIT_VERSION_BACKLOG_WAIT_TIME
> > > ...                                AUDIT_FEATURE_BITMAP_BACKLOG_WAIT_TIME
> >
> > So what terrible things happen to userspace if 
> AUDIT_VERSION_BACKLOG_WAIT_TIME
> > becomes 0x03 instead of 0x02?
>
> But it won't.  It gets the value of
> AUDIT_FEATURE_BITMAP_BACKLOG_WAIT_TIME, which is 0x00000002.
>
> I think you meant to ask about AUDIT_VERSION_LATEST, which would become 3.
>
> You *did* already ask that question in a previous thread, and there
> didn't seem to be a concern.  Steve Grubb could likely answer this
> question better than me.
>
> > 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
> --
> 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/

Re: [PATCH] audit: convert status version to a feature bitmap

[Steve Grubb]

On Thursday, November 13, 2014 08:08:52 PM Richard Guy Briggs wrote:
> > So what terrible things happen to userspace if
> > AUDIT_VERSION_BACKLOG_WAIT_TIME  becomes 0x03 instead of 0x02?
> 
> But it won't.  It gets the value of
> AUDIT_FEATURE_BITMAP_BACKLOG_WAIT_TIME, which is 0x00000002.
> 
> I think you meant to ask about AUDIT_VERSION_LATEST, which would become 3.
> 
> You *did* already ask that question in a previous thread, and there
> didn't seem to be a concern.  Steve Grubb could likely answer this
> question better than me.

The audit 2.4.1 package has been pushed to everything from F20 -> rawhide. If 
you don't see any problems, then its safe. But check carefully around the 
things that you did change. Right now, we only are caring about only one 
kernel feature, --loginuid-immutable. Check that it still works, auditctl -s.

-Steve

Re: [PATCH] audit: convert status version to a feature bitmap

[Richard Guy Briggs]

On 14/11/13, Paul Moore wrote:
> On Thursday, November 13, 2014 03:29:10 PM Richard Guy Briggs wrote:
> > The version field defined in the audit status structure was found to have
> > limitations in terms of its expressibility of features supported.  This is
> > distict from the get/set features call to be able to command those features
> > that are present.
> > 
> > Converting this field from a version number to a feature bitmap will allow
> > distributions to selectively backport and support certain features and will
> > allow upstream to be able to deprecate features in the future.  It will
> > allow userspace clients to first query the kernel for which features are
> > actually present and supported.  Currently, EINVAL is returned rather than
> > EOPNOTSUP, which isn't helpful in determining if there was an error in the
> > command, or if it simply isn't supported yet.  Past features are not
> > represented by this bitmap, but their use may be converted to EOPNOTSUP if
> > needed in the future.
> > 
> > Since "version" is too generic to convert with a #define, use a union in the
> > struct status, introducing the member "feature_bitmap" unionized with
> > "version".
> > 
> > Convert existing AUDIT_VERSION_* macros over to AUDIT_FEATURE_BITMAP*
> > counterparts, leaving the former for backwards compatibility.
> > 
> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > ---
> >  include/uapi/linux/audit.h |   17 +++++++++++++----
> >  kernel/audit.c             |    2 +-
> >  2 files changed, 14 insertions(+), 5 deletions(-)
> 
> Looks good for the most part, just a naming nit pick and a question about the 
> deprecated AUDIT_VERSION_* defines; see below ...
> 
> > diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> > index 4d100c8..74aa584 100644
> > --- a/include/uapi/linux/audit.h
> > +++ b/include/uapi/linux/audit.h
> > @@ -322,9 +322,15 @@ enum {
> >  #define AUDIT_STATUS_BACKLOG_LIMIT	0x0010
> >  #define AUDIT_STATUS_BACKLOG_WAIT_TIME	0x0020
> > 
> > -#define AUDIT_VERSION_BACKLOG_LIMIT	1
> > -#define AUDIT_VERSION_BACKLOG_WAIT_TIME	2
> > -#define AUDIT_VERSION_LATEST AUDIT_VERSION_BACKLOG_WAIT_TIME
> > +#define AUDIT_FEATURE_BITMAP_BACKLOG_LIMIT	0x00000001
> > +#define AUDIT_FEATURE_BITMAP_BACKLOG_WAIT_TIME	0x00000002
> > +#define AUDIT_FEATURE_BITMAP ( AUDIT_FEATURE_BITMAP_BACKLOG_LIMIT | \
> > +				                AUDIT_FEATURE_BITMAP_BACKLOG_WAIT_TIME )
> 
> How about AUDIT_FEATURE_BIMAP_ALL instead of just AUDIT_FEATURE_BITMAP?

Sure, I'm fine with that.

> > +/* deprecated: AUDIT_VERSION_* */
> > +#define AUDIT_VERSION_LATEST 		AUDIT_FEATURE_BITMAP
> > +#define AUDIT_VERSION_BACKLOG_LIMIT	AUDIT_FEATURE_BITMAP_BACKLOG_LIMIT
> > +#define AUDIT_VERSION_BACKLOG_WAIT_TIME
> > ...                                AUDIT_FEATURE_BITMAP_BACKLOG_WAIT_TIME
> 
> So what terrible things happen to userspace if AUDIT_VERSION_BACKLOG_WAIT_TIME 
> becomes 0x03 instead of 0x02?

But it won't.  It gets the value of
AUDIT_FEATURE_BITMAP_BACKLOG_WAIT_TIME, which is 0x00000002.

I think you meant to ask about AUDIT_VERSION_LATEST, which would become 3.

You *did* already ask that question in a previous thread, and there
didn't seem to be a concern.  Steve Grubb could likely answer this
question better than me.

> 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] audit: convert status version to a feature bitmap

[Richard Guy Briggs]

On 14/11/13, Joe Perches wrote:
> On Thu, 2014-11-13 at 15:29 -0500, Richard Guy Briggs wrote:
> > The version field defined in the audit status structure was found to have
> > limitations in terms of its expressibility of features supported.  This is
> > distict from the get/set features call to be able to command those features
> > that are present.
> > 
> > Converting this field from a version number to a feature bitmap will allow
> > distributions to selectively backport and support certain features and will
> > allow upstream to be able to deprecate features in the future.  It will allow
> > userspace clients to first query the kernel for which features are actually
> > present and supported.  Currently, EINVAL is returned rather than EOPNOTSUP,
> > which isn't helpful in determining if there was an error in the command, or if
> > it simply isn't supported yet.  Past features are not represented by this
> > bitmap, but their use may be converted to EOPNOTSUP if needed in the future.
> 
> Maybe use DECLARE_BITMAP instead of u32 and test_bit/set_bit

I don't think so.  I'd like to code to be readable...  I certainly don't
need the overhead of test/set_bit.  That doesn't look appropriate for
anything in include/uapi/.

> > diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> 
> > @@ -322,9 +322,15 @@ enum {
> >  #define AUDIT_STATUS_BACKLOG_LIMIT	0x0010
> >  #define AUDIT_STATUS_BACKLOG_WAIT_TIME	0x0020
> >  
> > -#define AUDIT_VERSION_BACKLOG_LIMIT	1
> > -#define AUDIT_VERSION_BACKLOG_WAIT_TIME	2
> > -#define AUDIT_VERSION_LATEST AUDIT_VERSION_BACKLOG_WAIT_TIME
> > +#define AUDIT_FEATURE_BITMAP_BACKLOG_LIMIT	0x00000001
> > +#define AUDIT_FEATURE_BITMAP_BACKLOG_WAIT_TIME	0x00000002
> > +#define AUDIT_FEATURE_BITMAP (	AUDIT_FEATURE_BITMAP_BACKLOG_LIMIT     | \
> > +				AUDIT_FEATURE_BITMAP_BACKLOG_WAIT_TIME   )
> > +
> > +/* deprecated: AUDIT_VERSION_* */
> > +#define AUDIT_VERSION_LATEST 		AUDIT_FEATURE_BITMAP
> > +#define AUDIT_VERSION_BACKLOG_LIMIT	AUDIT_FEATURE_BITMAP_BACKLOG_LIMIT
> > +#define AUDIT_VERSION_BACKLOG_WAIT_TIME	AUDIT_FEATURE_BITMAP_BACKLOG_WAIT_TIME
> >  
> >  				/* Failure-to-log actions */
> >  #define AUDIT_FAIL_SILENT	0
> > @@ -403,7 +409,10 @@ struct audit_status {
> >  	__u32		backlog_limit;	/* waiting messages limit */
> >  	__u32		lost;		/* messages lost */
> >  	__u32		backlog;	/* messages waiting in queue */
> > -	__u32		version;	/* audit api version number */
> > +	union {
> > +		__u32	version;	/* deprecated: audit api version num */
> > +		__u32	feature_bitmap;	/* bitmap of kernel audit features */
> > +	};
> >  	__u32		backlog_wait_time;/* message queue wait timeout */
> >  };
> >  
> > diff --git a/kernel/audit.c b/kernel/audit.c
> > index 8ee4508..c9d0e30 100644
> > --- a/kernel/audit.c
> > +++ b/kernel/audit.c
> > @@ -842,7 +842,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
> >  		s.backlog_limit		= audit_backlog_limit;
> >  		s.lost			= atomic_read(&audit_lost);
> >  		s.backlog		= skb_queue_len(&audit_skb_queue);
> > -		s.version		= AUDIT_VERSION_LATEST;
> > +		s.feature_bitmap	= AUDIT_FEATURE_BITMAP;
> >  		s.backlog_wait_time	= audit_backlog_wait_time;
> >  		audit_send_reply(skb, seq, AUDIT_GET, 0, 0, &s, sizeof(s));
> >  		break;

- 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] audit: convert status version to a feature bitmap

[Paul Moore]

On Thursday, November 13, 2014 03:29:10 PM Richard Guy Briggs wrote:
> The version field defined in the audit status structure was found to have
> limitations in terms of its expressibility of features supported.  This is
> distict from the get/set features call to be able to command those features
> that are present.
> 
> Converting this field from a version number to a feature bitmap will allow
> distributions to selectively backport and support certain features and will
> allow upstream to be able to deprecate features in the future.  It will
> allow userspace clients to first query the kernel for which features are
> actually present and supported.  Currently, EINVAL is returned rather than
> EOPNOTSUP, which isn't helpful in determining if there was an error in the
> command, or if it simply isn't supported yet.  Past features are not
> represented by this bitmap, but their use may be converted to EOPNOTSUP if
> needed in the future.
> 
> Since "version" is too generic to convert with a #define, use a union in the
> struct status, introducing the member "feature_bitmap" unionized with
> "version".
> 
> Convert existing AUDIT_VERSION_* macros over to AUDIT_FEATURE_BITMAP*
> counterparts, leaving the former for backwards compatibility.
> 
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
>  include/uapi/linux/audit.h |   17 +++++++++++++----
>  kernel/audit.c             |    2 +-
>  2 files changed, 14 insertions(+), 5 deletions(-)

Looks good for the most part, just a naming nit pick and a question about the 
deprecated AUDIT_VERSION_* defines; see below ...

> diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> index 4d100c8..74aa584 100644
> --- a/include/uapi/linux/audit.h
> +++ b/include/uapi/linux/audit.h
> @@ -322,9 +322,15 @@ enum {
>  #define AUDIT_STATUS_BACKLOG_LIMIT	0x0010
>  #define AUDIT_STATUS_BACKLOG_WAIT_TIME	0x0020
> 
> -#define AUDIT_VERSION_BACKLOG_LIMIT	1
> -#define AUDIT_VERSION_BACKLOG_WAIT_TIME	2
> -#define AUDIT_VERSION_LATEST AUDIT_VERSION_BACKLOG_WAIT_TIME
> +#define AUDIT_FEATURE_BITMAP_BACKLOG_LIMIT	0x00000001
> +#define AUDIT_FEATURE_BITMAP_BACKLOG_WAIT_TIME	0x00000002
> +#define AUDIT_FEATURE_BITMAP ( AUDIT_FEATURE_BITMAP_BACKLOG_LIMIT | \
> +				                AUDIT_FEATURE_BITMAP_BACKLOG_WAIT_TIME )

How about AUDIT_FEATURE_BIMAP_ALL instead of just AUDIT_FEATURE_BITMAP?

> +/* deprecated: AUDIT_VERSION_* */
> +#define AUDIT_VERSION_LATEST 		AUDIT_FEATURE_BITMAP
> +#define AUDIT_VERSION_BACKLOG_LIMIT	AUDIT_FEATURE_BITMAP_BACKLOG_LIMIT
> +#define AUDIT_VERSION_BACKLOG_WAIT_TIME
> ...                                AUDIT_FEATURE_BITMAP_BACKLOG_WAIT_TIME

So what terrible things happen to userspace if AUDIT_VERSION_BACKLOG_WAIT_TIME 
becomes 0x03 instead of 0x02?

-- 
paul moore
security and virtualization @ redhat

Re: [PATCH] audit: convert status version to a feature bitmap

[Paul Moore]

On Thursday, November 13, 2014 12:38:17 PM Joe Perches wrote:
> On Thu, 2014-11-13 at 15:29 -0500, Richard Guy Briggs wrote:
> > The version field defined in the audit status structure was found to have
> > limitations in terms of its expressibility of features supported.  This is
> > distict from the get/set features call to be able to command those
> > features
> > that are present.
> > 
> > Converting this field from a version number to a feature bitmap will allow
> > distributions to selectively backport and support certain features and
> > will
> > allow upstream to be able to deprecate features in the future.  It will
> > allow userspace clients to first query the kernel for which features are
> > actually present and supported.  Currently, EINVAL is returned rather
> > than EOPNOTSUP, which isn't helpful in determining if there was an error
> > in the command, or if it simply isn't supported yet.  Past features are
> > not represented by this bitmap, but their use may be converted to
> > EOPNOTSUP if needed in the future.
>
> Maybe use DECLARE_BITMAP instead of u32 and test_bit/set_bit

The audit_status struct is user visible and the version field is currently a 
u32 where DECLARE_BITMAP is an unsigned long.

-- 
paul moore
security and virtualization @ redhat

Re: [PATCH] audit: convert status version to a feature bitmap

[Joe Perches]

On Thu, 2014-11-13 at 15:29 -0500, Richard Guy Briggs wrote:
> The version field defined in the audit status structure was found to have
> limitations in terms of its expressibility of features supported.  This is
> distict from the get/set features call to be able to command those features
> that are present.
> 
> Converting this field from a version number to a feature bitmap will allow
> distributions to selectively backport and support certain features and will
> allow upstream to be able to deprecate features in the future.  It will allow
> userspace clients to first query the kernel for which features are actually
> present and supported.  Currently, EINVAL is returned rather than EOPNOTSUP,
> which isn't helpful in determining if there was an error in the command, or if
> it simply isn't supported yet.  Past features are not represented by this
> bitmap, but their use may be converted to EOPNOTSUP if needed in the future.

Maybe use DECLARE_BITMAP instead of u32 and test_bit/set_bit

> diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h

> @@ -322,9 +322,15 @@ enum {
>  #define AUDIT_STATUS_BACKLOG_LIMIT	0x0010
>  #define AUDIT_STATUS_BACKLOG_WAIT_TIME	0x0020
>  
> -#define AUDIT_VERSION_BACKLOG_LIMIT	1
> -#define AUDIT_VERSION_BACKLOG_WAIT_TIME	2
> -#define AUDIT_VERSION_LATEST AUDIT_VERSION_BACKLOG_WAIT_TIME
> +#define AUDIT_FEATURE_BITMAP_BACKLOG_LIMIT	0x00000001
> +#define AUDIT_FEATURE_BITMAP_BACKLOG_WAIT_TIME	0x00000002
> +#define AUDIT_FEATURE_BITMAP (	AUDIT_FEATURE_BITMAP_BACKLOG_LIMIT     | \
> +				AUDIT_FEATURE_BITMAP_BACKLOG_WAIT_TIME   )
> +
> +/* deprecated: AUDIT_VERSION_* */
> +#define AUDIT_VERSION_LATEST 		AUDIT_FEATURE_BITMAP
> +#define AUDIT_VERSION_BACKLOG_LIMIT	AUDIT_FEATURE_BITMAP_BACKLOG_LIMIT
> +#define AUDIT_VERSION_BACKLOG_WAIT_TIME	AUDIT_FEATURE_BITMAP_BACKLOG_WAIT_TIME
>  
>  				/* Failure-to-log actions */
>  #define AUDIT_FAIL_SILENT	0
> @@ -403,7 +409,10 @@ struct audit_status {
>  	__u32		backlog_limit;	/* waiting messages limit */
>  	__u32		lost;		/* messages lost */
>  	__u32		backlog;	/* messages waiting in queue */
> -	__u32		version;	/* audit api version number */
> +	union {
> +		__u32	version;	/* deprecated: audit api version num */
> +		__u32	feature_bitmap;	/* bitmap of kernel audit features */
> +	};
>  	__u32		backlog_wait_time;/* message queue wait timeout */
>  };
>  
> diff --git a/kernel/audit.c b/kernel/audit.c
> index 8ee4508..c9d0e30 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -842,7 +842,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
>  		s.backlog_limit		= audit_backlog_limit;
>  		s.lost			= atomic_read(&audit_lost);
>  		s.backlog		= skb_queue_len(&audit_skb_queue);
> -		s.version		= AUDIT_VERSION_LATEST;
> +		s.feature_bitmap	= AUDIT_FEATURE_BITMAP;
>  		s.backlog_wait_time	= audit_backlog_wait_time;
>  		audit_send_reply(skb, seq, AUDIT_GET, 0, 0, &s, sizeof(s));
>  		break;

[PATCH] audit: convert status version to a feature bitmap

[Richard Guy Briggs]

The version field defined in the audit status structure was found to have
limitations in terms of its expressibility of features supported.  This is
distict from the get/set features call to be able to command those features
that are present.

Converting this field from a version number to a feature bitmap will allow
distributions to selectively backport and support certain features and will
allow upstream to be able to deprecate features in the future.  It will allow
userspace clients to first query the kernel for which features are actually
present and supported.  Currently, EINVAL is returned rather than EOPNOTSUP,
which isn't helpful in determining if there was an error in the command, or if
it simply isn't supported yet.  Past features are not represented by this
bitmap, but their use may be converted to EOPNOTSUP if needed in the future.

Since "version" is too generic to convert with a #define, use a union in the
struct status, introducing the member "feature_bitmap" unionized with
"version".

Convert existing AUDIT_VERSION_* macros over to AUDIT_FEATURE_BITMAP*
counterparts, leaving the former for backwards compatibility.

Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
 include/uapi/linux/audit.h |   17 +++++++++++++----
 kernel/audit.c             |    2 +-
 2 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
index 4d100c8..74aa584 100644
--- a/include/uapi/linux/audit.h
+++ b/include/uapi/linux/audit.h
@@ -322,9 +322,15 @@ enum {
 #define AUDIT_STATUS_BACKLOG_LIMIT	0x0010
 #define AUDIT_STATUS_BACKLOG_WAIT_TIME	0x0020
 
-#define AUDIT_VERSION_BACKLOG_LIMIT	1
-#define AUDIT_VERSION_BACKLOG_WAIT_TIME	2
-#define AUDIT_VERSION_LATEST AUDIT_VERSION_BACKLOG_WAIT_TIME
+#define AUDIT_FEATURE_BITMAP_BACKLOG_LIMIT	0x00000001
+#define AUDIT_FEATURE_BITMAP_BACKLOG_WAIT_TIME	0x00000002
+#define AUDIT_FEATURE_BITMAP (	AUDIT_FEATURE_BITMAP_BACKLOG_LIMIT     | \
+				AUDIT_FEATURE_BITMAP_BACKLOG_WAIT_TIME   )
+
+/* deprecated: AUDIT_VERSION_* */
+#define AUDIT_VERSION_LATEST 		AUDIT_FEATURE_BITMAP
+#define AUDIT_VERSION_BACKLOG_LIMIT	AUDIT_FEATURE_BITMAP_BACKLOG_LIMIT
+#define AUDIT_VERSION_BACKLOG_WAIT_TIME	AUDIT_FEATURE_BITMAP_BACKLOG_WAIT_TIME
 
 				/* Failure-to-log actions */
 #define AUDIT_FAIL_SILENT	0
@@ -403,7 +409,10 @@ struct audit_status {
 	__u32		backlog_limit;	/* waiting messages limit */
 	__u32		lost;		/* messages lost */
 	__u32		backlog;	/* messages waiting in queue */
-	__u32		version;	/* audit api version number */
+	union {
+		__u32	version;	/* deprecated: audit api version num */
+		__u32	feature_bitmap;	/* bitmap of kernel audit features */
+	};
 	__u32		backlog_wait_time;/* message queue wait timeout */
 };
 
diff --git a/kernel/audit.c b/kernel/audit.c
index 8ee4508..c9d0e30 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -842,7 +842,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
 		s.backlog_limit		= audit_backlog_limit;
 		s.lost			= atomic_read(&audit_lost);
 		s.backlog		= skb_queue_len(&audit_skb_queue);
-		s.version		= AUDIT_VERSION_LATEST;
+		s.feature_bitmap	= AUDIT_FEATURE_BITMAP;
 		s.backlog_wait_time	= audit_backlog_wait_time;
 		audit_send_reply(skb, seq, AUDIT_GET, 0, 0, &s, sizeof(s));
 		break;
-- 
1.7.1