audit_status in kernel

audit_status in kernel

[Steve Grubb]

Hello,

I was looking at a new kernel and see that the audit_status structure has 
changed. The first member of the structure is a bit mask that tells what all is 
in the structure. So, if we add this:

        __u32           version;        /* audit api version number */
        __u32           backlog_wait_time;/* message queue wait timeout */
};

Then we need to have a define for those two:

#define AUDIT_STATUS_BACKLOG_LIMIT      0x0010
+#define AUDIT_STATTUS_VERSION			0x0020
-#define AUDIT_STATUS_BACKLOG_WAIT_TIME  0x0020
+#define AUDIT_STATUS_BACKLOG_WAIT_TIME  0x0040

IOW, each entry in the structure is supposed to have a mask value.

-Steve

Re: audit_status in kernel

[Steve Grubb]

On Monday, March 10, 2014 05:48:06 PM Steve Grubb wrote:
> Hello,
> 
> I was looking at a new kernel and see that the audit_status structure has
> changed. The first member of the structure is a bit mask that tells what all
> is in the structure. So, if we add this:
> 
>         __u32           version;        /* audit api version number */
>         __u32           backlog_wait_time;/* message queue wait timeout */
> };
> 
> Then we need to have a define for those two:
> 
> #define AUDIT_STATUS_BACKLOG_LIMIT      0x0010
> +#define AUDIT_STATTUS_VERSION			0x0020
> -#define AUDIT_STATUS_BACKLOG_WAIT_TIME  0x0020
> +#define AUDIT_STATUS_BACKLOG_WAIT_TIME  0x0040
> 
> IOW, each entry in the structure is supposed to have a mask value.

Actually, I think the problems are worse. We have the issue of an expanding 
data structure over time as new things get added. But yet we have a fixed sized 
audit_status structure. I could copy that into the audit package's source code 
so that I have the new structure definition. But I will have to do the same 
thing each time. Also, how would an old kernel tolerate a bigger audit_status 
structure being sent with AUDIT_SET commands by a new auditctl?

What we should do is leave AUDIT_GET the way it was. We should then define 
AUDIT_GET_EXT and then use it a lot like audit_rule_data which has 

struct audit_status_ext {
        __u32           field_count;
        __u32           fields[AUDIT_MAX_FIELDS];
        __u32           values[AUDIT_MAX_FIELDS];
};

Then insert the field identifier in fields and the value in the other. This way 
the format is defined once and we can reuse it for a long time.

>From user space, the migration would be easy. Old auditctl still uses 
AUDIT_GET. New auditctl would try AUDIT_GET_EXT and if that's not recognized, 
drop back to AUDIT_GET. Then one day down the road we remove AUDIT_GET in the 
kernel.

This is how we did the migration from AUDIT_RULES to AUDIT_RULES_DATA.

-Steve

Re: audit_status in kernel

[Eric Paris]

On Mon, 2014-03-10 at 18:25 -0400, Steve Grubb wrote:
> On Monday, March 10, 2014 05:48:06 PM Steve Grubb wrote:
> > Hello,
> > 
> > I was looking at a new kernel and see that the audit_status structure has
> > changed. The first member of the structure is a bit mask that tells what all
> > is in the structure.

That's not what the bit does.  The bit tells what portions of the struct
contain updates the kernel should upon.  Notice that the 'lost' and
'backlog' fields have no bit assigned.

> So, if we add this:
> > 
> >         __u32           version;        /* audit api version number */
> >         __u32           backlog_wait_time;/* message queue wait timeout */
> > };
> > 
> > Then we need to have a define for those two:
> > 
> > #define AUDIT_STATUS_BACKLOG_LIMIT      0x0010
> > +#define AUDIT_STATTUS_VERSION			0x0020
> > -#define AUDIT_STATUS_BACKLOG_WAIT_TIME  0x0020
> > +#define AUDIT_STATUS_BACKLOG_WAIT_TIME  0x0040
> > 
> > IOW, each entry in the structure is supposed to have a mask value.

If the version were a writable field then a bit would be necessary.  But
as of right now, it isn't writable, therefore the bit serves no purpose.
Just like 'lost' and 'backlog'

> Actually, I think the problems are worse. We have the issue of an expanding 
> data structure over time as new things get added. But yet we have a fixed sized 
> audit_status structure.

Sure, every program is compiled with a fixed size.  This is very common.
I don't recognize the problem.  You have to compile a new userspace to
make use of new functions/features.  No matter what.

>  I could copy that into the audit package's source code 
> so that I have the new structure definition. But I will have to do the same 
> thing each time.

As userspace comes to understand new parts of the structure userspace
would need to grow the definition.  Agreed.  Every design has this
issue.  Userspace can't understand a feature unless you tell it.

>  Also, how would an old kernel tolerate a bigger audit_status 
> structure being sent with AUDIT_SET commands by a new auditctl?

Unknown bits have always been ignored.  I thought we returned EINVAL on
unknown bits, but it seems we don't.  We could change that going
forward, but, it doesn't matter.  Userspace must query the kernel for
the version.  And can then obviously know what is/isn't supported.

> What we should do is leave AUDIT_GET the way it was. We should then define 
> AUDIT_GET_EXT and then use it a lot like audit_rule_data which has 
> 
> struct audit_status_ext {
>         __u32           field_count;
>         __u32           fields[AUDIT_MAX_FIELDS];
>         __u32           values[AUDIT_MAX_FIELDS];
> };
> 
> Then insert the field identifier in fields and the value in the other. This way 
> the format is defined once and we can reuse it for a long time.

That adds absolutely nothing except wasted memory and greater
readability problems...  We'd be sending around gigantic empty stuctures
for no gain...   (although on AUDIT_FEATURE_SET the current interface is
totally craptastic as all but one of the fields is typically useless,
but at least this is the craptastic we have today)

> From user space, the migration would be easy. Old auditctl still uses 
> AUDIT_GET. New auditctl would try AUDIT_GET_EXT and if that's not recognized, 
> drop back to AUDIT_GET. Then one day down the road we remove AUDIT_GET in the 
> kernel.
> 
> This is how we did the migration from AUDIT_RULES to AUDIT_RULES_DATA.

Great.  But this isn't needed here.  This isn't an ABI change.  Old
userspace will run just fine on new kernels.  New userspace will run
just fine on old kernels.  (Although (poorly coded userspace sending)
new features will be silently ignored instead of rejected, as I would
prefer)

It really is easy.  On GET userspace just needs to use

	struct audit_status     s;
	memset(&s, 0, sizeof(s));
	/* guard against past and future API changes */
	memcpy(&s, data, min_t(size_t, sizeof(s), nlmsg_len(nlh)));

if (s.version == 0)
	no new features, legacy api (notice this works because you memset the
userspace status struct before you copied in the kernel verion.  So
stuff at the end is all 0's

if (s.version == AUDIT_VERSION_BACKLOG_LIMIT)
	version field, but nothing else

if (s.version >= AUDIT_VERSION_BACKLOG_WAIT_TIME)
	you can set the version field!

And the struct can keep growing and keep being used!  Both ends know how
to handle things, all with no new API, no new deprecation, no breaking
working code.  I have actually written kernel<->userspace APIs before.
Admittedly this time I was starting with junk, but we still found a way
to do it allowing us to grow into the future and keep everything working
just the way it always has.

-Eric