On Tue, Oct 18, 2022 at 2:06 PM Günther Noack <gnoack3000@xxxxxxxxx> 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@xxxxxxxxxxx> 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
