Re: [PATCH 1/7] audit: implement generic feature setting and retrieving

[William Roberts <bill.c.roberts@gmail.com>]

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

A couple other comments I missed before:

 #define AUDIT_FEATURE_TO_MASK(x)       (1 << ((x) & 31)) /* mask for __u32
*/

Why &31 --> if someone adds more than 5 things, they will git dropped out...

 #define AUDIT_FEATURE_TO_MASK(x)       (1 << ((x) & (~(__u32)0))) /* mask
for __u32 */

something like this perhaps?

Also
static char *audit_feature_names[2] = { ...

Drop the number, I had to inc this when I added mine...

Bill


On Fri, May 24, 2013 at 1:41 PM, William Roberts
<bill.c.roberts@gmail.com>wrote:

> Looking through the patch, these are my thoughts:
>
> I like that my "splitlog" patch shrunk a lot, way easier. I like that. It
> also proves something like this is the correct direction.
>
> Shouldn't audit_set_feature() check the version number, granted it doesn't
> mater now, but shouldn't their be a:
>
>  if(uaf->version <= AUDIT_FEATURE_SUPPORTED_VERSION)
>
>
> This branchy code could be:
> if (new_feature)
> af.features |= feature;
>  else
> af.features &= ~feature;
>
> changed to:
> af.features = (af.features & ~feature) | feature
>
> Ie just do a clear bit than or in what you want.
>
> Otherwise LGTM
>
> Bill
>
>
>
>
>
> On Fri, May 24, 2013 at 9:28 AM, Eric Paris <eparis@redhat.com> wrote:
>
>> On Fri, 2013-05-24 at 12:11 -0400, Eric Paris wrote:
>> > The audit_status structure was not designed with extensibility in mind.
>> > Define a new AUDIT_SET_FEATURE message type which takes a new structure
>> > of bits where things can be enabled/disabled/locked one at a time.  This
>> > structure should be able to grow in the future while maintaining forward
>> > and backward compatibility (based loosly on the ideas from capabilities
>> > and prctl)
>> >
>> > This does not actually add any features, but is just infrastructure to
>> > allow new on/off types of audit system features.
>> >
>> > Signed-off-by: Eric Paris <eparis@redhat.com>
>>
>> Attached you will find the test program I used to check that things were
>> working correctly.  It should give an idea to Steve how we can program
>> the features support in userspace.  I believe it fits very nicely to
>> have a new syntax in audit.rules to set (and lock if needed/wanted)
>> these features.
>>
>> netlink.c is just some helper code I stole from the audit tree to get
>> some functions which weren't exposed externally.  The only part really
>> interesting is test.c.
>>
>> You will also need the include/uapi/linux/audit.h file from this patch
>> to build test.c
>>
>> -Eric
>>
>> --
>> Linux-audit mailing list
>> Linux-audit@redhat.com
>> https://www.redhat.com/mailman/listinfo/linux-audit
>>
>
>
>
> --
> Respectfully,
>
> William C Roberts
>
>


-- 
Respectfully,

William C Roberts

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

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