On Mon, 2022-11-21 at 14:29 +0100, Roberto Sassu wrote:

On Fri, 2022-11-18 at 09:31 -0800, Casey Schaufler wrote:

quoted

On 11/18/2022 7:10 AM, Mimi Zohar wrote:

quoted

On Fri, 2022-11-18 at 10:14 +0100, Roberto Sassu wrote:

quoted

quoted

quoted

+static int security_check_compact_xattrs(struct xattr *xattrs,
+                                     int num_xattrs, int *checked_xattrs)

Perhaps the variable naming is off, making it difficult to read.   So
although this is a static function, which normally doesn't require a
comment, it's definitely needs one.

Ok, will improve it.

quoted

quoted

+{
+    int i;
+
+    for (i = *checked_xattrs; i < num_xattrs; i++) {

If the number of "checked" xattrs was kept up to date, removing the
empty xattr gaps wouldn't require a loop.  Is the purpose of this loop
to support multiple per LSM xattrs?

An LSM might reserve one or more xattrs, but not set it/them (for 
example because it is not initialized). In this case, removing the gaps 
is needed for all subsequent LSMs.

Including this sort of info in the function description or as a comment
in the code would definitely simplify review.

security_check_compact_xattrs() is called in the loop after getting
each LSM's xattr(s).  Only the current LSMs xattrs need to be
compressed, yet the loop goes to the maximum number of xattrs each
time. Just wondering if there is a way of improving it.

At security module registration each module could identify how
many xattrs it uses. That number could be used to limit the range
of the loop. I have to do similar things for the forthcoming LSM
syscalls and module stacking beyond that.

Yes, blob_sizes.lbs_xattr contains the total number of xattrs requested
by LSMs. To stop the loop earlier, at the offset of the next LSM, we
would need to search the LSM's lsm_info, using the LSM name in
the security_hook_list structure. Although it is not optimal, not doing
it makes the code simpler. I could do that, if preferred.

Either way is fine, as long as the code is readable.  At minimum add a
comment.

-- 
thanks,

Mimi