On Wed, Sep 7, 2022 at 7:53 PM Casey Schaufler <casey@xxxxxxxxxxxxxxxx> wrote:
> On 9/7/2022 4:27 PM, Paul Moore wrote:
...
> > I
> > just want an interface that is clearly defined, has reasonable
> > capacity to be extended in the future as needed, and is easy(ish) to
> > use and support over extended periods of time (both from a kernel and
> > userspace perspective).
> >
> > The "smack_label\0apparmor_label\0selinux_label\0future_lsm_label\0"
> > string interface is none of these things.
>
> In this we disagree ....
I think we can both agree that there is a subjective aspect to this
argument and it may be that we never reach agreement on the "best"
approach, if there even is such a thing. What I am trying to do here
is describe a path that would allow me to be more comfortable merging
the LSM stacking functionality; I don't think you've had a clearly
defined path, of any sort, towards getting these patches merged, which
is a problem and I suspect the source of some of the frustration. My
comments, as objectionable as you may find them to be, are intended to
help you move forward with these patches.
> > It is not clearly defined
> > as it requires other interfaces to associate the labels with the
> > correct LSMs.
>
> The label follows the lsm name directly. What other association is required?
You need to know the order of the LSMs in order to
interpret/de-serialize the string.
> > The ease-of-use quality is a bit subjective, but it does need
> > another interface to use properly and it requires string parsing which
> > history has shown to be an issue time and time again (although it is
> > relatively simple here).
>
> There was a lot of discussion regarding that. My proposed
> apparmor="unconfined",smack="User" format was panned for those same reasons.
> The nil byte format has been used elsewhere and suggested for that reason.
Based on what I recall from those discussions, it was my impression
the nil byte label delimiter was suggested simply because no one was
entertaining the idea of using something other than the existing
procfs interface. It is my opinion that we've taken that interface
about as far as it can go, and while it needs to stay intact for
compatibility reasons, the LSM stacking functionality should move to a
different API that is better suited for it.
> > Once again, the syscall example I tossed out was a quick strawman, but
> > it had clearly separated and defined labels conveyed in data
> > structures that didn't require string parsing to find the label
> > associated with an LSM.
>
> True, but it uses pointers internally and you can't do that in memory
> you're sending up from the system. What comes from the syscall has to
> look something like the nil byte format. The strawman would have to do
> the same sort of parsing in userspace that you are objecting to.
Then we change the strawman. That's pretty much the definition of a
strawman example, it's something you toss out as starting point for
discussion and future improvements. If it was something much more
fully developed I would have submitted a patch .... sheesh.
In case it helps spur your imagination, here is a revised strawman:
/**
* struct lsm_ctx - LSM context
* @id: the LSM id number, see LSM_ID_XXX
* @flags: LSM specific flags, zero if unused
* @ctx_len: the size of @ctx
* @ctx: the LSM context
*/
struct lsm_ctx {
unsigned in id;
unsigned int flags;
size_t ctx_len;
unsigned char ctx[];
};
/**
* lsm_current_ctx - Return current tasks's LSM context
* @ctx: the LSM contexts
* @size: the size of @ctx, updated on return
* @flags: reserved for future use, must be zero
*
* Returns the calling task's LSM contexts. On success this
* function returns a positive number representing the number of
* @ctx array elements, which may be zero if there are no LSM
* contexts assigned to the caller. If the size of @ctx is
* insufficient, -E2BIG is returned and the minimum required
* size is returned via @count. In all other failure cases, a
* negative value indicating the error is returned.
*/
int lsm_current_ctx(struct lsm_ctx *ctx, size_t *size,
unsigned int flags);
> > It was also self-contained in that no other
> > interface was needed to interpret the results of the syscall (well,
> > beyond the header file definitions I guess). Finally, it made use of
> > flags and "reserved for future use" token values to allow for
> > additional data to be conveyed in the future.
>
> Can you describe potential flags or additional data? Planning for the future
> is a good idea, but throwing fields on "just in case" seems contrary to
> what I'm used to hearing from you.
Adding flags to a syscall is a fairly common practice. If you're
making the mistake of confusing this with the discussion that is
ongoing audit/fanotify discussion, you can think of the flags as
similar to adding a type/length field to a struct (which I support).
--
paul-moore.com
