Re: [RFC PATCH] lsm,fs: fix vfs_getxattr_alloc() return type and caller error paths

[Paul Moore <paul@paul-moore.com>]

On Fri, Nov 18, 2022 at 2:09 PM Mimi Zohar <zohar@linux.ibm.com> wrote:
> On Fri, 2022-11-18 at 13:44 -0500, Paul Moore wrote:
> > On Fri, Nov 18, 2022 at 1:30 PM Mimi Zohar <zohar@linux.ibm.com> wrote:
> > > On Fri, 2022-11-18 at 08:44 -0500, Paul Moore wrote:
> > > > On Thu, Nov 17, 2022 at 8:54 PM Serge E. Hallyn <serge@hallyn.com> wrote:
> > > > > On Wed, Nov 09, 2022 at 11:36:14PM -0500, Paul Moore wrote:
> > > > > > The vfs_getxattr_alloc() function currently returns a ssize_t value
> > > > > > despite the fact that it only uses int values internally for return
> > > > > > values.  Fix this by converting vfs_getxattr_alloc() to return an
> > > > > > int type and adjust the callers as necessary.  As part of these
> > > > > > caller modifications, some of the callers are fixed to properly free
> > > > > > the xattr value buffer on both success and failure to ensure that
> > > > > > memory is not leaked in the failure case.
> > > >
> > > > > > Signed-off-by: Paul Moore <paul@paul-moore.com>
> > > > >
> > > > > Reviewed-by: Serge Hallyn <serge@hallyn.com>
> > > > >
> > > > > Do I understand right that the change to process_measurement()
> > > > > will avoid an unnecessary call to krealloc() if the xattr has
> > > > > not changed size between the two calls to ima_read_xattr()?
> > > > > If something more than that is going on there, it might be
> > > > > worth pointing out in the commit message.
> > > >
> > > > Yes, that was the intent, trying to avoid extra calls to krealloc().
> > > >
> > > > Mimi, have you had a chance to look at this patch yet?  In addition to
> > > > cleaning up the vfs_getxattr_alloc() function it resolves some issues
> > > > with IMA (memory leaks), but as you're the IMA expert I really need
> > > > your review on this ...b
> > >
> > > All the other vfs_{get/set/list}xattr functions return ssize_t.  Why
> > > should vfs_getxattr_alloc() be any different?
> >
> > The xattr_handler::get() function, the main engine behind
> > vfs_getxattr_alloc() and the source of the non-error return values,
> > returns an int.  The error return values returned by
> > vfs_getxattr_alloc() are the usual -E* integer values.
> >
> > > The only time there could be a memory leak is when the
> > > vfs_getxattr_alloc() caller provides a buffer which isn't large enough.
> > > The one example in IMA/EVM is the call to evm_calc_hmac_or_hash(),
> > > which is freeing the memory.
> > >
> > > Perhaps I'm missing something, but from an IMA/EVM perspective, I see a
> > > style change (common exit), but not any memory leak fixes.  I'm fine
> > > with the style change.
> >
> > Picking one at random, what about the change in
> > ima_eventevmsig_init()?  The current code does not free @xattr_data on
> > error which has the potential to leak memory if vfs_getxattr_alloc()'s
> > second call to the xattr get'er function fails.  Granted, the
> > likelihood of this, if it is even possible, is an open question, but I
> > don't think that is an excuse for the callers to not do The Right
> > Thing.
>
> Oh!  This is about the 2nd handler call failing.
>
> Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>b

Merged into lsm/next, thanks all.

-- 
paul-moore.com