Re: [PATCH 2/2] selinux: add attributes to avc tracepoint

From: peter enderborg
Date: Thu Aug 06 2020 - 13:53:00 EST

On 8/6/20 5:03 PM, Stephen Smalley wrote:
> On Thu, Aug 6, 2020 at 10:51 AM peter enderborg
> <peter.enderborg@xxxxxxxx> wrote:
>> On 8/6/20 3:49 PM, Stephen Smalley wrote:
>>> On Thu, Aug 6, 2020 at 9:45 AM Stephen Smalley
>>> <> wrote:
>>>> On 8/6/20 8:32 AM, Stephen Smalley wrote:
>>>>> On 8/6/20 8:24 AM, peter enderborg wrote:
>>>>>> On 8/6/20 2:11 PM, Stephen Smalley wrote:
>>>>>>> On 8/6/20 4:03 AM, Thiébaud Weksteen wrote:
>>>>>>>> From: Peter Enderborg <peter.enderborg@xxxxxxxx>
>>>>>>>> Add further attributes to filter the trace events from AVC.
>>>>>>> Please include sample usage and output in the description.
>>>>>> Im not sure where you want it to be.
>>>>>> In the commit message or in a Documentation/trace/events-avc.rst ?
>>>>> I was just asking for it in the commit message / patch description. I
>>>>> don't know what is typical for Documentation/trace.
>>>> For example, I just took the patches for a spin, running the
>>>> selinux-testsuite under perf like so:
>>>> sudo perf record -e avc:selinux_audited -g make test
>>>> and then ran:
>>>> sudo perf report -g
>>>> and a snippet of sample output included:
>>>> 6.40% 6.40% requested=0x800000 denied=0x800000
>>>> audited=0x800000 result=-13 ssid=922 tsid=922
>>>> scontext=unconfined_u:unconfined_r:test_binder_mgr_t:s0-s0:c0.c1023
>>>> tcontext=unconfined_u:unconfined_r:test_binder_mgr_t:s0-s0:c0.c1023
>>>> tclass=capability
>>> So then the question becomes how do you use the above information,
>>> e.g. is that sufficient to correlate it to an actual avc: denied
>>> message, how do you decode the requested/denied/audited fields (or
>>> should the code do that for you and just report the string name(s) of
>>> the permission(s), do you need all three of those fields separately,
>>> is it useful to log the ssid/tsid at all given that you have the
>>> contexts and sids are dynamically assigned, etc.
>>>> |
>>>> ---0x495641000028933d
>>>> __libc_start_main
>>>> |
>>>> |--4.60%--__GI___ioctl
>>>> | entry_SYSCALL_64
>>>> | do_syscall_64
>>>> | __x64_sys_ioctl
>>>> | ksys_ioctl
>>>> | binder_ioctl
>>>> | binder_set_nice
>>>> | can_nice
>>>> | capable
>>>> | security_capable
>>>> | cred_has_capability.isra.0
>>>> | slow_avc_audit
>>>> | common_lsm_audit
>>>> | avc_audit_post_callback
>>>> | avc_audit_post_callback
>> The real cool thing happen when you enable "user-stack-trace" too.
>> <...>-4820 [007] .... 85878.897553: selinux_audited: requested=0x4000000 denied=0x4000000 audited=0x4000000 result=-13 ssid=341 tsid=61 scontext=system_u:system_r:ntpd_t:s0 tcontext=system_u:object_r:bin_t:s0 tclass=file
>> <...>-4820 [007] .... 85878.897572: <user stack trace>
>> => <00007f07d99bb45b>
>> => <0000555ecd89ca57>
>> The fields are useful for filter what you what to see and what you can ignore. Having the ssid and text was from the part where it is called.
>> The numeric can be used for two things. When you dont have any context. Same same reason as in post_callback. We need to be static
>> in format so it need be there if it ever can happen. And it is also useful for faster filtering.
>> You can do "ssid!=42 && ssid!=43 && tsid==666". From my view it would be good to have all fields there. But they need to right typed to be able
>> to use the filter mechanism. There must me some trade-off too where the argument filtering get bigger than the processing, but I think we can
>> add a lot more before we reach that threshold.
> I don't think the SIDs are useful because they are dynamically
> assigned (aside from the initial SIDs for bootstrapping before policy
> load) and are not exported to userspace (userspace has no way to look
> them up). It is probably a mistake that we even fall back to logging
> them in the existing code and may just be a legacy of when SIDs were
> exported to userspace (ancient history, before mainline merge of
> SELinux).
> In any event, if you were to include the sample usage and output I
> provided as part of the commit message / patch description (or replace
> with your own example), that would be helpful I think. Even better
> would be to also provide some hint as to how people are expected to
> decode the requested/denied/audited fields (I know how to do that but
> not everyone will, and ideally one would have a script or something
> for doing so).

But they are always the same until load off a new policy? And they can be mapped while debugging.

You see one event and then you "know" it. This is for developers so it is a different mindset than for the
netlink type that is for administrators.  Features for speeding things up are useful.

Im doing a update on commit message, will send it to Weksteen for a review round first.