[PATCH v9 00/11] landlock: truncate support

[Paul Moore]

On Tue, Oct 18, 2022 at 2:06 PM Günther Noack <gnoack3000 at gmail.com> wrote:
>
> On Tue, Oct 18, 2022 at 01:12:48PM -0400, Paul Moore wrote:
> > On Mon, Oct 17, 2022 at 5:16 AM Mickaël Salaün <mic at digikod.net> wrote:
> > > On 16/10/2022 22:07, Günther Noack wrote:
> >
> > ...
> >
> > > > Proposed fix
> > > > ------------
> > > >
> > > > I think the LSM framework should ensure that security blobs are
> > > > pointer-aligned.
> > > >
> > > > The LSM framework takes the role of a memory allocator here, and
> > > > memory allocators should normally return aligned addresses, in my
> > > > understanding. -- It seems reasonable for AppArmor to make that
> > > > assumption.
> > > >
> > > > The proposed one-line fix is: Change lsm_set_blob_size() in
> > > > security/security.c, where the positions of the individual security
> > > > blobs are calculated, so that each allocated blob is aligned to a
> > > > pointer size boundary.
> > > >
> > > > if (*need > 0) {
> > > >    *lbs = ALIGN(*lbs, sizeof(void *));   // NEW
> > > >
> > > >    offset = *lbs;
> > > >    *lbs += *need;
> > > >    *need = offset;
> > > > }
> > >
> > > This looks good to me. This fix should be part of patch 4/11 since it
> > > only affects Landlock for now.
> >
> > Hi Günther,
> >
> > Sorry for not seeing this email sooner; I had thought the landlock
> > truncate work was largely resolved with just a few small things for
> > you to sort out with Mickaël so I wasn't following this thread very
> > closely anymore.
> >
> > Regarding the fix, yes, I think the solution is to fixup the LSM
> > security blob allocator to properly align the entries.  As you already
> > mentioned, that's common behavior elsewhere and I see no reason why we
> > should deviate from that in the LSM allocator.  Honestly, looking at
> > the rest of the allocator right now I can see a few other things to
> > improve, but those can wait for a later time so as to not conflict
> > with this work (/me adds a new entry to my todo list).
> >
> > Other than that, I might suggest the lsm_set_blob_size()
> > implementation below as it seems cleaner to me and should be
> > functionally equivalent ... at least on quick inspection, if I've done
> > something dumb with the code below please feel free to ignore me ;)
> >
> >   static void __init lsm_set_blob_size(int *need, int *lbs)
> >   {
> >     if (*need <= 0)
> >       return;
> >
> >     *need = ALIGN(*need, sizeof(void *));
> >     *lbs += *need;
> >   }
>
> Hello Paul,
>
> thanks for the reply. Sounds good, I'll go forward with this approach
> then and send a V10 soon.
>
> Implementation-wise for this function, I think this is the closest to
> your suggestion I can get:
>
> static void __init lsm_set_blob_size(int *need, int *lbs)
> {
>   int offset;
>
>   if (*need <= 0)
>     return;
>
>   offset = ALIGN(*lbs, sizeof(void *));
>   *lbs = offset + *need;
>   *need = offset;
> }
>
> This differs from your suggestion in that:
>
> - *need gets assigned to the offset at the end. (It's a bit unusual:
>   *need is both used to specify the requested blob size when calling
>   the function, and for returning the allocated offset from the
>   function call.)
>
> - This implementation aligns the blob's start offset, not the end
>   offset. (probably makes no real difference in practice)
>
> As suggested by Mickaël, I'll make this fix part of the "Landlock:
> Support file truncation" patch, so that people backporting it won't
> accidentally leave it out.

That's all fine with me, the important thing is to make sure the
landlock truncate patches don't break anything.  We'll cleanup the
allocator later.

-- 
paul-moore.com