Al Viro <v...@zeniv.linux.org.uk> writes:
> On Tue, Jan 05, 2021 at 04:50:05PM +0000, Al Viro wrote:
>
>> LSM_AUDIT_DATA_DENTRY is easy to handle - wrap
>> audit_log_untrustedstring(ab, a->u.dentry->d_name.name);
>> into grabbing/dropping a->u.dentry->d_lock and we are done.
>
> Incidentally, LSM_AUDIT_DATA_DENTRY in mainline is *not* safe wrt
> rename() - for long-named dentries it is possible to get preempted
> in the middle of
> audit_log_untrustedstring(ab, a->u.dentry->d_name.name);
> and have the bugger renamed, with old name ending up freed. The
> same goes for LSM_AUDIT_DATA_INODE...
In the case of proc_pid_permission(), this preemption doesn't seem
possible. We have task_lock() (a spinlock) held by ptrace_may_access()
during this call, so preemption should be disabled:
proc_pid_permission()
has_pid_permissions()
ptrace_may_access()
task_lock()
__ptrace_may_access()
| security_ptrace_access_check()
| ptrace_access_check -> selinux_ptrace_access_check()
| avc_has_perm()
| avc_audit() // note that has_pid_permissions() didn't get a
| // flags field to propagate, so flags will not
| // contain MAY_NOT_BLOCK
| slow_avc_audit()
| common_lsm_audit()
| dump_common_audit_data()
task_unlock()
I understand the issue of d_name.name being freed across a preemption is
more general than proc_pid_permission() (as other callers may have
preemption enabled). However, it seems like there's another issue here.
avc_audit() seems to imply that slow_avc_audit() would sleep:
static inline int avc_audit(struct selinux_state *state,
u32 ssid, u32 tsid,
u16 tclass, u32 requested,
struct av_decision *avd,
int result,
struct common_audit_data *a,
int flags)
{
u32 audited, denied;
audited = avc_audit_required(requested, avd, result, 0, &denied);
if (likely(!audited))
return 0;
/* fall back to ref-walk if we have to generate audit */
if (flags & MAY_NOT_BLOCK)
return -ECHILD;
return slow_avc_audit(state, ssid, tsid, tclass,
requested, audited, denied, result,
a);
}
If there are other cases in here where we might sleep, it would be a
problem to sleep with the task lock held, correct?
