On Fri, Nov 18, 2022 at 2:09 PM Mimi Zohar [off-list ref] wrote:

On Fri, 2022-11-18 at 13:44 -0500, Paul Moore wrote:

quoted

On Fri, Nov 18, 2022 at 1:30 PM Mimi Zohar [off-list ref] wrote:

quoted

On Fri, 2022-11-18 at 08:44 -0500, Paul Moore wrote:

quoted

On Thu, Nov 17, 2022 at 8:54 PM Serge E. Hallyn [off-list ref] wrote:

quoted

On Wed, Nov 09, 2022 at 11:36:14PM -0500, Paul Moore wrote:

quoted

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.

quoted

quoted

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.

quoted

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