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.
—Günther
--
