From mboxrd@z Thu Jan 1 00:00:00 1970 From: William Roberts Subject: Re: [PATCH 1/7] audit: implement generic feature setting and retrieving Date: Fri, 24 May 2013 13:41:53 -0700 Message-ID: References: <1369411910-13777-1-git-send-email-eparis@redhat.com> <1369412920.2514.12.camel@dhcp137-228.rdu.redhat.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1764833092329642567==" Return-path: In-Reply-To: <1369412920.2514.12.camel@dhcp137-228.rdu.redhat.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: linux-audit-bounces@redhat.com Errors-To: linux-audit-bounces@redhat.com To: Eric Paris Cc: linux-audit@redhat.com List-Id: linux-audit@redhat.com --===============1764833092329642567== Content-Type: multipart/alternative; boundary=001a11c3421a41804804dd7cd4a9 --001a11c3421a41804804dd7cd4a9 Content-Type: text/plain; charset=ISO-8859-1 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 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 > > 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 --001a11c3421a41804804dd7cd4a9 Content-Type: text/html; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable
Looking through the patch, these are my thoughts:

=
I like that my "splitlog" patch shrunk a lot, way easi= er. I like that. It also proves something like this is the correct directio= n.

Shouldn't=A0audit_set_feature() check the version n= umber, granted it doesn't mater now, but shouldn't their be a:

=A0if(uaf->version <=3D AUDIT_FEATURE_SUPPORTED_= VERSION)


This branchy code could be:
<= div> if (new_feature)
af.features |= =3D feature;
else
af.features &=3D ~fea= ture;

changed to:
af.features=A0=3D=A0(af.features=A0&a= mp; ~feature) |=A0feature

Ie just do a clear bit than or in what you = want.

Otherwise LGTM

Bill





On Fri, May 24, 2013 at 9:2= 8 AM, Eric Paris <eparis@redhat.com> wrote:
On Fri, 2013-05-24 at 12:1= 1 -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 structur= e
> of bits where things can be enabled/disabled/locked one at a time. =A0= This
> structure should be able to grow in the future while maintaining forwa= rd
> and backward compatibility (based loosly on the ideas from capabilitie= s
> 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 <epa= ris@redhat.com>

Attached you will find the test program I used to check that things w= ere
working correctly. =A0It should give an idea to Steve how we can program the features support in userspace. =A0I 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. =A0The only part reall= y
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

--001a11c3421a41804804dd7cd4a9-- --===============1764833092329642567== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline --===============1764833092329642567==--