Re: [PATCH 0/5] selinux: add prefix/suffix matching to filename type transitions

[Juraj Marcin <juraj@jurajmarcin.com>]

On 2023-06-19 17:53, Paul Moore wrote:
> On Sun, Jun 18, 2023 at 5:41 AM Juraj Marcin <juraj@jurajmarcin.com> wrote:
> >
> > On 2023-06-15 22:04, Paul Moore wrote:
> > > On Thu, Jun 1, 2023 at 1:03 PM Juraj Marcin <juraj@jurajmarcin.com> wrote:
> > > > On 2023-05-31 18:24, Paul Moore wrote:
> > > > > On Wed, May 31, 2023 at 7:32 AM Juraj Marcin <juraj@jurajmarcin.com> wrote:
> > > > > >
> > > > > > Currently, filename transitions are stored separately from other type
> > > > > > enforcement rules and only support exact name matching. However, in
> > > > > > practice, the names contain variable parts. This leads to many
> > > > > > duplicated rules in the policy that differ only in the part of the name,
> > > > > > or it is even impossible to cover all possible combinations.
> > > > > >
> > > > > > First, this series of patches moves the filename transitions to be part
> > > > > > of the avtab structures. This not only makes the implementation of
> > > > > > prefix/suffix matching and future enhancements easier, but also reduces
> > > > > > the technical debt regarding the filename transitions. Next, the last
> > > > > > patch implements the support for prefix/suffix name matching itself by
> > > > > > extending the structures added in previous patches in this series.
> > > > > >
> > > > > > Even though, moving everything to avtab increases the memory usage and
> > > > > > the size of the binary policy itself and thus the loading time, the
> > > > > > ability to match the prefix or suffix of the name will reduce the
> > > > > > overall number of rules in the policy which should mitigate this issue.
> > > > > >
> > > > > > This implementation has been successfully tested using the existing and
> > > > > > also new tests in the SELinux Testsuite.
> > > > > >
> > > > > > Juraj Marcin (5):
> > > > > >   selinux: move transition to separate structure in avtab_datum
> > > > > >   selinux: move filename transitions to avtab
> > > > > >   selinux: implement new binary format for filename transitions in avtab
> > > > > >   selinux: filename transitions move tests
> > > > > >   selinux: add prefix/suffix matching support to filename type
> > > > > >     transitions
> > > > >
> > > > > Just a quick comment as I haven't had a chance to properly review this
> > > > > series yet; you show some memory usage and performance measurements in
> > > > > some of the intermediate patches, that's good, but I don't see the
> > > > > same measurements taken when the full patchset is applied.  Please
> > > > > provide the same memory usage and performance comparisons with the
> > > > > full patchset applied.
> > > >
> > > > Of course, here are the measurements with the whole patchset applied.
> > > >
> > > > I also included measurements with new policy (based on the Fedora
> > > > policy) that uses prefix filename transitions where possible. This new
> > > > policy has been generated by merging existing filename transitions into
> > > > prefix ones if it would reduce the number of transitions overall while
> > > > keeping the resulting type same.
> > > >
> > > > [1] Reference kernel (c52df19e3759), Fedora policy (format v33)
> > > > [2] This patchset, Fedora policy (format v33)
> > > > [3] This patchset, Fedora policy without prefix/suffix rules (format v35)
> > > > [4] This patchset, Fedora policy with prefix rules (format v35)
> > > >
> > > >
> > > >  Test | Mem   | Binary | Policy | Create tty      | osbench
> > > >       | Usage | policy | load   |                 | create
> > > >       |       | size   | time   | (ms/file)       | files
> > > >       | (MiB) | (MiB)  | (ms)   | real   | kernel | (us/file)
> > > > ------+-------+--------+--------+--------+--------+-----------
> > > >  [1]  |   157 |    3.4 |     78 | 1.1021 | 0.7586 | 7.8277
> > > >  [2]  |   200 |    3.4 |    206 | 1.1193 | 0.7724 | 8.2711
> > > >  [3]  |   169 |    5.8 |    106 | 1.1021 | 0.7724 | 8.0304
> > > >  [4]  |   164 |    3.8 |     86 | 1.1029 | 0.7586 | 7.9609
> > >
> > > Thanks for performing those measurements.
> > >
> > > I apologize that I haven't had an opportunity to review your patcheset
> > > in detail just yet (I've been struggling with some self-inflicted
> > > networking issues this week), but looking strictly at the numbers
> > > above it appears that by every metric in the table this patchset
> > > results in a policy that is larger (both on-disk and in-memory) as
> > > well as performance that is at best the same (although in most cases,
> > > it is worse).  Are there any improvements expected beyond test
> > > configuration [4] (above)?
> >
> > The main goal of this patchset is to bring the possibility to use prefix
> > or suffix matching in filename transitions, as now it is not possible to
> > cover files that have fixed prefix and variable part after it. For
> > example devices in /dev (the policy now enumerates all possible number
> > suffixes) or files with random suffix/prefix (not possible at all).
> >
> > The next goal is to make future improvements easier, for example
> > supporting filename transitions in conditional policies or inherent
> > support for type attributes.
> >
> > As for performance, the goal is to implement previous goals while not
> > killing the performance by them. Christian suggested some possible
> > optimizations [1], but after trying them out [2] they either not provide
> > much measurable difference or the difference is small and the
> > implementation hacky.
> 
> I understand performance improvements were not the main motivation
> behind this patchset, but I'm somewhat concerned that policy load time
> *almost* triples in the case of an unmodified policy with this patch
> applied.  Since that will be most everyone as soon as this patch is
> applied, that regression does concern me ... I'm not sure just yet how
> much it concerns me, but it isn't trivial.

I also understand your concern. That higher load time (and also memory
usage) is the cost of doing the conversion from the older binary policy
format in the kernel during load.

However, to reduce both load time and memory usage to the values in the
third test, the only action needed is recompiling the policy with newer
checkpolicy/libsepol, patches to which I also proposed.

-- 
Juraj Marcin