On Mon, Oct 17, 2022 at 5:16 AM Mickaël Salaün [off-list ref] wrote:

On 16/10/2022 22:07, Günther Noack wrote:

...

quoted

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;
  }

-- 
paul-moore.com