Re: [PATCH v2 2/2] selinux: reject a class permission count below its inherited common

[Paul Moore]

On Tue, Jul 28, 2026 at 1:42 PM Bryam Vargas <hexlabsecurity@xxxxxxxxx> wrote:
> On Tue, 28 Jul 2026 10:43:39 -0400, Stephen Smalley wrote:
> > Sashiko again made some interesting observations but not about your patch per se,
> > https://lore.kernel.org/selinux/20260728014625.C7A391F000E9@xxxxxxxxxxxxxxx/T/#u
>
> The first one reproduces. security_get_permissions() sizes the array by the
> class's permissions.nprim and fills it by value, so any index no perm datum
> claims stays NULL, and sel_make_perm_files() passes every entry to
> sel_attach_file() -> d_alloc_name() -> hashlen_string(). One u32 in the policy
> image is enough: on the policy I tested, class process has nel == nprim == 31
> and inherits no common, so raising its nprim to 39 leaves slots 31..38
> unwritten.
>
>   BUG: kernel NULL pointer dereference, address: 0000000000000000
>   RIP: 0010:hashlen_string+0xa/0xc0
>   Call Trace:
>    d_alloc_name+0x61/0xa0
>    sel_make_policy_nodes+0xb35/0x1310
>    sel_write_load+0x2a5/0x490
>
> load_unaligned_zeropad() doesn't absorb it: ex_handler_zeropad() rounds the
> address down and bails unless fault_addr == addr + sizeof(long), so a NULL
> base takes the ordinary fault.
>
> It doesn't stop at the loading task. sel_make_policy_nodes() runs under
> selinux_state.policy_mutex and the only unlock is the out_unlock label the
> oops never reaches, so every later load blocks in D state; SIGKILL doesn't
> clear it and only a reboot recovers. Under panic_on_oops it is a panic at
> step one.
>
> The second observation is real as stated: get_permissions_callback() does
> overwrite perms[value - 1], and the cleanup loop walks i < nperms freeing
> whichever pointer survived, so the first kstrdup() leaks. What makes it worth
> more than its severity is that it also defeats the obvious fix for the first
> one. Requiring the counts to add up -- nel plus the inherited nprim equals
> permissions.nprim -- doesn't close the gap, because symtab_insert() hashes
> and compares the name only, so two perms may carry the same value: the count
> still matches and one slot is still never written. The property that holds is
> that the values present are exactly {1..nprim}, onto and injective, where the
> two patches on the list establish only "into".
>
> The classes side has the same shape: get_classes_callback() also writes at
> value - 1, and sel_make_classes() passes classes[i] to sel_make_dir(). By
> inspection; I have not reproduced that one.
>
> Policy crafter and the full log on request.

Thanks for looking into this, Bryam.  Are you interested in working on
a patchset to fix these issues?  We could always use the help :)

-- 
paul-moore.com