On 2021-05-25 12:06, Casey Schaufler wrote: > On 5/25/2021 11:23 AM, Richard Guy Briggs wrote: > > On 2021-05-25 10:28, Casey Schaufler wrote: > >> On 5/21/2021 1:19 PM, Paul Moore wrote: > >> > >> <snip> and trim the CC list. > >> > >>> Okay, we've got a disconnect here regarding "audit contexts" and > >>> "local contexts", skip down below where I attempt to explain things a > >>> little more but basically if there is a place that uses this pattern: > >>> > >>> audit_log_start(audit_context(), ...); > >> This pattern isn't helpful. audit_context() returns NULL in about 1 of 4 calls. > > Ok, this rings a bell... I think we talked about this on an earlier > > revision... > > > >> All of these callers of audit_context() get a NULL result some of the time. > >> > >> getname_kernel > >> debugfs_create_dir > >> tracefs_create_file > >> vfs_fchown > >> do_settimeofday64 > >> bprm_execve > >> ksys_mmap_pgoff > >> move_addr_to_kernel > >> __do_pipe_flags > >> __do_sys_capset > >> syscall_trace_enter > >> cap_bprm_creds_from_file > >> load_module > >> __x64_sys_fsetxattr > >> bpf_prog_load > >> audit_signal_info_syscall > >> sel_write_enforce > >> common_lsm_audit > >> audit_set_loginuid > >> __dev_set_promiscuity > >> ipcperms > >> devpts_pty_new > >> > >>> ... you don't need, or want, a "local context". You might need a > >>> local context if you see the following pattern: > >>> > >>> audit_log_start(NULL, ...); > >>> > >>> The "local context" idea is a hack and should be avoided whenever > >>> possible; if you have an existing audit context from a syscall, or > >>> something else, you *really* should use it ... or have a *really* good > >>> explanation as to why you can not. > >> Almost all audit events want to record the subject context by calling > >> audit_log_task_context(). If multiple contexts need to be recorded > >> there has to be a separate record, which means there has to be an > >> audit context. The only case where an audit context is reliably available > >> is in syscalls. Hence, a "local context" for many of the cases where the > >> first argument to audit_log_start() would otherwise be NULL, either > >> explicitly or because audit_context() returns NULL. > > Ok, so in that case, I think I'd test audit_context() and if it is > > indeed NULL then create a local context, otherwise use the one that is > > available. > > audit_alloc_for_lsm() returns the value of audit_context() if it is > not NULL. Otherwise it checks to see if a separate record will > be required. If it is the value from audit_alloc_local() is returned. > Otherwise, it returns NULL. > > > I should really go back and look carefully again at your > > code to see if it is in fact doing this, but unilaterally creating a > > local context if a context already exists is going to cause confusion > > because there will be two events generated for one event. > > Indeed. I had discovered that. > > > Is it possible these functions are not actually generating records if > > the context is NULL? > > There are definitely cases where records are generated when audit_context() > is NULL. > > > I'd be digging to find out why the context is NULL in these cases and if > > any record is even being produced in those cases? > > Context is NULL because they're not coming out of syscalls. Where are they coming from then? I'm guessing that they are in fact coming from syscalls, but that it wasn't a syscall rule that triggered the need to record the event. > > Perhaps there is an > > active filter that indicates that logging is not of interest for that > > process/task/file/syscall/perm/etc... > > > >> Is there some other way to synchronize the "timestamp" without use of > >> an audit context? My understanding is that this is the primary purpose > >> of the audit context. > > What has been done is to call it with a NULL context and it would assign > > a timestamp and serial number, but those are all single records that > > don't need synchronization (obviously). This was why I'd come up with > > this mechanism to solve the need to attach a contid record to a single > > record associated with a network event (or user record that should not > > be associated with a syscall). Those were the only two use cases I had > > up until now. > > Right. Adding the contid record is a special case. Adding the lsmcontext > record is the common case. Thus Paul's lament. Yeah, this is a similar sort of accompanying record that needs to be tied into an event which is why I had suggested the similarity behind your local context generation patch and the one in the contid patchset. > > - RGB - RGB -- Richard Guy Briggs <rgb@xxxxxxxxxx> Sr. S/W Engineer, Kernel Security, Base Operating Systems Remote, Ottawa, Red Hat Canada IRC: rgb, SunRaycer Voice: +1.647.777.2635, Internal: (81) 32635
