[apparmor] [PATCH 24/36] apparmor: add abilitiy to print labels and update interface to use

[Seth Arnold]

On Wed, May 29, 2013 at 09:23:52AM -0700, John Johansen wrote:
> > Would you mind renaming the functions to _printf() where appropriate?
> > These names don't really speak to me.. (Also, are all those really
> > needed? :)
> > 
> yes I would, because its not appropriate, non of them take a format string
> they just take and print the label. The naming is similar because they
> closely mimic the printf fns but without the fmt string.

Yeah, I realized this after going to bed. Sigh. :) Thanks.

> I didn't have to make the base routines visible but I saw no reason not to

Well, okay, if they are also useful for debugging, that's something.

> >>  /**
> >> + * aa_update_label_name - update a label to have a stored name
> >> + * @ns: ns being viewed from (NOT NULL)
> >> + * @label: label to update (NOT NULL)
> >> + * @gfp: type of memory allocation
> >> + *
> >> + * Requires: labels_set(label) not locked in caller
> >> + *
> >> + * note: only updates the label name if it does not have a name already
> >> + *       and if it is in the labelset
> > 
> > Why exactly the constraint on labelset? (Is that 1:1 with FLAG_IN_TREE?)
> > 
> right FLAG_IN_TREE should only be set if the label is in a labelset

Thanks. But why does the code need this constraint?

> > This doesn't feel hierarchical enough, if you know what I mean:
> > it only checks the namespace of the last profile on the label,
> > rather than checking all the labels..
> > 
> thats because we order profiles within a label by namespace hierarchy
> so the last label will always be the bottom namespace

Oh! That's handy. I didn't spot that on the merge and sort function. :)

> > It also looks like it intends to enforce visibility only within a single
> > specific ns -- not at all hierarchical. (No visibility to parents _or_
> > children.) Right?
> > 
> right, but this is only for the cached name. Otherwise its either not
> visible or needs ns info prepended

Ah! Makes sense. Thanks.

> >> +/**
> >> + * aa_label_snprint - print a label name to a string buffer
> >> + * @str: buffer to write to. (MAY BE NULL if @size == 0)
> >> + * @size: size of buffer
> >> + * @ns: namespace profile is being viewed from (NOT NULL)
> >> + * @label: label to view (NOT NULL)
> >> + * @mode: whether to include the mode string
> >> + *
> >> + * Returns: size of name written or would be written if larger than
> >> + *          available buffer
> >> + *
> >> + * Note: labels do not have to be strictly hierarchical to the ns as
> >> + *       objects may be shared across different namespaces and thus
> >> + *       pickup labeling from each ns.  If a particular part of the
> >> + *       label is not visible it will just be excluded.  And if none
> >> + *       of the label is visble "---" will be used.
> > 
> > Where's the "---" bit implemented?
> > 
> not visible so its masked. Its what we currently use

Ah, dunno how I missed it the first go-around:

const char *aa_hidden_ns_name = "---";

> >> +
> >> +	if (!label_name_visible(ns, label)) {
> >> +		labelstats_inc(printk_name_alloc);
> >> +		len = aa_label_asprint(&str, ns, label, mode, gfp);
> >> +		if (len == -1) {
> >> +			labelstats_inc(printk_name_fail);
> >> +			printk("<label error>");
> > 
> > Here we go :) though it does read oddly. I'd kind of rather see
> > something like:
> > 
> >     printk("%s", foo ? str : "<label error>");
> > 
> > but maybe that'd be too ugly to work into the flow.
> > 
> err why? that is a static string as this is the error path and str is
> never valid.
> 
> I agree that it should be moved from printk to AA_DEBUG

Well, in this case, you're _trying_ to printk() the label, so <label
error>" as output is fine enough. (Note to future: a profile named
"<label error>" might be confusing. :)

I just thought it odd that the error case was handled so differently
from the normal case. Doesn't _really_ matter...

> > I'm a touch surprised the need for merging would arise; is it due to
> > rename replace loads?
> > 
> merging is used all over the place. In stacking, when merging a label into
> a cache to create a new label etc. Anyways the specific case here, is we
> do not trust profile/label names from userspace. Who knows what are in
> them.
> 
> We lookup each individual component, then we sort them (labels in the
> kernel are always sorted, its an optimization), and merge out any
> duplicates. Sure its slow but its input from userspace, for some reason
> I just don't trust it.

Thanks for the explanation. :)

> >> +struct aa_profile *aa_fqlookupn_profile(struct aa_namespace *base, char *fqname,
> >> +					int n)
> >> +{
> >> +	struct aa_profile *profile;
> >> +	struct aa_namespace *ns;
> >> +	char *name, *ns_name;
> >> +
> >> +	name = aa_split_fqname(fqname, &ns_name);
> >> +	if (ns_name) {
> >> +		ns = aa_find_namespace(base, ns_name);
> >> +		if (!ns)
> >> +			return NULL;
> >> +	} else
> >> +		ns = aa_get_namespace(base);
> >> +	profile = aa_lookupn_profile(ns, name, n - (name - fqname));
> >> +	aa_put_namespace(ns);
> >> +
> >> +       return profile;
> >> +}
> > 
> > If 'n' points somewhere within the namespace rather than the profile,
> > how's this thing handle it? Could we enforce sane values for 'n' here?
> > 
> It doesn't return the correct value. We could add a debug test to check
> that n is on a // or \0, but nothing beyond that

That sounds nice :)


Thanks John
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 490 bytes
Desc: Digital signature
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20130529/b1161d26/attachment-0001.pgp>