<div dir="ltr">A couple other comments I missed before:<div><br></div><div><div> #define AUDIT_FEATURE_TO_MASK(x) (1 << ((x) & 31)) /* mask for __u32 */</div></div><div><br></div><div style>Why &31 --> if someone adds more than 5 things, they will git dropped out...</div>
<div><br></div><div> #define AUDIT_FEATURE_TO_MASK(x) (1 << ((x) & (~(__u32)0))) /* mask for __u32 */<br></div><div><br></div><div style>something like this perhaps?</div><div><br></div><div style>Also</div>
<div style><div>static char *audit_feature_names[2] = { ...</div><div><br></div><div style>Drop the number, I had to inc this when I added mine... </div><div><br></div></div><div style>Bill</div></div><div class="gmail_extra">
<br><br><div class="gmail_quote">On Fri, May 24, 2013 at 1:41 PM, William Roberts <span dir="ltr"><<a href="mailto:bill.c.roberts@gmail.com" target="_blank">bill.c.roberts@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div dir="ltr">Looking through the patch, these are my thoughts:<div><br></div><div>I like that my "splitlog" patch shrunk a lot, way easier. I like that. It also proves something like this is the correct direction.</div>
<div><br></div><div>Shouldn't audit_set_feature() check the version number, granted it doesn't mater now, but shouldn't their be a:</div><div><br></div><div> if(uaf->version <= AUDIT_FEATURE_SUPPORTED_VERSION)</div>
<div>
<br></div><div><br></div><div>This branchy code could be:</div><div><div><span style="white-space:pre-wrap"> </span>if (new_feature)</div><div><span style="white-space:pre-wrap"> </span>af.features |= feature;</div>
<div><span style="white-space:pre-wrap"> </span>else</div><div><span style="white-space:pre-wrap"> </span>af.features &= ~feature;</div></div><div><span style="color:rgb(51,51,51);font-family:'Bitstream Vera Sans Mono','DejaVu Sans Mono',Monaco,monospace;font-size:12px;line-height:1.4"><br>
</span></div><div><span style="color:rgb(51,51,51);font-family:'Bitstream Vera Sans Mono','DejaVu Sans Mono',Monaco,monospace;font-size:12px;line-height:1.4">changed to:</span></div><div><span style="color:rgb(51,51,51);font-family:'Bitstream Vera Sans Mono','DejaVu Sans Mono',Monaco,monospace;font-size:12px;line-height:1.4">af.features </span><span style="color:rgb(51,51,51);font-family:'Bitstream Vera Sans Mono','DejaVu Sans Mono',Monaco,monospace;font-size:12px;line-height:1.4">=</span><span style="color:rgb(51,51,51);font-family:'Bitstream Vera Sans Mono','DejaVu Sans Mono',Monaco,monospace;font-size:12px;line-height:1.4"> </span><span style="color:rgb(51,51,51);font-family:'Bitstream Vera Sans Mono','DejaVu Sans Mono',Monaco,monospace;font-size:12px;line-height:1.4">(af.features</span><span style="color:rgb(51,51,51);font-family:'Bitstream Vera Sans Mono','DejaVu Sans Mono',Monaco,monospace;font-size:12px;line-height:1.4"> </span><span style="color:rgb(51,51,51);font-family:'Bitstream Vera Sans Mono','DejaVu Sans Mono',Monaco,monospace;font-size:12px;line-height:1.4">&</span><span style="color:rgb(51,51,51);font-family:'Bitstream Vera Sans Mono','DejaVu Sans Mono',Monaco,monospace;font-size:12px;line-height:1.4"> </span><span style="color:rgb(51,51,51);font-family:'Bitstream Vera Sans Mono','DejaVu Sans Mono',Monaco,monospace;font-size:12px;line-height:1.4">~</span><span style="font-family:'Bitstream Vera Sans Mono','DejaVu Sans Mono',Monaco,monospace;font-size:12px;line-height:1.4"><font color="#009999">feature</font></span><span style="color:rgb(51,51,51);font-family:'Bitstream Vera Sans Mono','DejaVu Sans Mono',Monaco,monospace;font-size:12px;line-height:1.4">)</span><span style="color:rgb(51,51,51);font-family:'Bitstream Vera Sans Mono','DejaVu Sans Mono',Monaco,monospace;font-size:12px;line-height:1.4"> </span><span style="color:rgb(51,51,51);font-family:'Bitstream Vera Sans Mono','DejaVu Sans Mono',Monaco,monospace;font-size:12px;line-height:1.4">|</span><span style="color:rgb(51,51,51);font-family:'Bitstream Vera Sans Mono','DejaVu Sans Mono',Monaco,monospace;font-size:12px;line-height:1.4"> feature</span><br>
</div><div><br></div><div>Ie just do a clear bit than or in what you want.</div><div><br></div><div>Otherwise LGTM</div><div><br></div><div>
Bill</div><div><br></div><div><br></div><div><br></div></div><div class="gmail_extra"><br><br><div class="gmail_quote"><div><div class="h5">On Fri, May 24, 2013 at 9:28 AM, Eric Paris <span dir="ltr"><<a href="mailto:eparis@redhat.com" target="_blank">eparis@redhat.com</a>></span> wrote:<br>
</div></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div class="h5"><div>On Fri, 2013-05-24 at 12:11 -0400, Eric Paris wrote:<br>
> The audit_status structure was not designed with extensibility in mind.<br>
> Define a new AUDIT_SET_FEATURE message type which takes a new structure<br>
> of bits where things can be enabled/disabled/locked one at a time. This<br>
> structure should be able to grow in the future while maintaining forward<br>
> and backward compatibility (based loosly on the ideas from capabilities<br>
> and prctl)<br>
><br>
> This does not actually add any features, but is just infrastructure to<br>
> allow new on/off types of audit system features.<br>
><br>
> Signed-off-by: Eric Paris <<a href="mailto:eparis@redhat.com" target="_blank">eparis@redhat.com</a>><br>
<br>
</div>Attached you will find the test program I used to check that things were<br>
working correctly. It should give an idea to Steve how we can program<br>
the features support in userspace. I believe it fits very nicely to<br>
have a new syntax in audit.rules to set (and lock if needed/wanted)<br>
these features.<br>
<br>
netlink.c is just some helper code I stole from the audit tree to get<br>
some functions which weren't exposed externally. The only part really<br>
interesting is test.c.<br>
<br>
You will also need the include/uapi/linux/audit.h file from this patch<br>
to build test.c<br>
<span><font color="#888888"><br>
-Eric<br>
</font></span><br></div></div><div class="im">--<br>
Linux-audit mailing list<br>
<a href="mailto:Linux-audit@redhat.com" target="_blank">Linux-audit@redhat.com</a><br>
<a href="https://www.redhat.com/mailman/listinfo/linux-audit" target="_blank">https://www.redhat.com/mailman/listinfo/linux-audit</a><br></div></blockquote></div><br><br clear="all"><div class="im"><div><br></div>-- <br>
Respectfully,<br><br>William C Roberts<br>
<br>
</div></div>
</blockquote></div><br><br clear="all"><div><br></div>-- <br>Respectfully,<br><br>William C Roberts<br><br>
</div>