Re: [PATCH v9 00/11] landlock: truncate support

["Günther Noack" <gnoack3000@gmail.com>]

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@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.

—Günther

--