On 4/26/22 11:57, Paul Moore wrote:
> On Mon, Apr 25, 2022 at 11:38 PM John Johansen
> <john.johansen@xxxxxxxxxxxxx> wrote:
>> On 4/18/22 07:59, Casey Schaufler wrote:
>>> Create a new audit record AUDIT_MAC_OBJ_CONTEXTS.
>>> An example of the MAC_OBJ_CONTEXTS (1421) record is:
>>>
>>> type=MAC_OBJ_CONTEXTS[1421]
>>> msg=audit(1601152467.009:1050):
>>> obj_selinux=unconfined_u:object_r:user_home_t:s0
>>>
>>> When an audit event includes a AUDIT_MAC_OBJ_CONTEXTS record
>>> the "obj=" field in other records in the event will be "obj=?".
>>> An AUDIT_MAC_OBJ_CONTEXTS record is supplied when the system has
>>> multiple security modules that may make access decisions based
>>> on an object security context.
>>>
>>> Signed-off-by: Casey Schaufler <casey@xxxxxxxxxxxxxxxx>
>>> ---
>>> include/linux/audit.h | 5 +++
>>> include/uapi/linux/audit.h | 1 +
>>> kernel/audit.c | 47 +++++++++++++++++++++++
>>> kernel/auditsc.c | 79 ++++++++++++--------------------------
>>> 4 files changed, 77 insertions(+), 55 deletions(-)
>
> ...
>
>>> diff --git a/kernel/audit.c b/kernel/audit.c
>>> index 8ed2d717c217..a8c3ec6ba60b 100644
>>> --- a/kernel/audit.c
>>> +++ b/kernel/audit.c
>>> @@ -2226,6 +2226,53 @@ static void audit_buffer_aux_end(struct audit_buffer *ab)
>>> ab->skb = skb_peek(&ab->skb_list);
>>> }
>>>
>>> +void audit_log_object_context(struct audit_buffer *ab, struct lsmblob *blob)
>>> +{
>>> + int i;
>>> + int error;
>>> + struct lsmcontext context;
>>> +
>>> + if (!lsm_multiple_contexts()) {
>>> + error = security_secid_to_secctx(blob, &context, LSMBLOB_FIRST);
>>> + if (error) {
>>> + if (error != -EINVAL)
>>> + goto error_path;
>>> + return;
>>> + }
>>> + audit_log_format(ab, " obj=%s", context.context);
>>> + security_release_secctx(&context);
>>> + } else {
>>> + audit_log_format(ab, " obj=?");
>>> + error = audit_buffer_aux_new(ab, AUDIT_MAC_OBJ_CONTEXTS);
>>> + if (error)
>>> + goto error_path;
>>> +
>>> + for (i = 0; i < LSMBLOB_ENTRIES; i++) {
>>> + if (blob->secid[i] == 0)
>>> + continue;
>>> + error = security_secid_to_secctx(blob, &context, i);
>>> + if (error) {
>>> + audit_log_format(ab, "%sobj_%s=?",
>>> + i ? " " : "",
>>> + lsm_slot_to_name(i));
>>> + if (error != -EINVAL)
>>> + audit_panic("error in audit_log_object_context");
>>> + } else {
>>> + audit_log_format(ab, "%sobj_%s=%s",
>>> + i ? " " : "",
>>> + lsm_slot_to_name(i),
>>> + context.context);
>>> + security_release_secctx(&context);
>>> + }
>>> + }
>>> +
>>> + audit_buffer_aux_end(ab);
>>> + }
>>> + return;
>>> +
>>> +error_path:
>>> + audit_panic("error in audit_log_object_context");
>>
>> This moves the audit_panic around, so certain operations are not
>> done before the call. I am currently not sure of the implications.
>
> Short version: It's okay.
>
> Longer version: The audit_panic() call is either going to panic the
> kernel (NOT the default), do a pr_err(), or essentially be a no-op.
> In the case of the full blown kernel panic we don't really care, the
> system is going to die before there is any chance of this record in
> progress getting logged. In the case of a pr_err() or no-op the key
> part is making sure we leave the audit_buffer in a consistent state so
> that we preserve whatever information is already present. In the
> !lsm_multiple_contexts case we simply return without making any
> changes to the audit_buffer so we're good there; in the multiple LSM
> case we always end the aux record properly (using a "?" when
> necessary) if an aux record has been successfully created.
>
> Feel free to point out a specific scenario that you think looks wrong
> - I may have missed it - but I believe this code to be correct.
>
mostly I am good, I was worried I was missing something since the old
code made an effort to have the call of audit_panic() at the end.
The current change does result in potential multiple calls to
audit_panic() in a single audit_log_exit(). This doesn't matter in
the case of a full blown kernel panic, but it could result in multiple
pr_err() messages where previously the code would only generate one.
It does simplify the code, and the case should be quite rare so I
am fine with the trade-off.
>>> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
>>> index 557713954a69..04bf3c04ef3d 100644
>>> --- a/kernel/auditsc.c
>>> +++ b/kernel/auditsc.c
>>> @@ -1420,18 +1409,10 @@ static void show_special(struct audit_context *context, int *call_panic)
>>
>> If pushing audit_panic into audit_log_object_context() is acceptable then this call_panic arg is
>> no longer needed. The same goes for the call_panic arg in audit_log_name(). And call_panic can
>> be dropped from audit_log_exit()
>
> Good catch.
>
> I suspect this is a vestige from when audit_log_end() used to do the
> record's skb write to userspace, meaning it was possible that you
> might get some of the records written to userspace before the system
> killed itself. Now with all of the queuing involved it's less likely
> that this would be the case, and even if it does happen in some cases,
> it's basically a toss up depending on how the system is loaded, the
> scheduler, etc.
>
