On Wed, May 01, 2013 at 02:31:09PM -0700, John Johansen wrote: A few comments in line..
> +bool aa_update_label_name(struct aa_namespace *ns, struct aa_label *label,
> + gfp_t gfp);
> +
> +int aa_profile_snprint(char *str, size_t size, struct aa_namespace *ns,
> + struct aa_profile *profile, bool mode);
> +int aa_label_snprint(char *str, size_t size, struct aa_namespace *ns,
> + struct aa_label *label, bool mode);
> +int aa_label_asprint(char **strp, struct aa_namespace *ns,
> + struct aa_label *label, bool mode, gfp_t gfp);
> +int aa_label_acntsprint(char __counted **strp, struct aa_namespace *ns,
> + struct aa_label *label, bool mode, gfp_t gfp);
> +void aa_label_audit(struct audit_buffer *ab, struct aa_namespace *ns,
> + struct aa_label *label, bool mode, gfp_t gfp);
> +void aa_label_seq_print(struct seq_file *f, struct aa_namespace *ns,
> + struct aa_label *label, bool mode, gfp_t gfp);
> +void aa_label_printk(struct aa_namespace *ns, struct aa_label *label,
> + bool mode, gfp_t gfp);
> +struct aa_label *aa_label_parse(struct aa_namespace *base, char *str,
> + gfp_t gfp);
Would you mind renaming the functions to _printf() where appropriate?
These names don't really speak to me.. (Also, are all those really
needed? :)
> struct aa_profile *aa_match_profile(struct aa_namespace *ns, const char
> *name);
>
> ssize_t aa_replace_profiles(void *udata, size_t size, bool noreplace);
> diff --git a/security/apparmor/label.c b/security/apparmor/label.c
> index 0c67a80..338b6bd 100644
> --- a/security/apparmor/label.c
> +++ b/security/apparmor/label.c
> @@ -517,6 +517,535 @@ static struct aa_label *__aa_label_insert(struct
> aa_labelset *ls,
> }
>
> /**
> + * 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?)
> + */
> +bool aa_update_label_name(struct aa_namespace *ns, struct aa_label *label,
> + gfp_t gfp)
> +{
> + struct aa_labelset *ls;
> + unsigned long flags;
> + char __counted *name;
> + bool res = false;
> +
> + AA_WARN(!ns);
> + AA_WARN(!label);
> +
> + if (label->hname || !(label->flags & FLAG_IN_TREE) ||
> + labels_ns(label) != ns)
> + return res;
> +
> + if (aa_label_acntsprint(&name, ns, label, false, gfp) == -1)
> + return res;
> +
> + ls = labels_set(label);
> + write_lock_irqsave(&ls->lock, flags);
> + if (!label->hname && label->flags & FLAG_IN_TREE) {
> + label->hname = name;
> + res = true;
> + } else
> + aa_put_str(name);
> + write_unlock_irqrestore(&ls->lock, flags);
> +
> + return res;
> +}
> +
> +/* cached label name is present and visible
> + * @label->hname only exists if label is namespace hierachical */
> +static inline bool label_name_visible(struct aa_namespace *ns,
> + struct aa_label *label)
> +{
> + if (label->hname && labels_ns(label) == ns)
> + return true;
> +
> + return false;
> +}
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..
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?
> +/* helper macro for snprint routines */
> +#define update_for_len(total, len, size, str) \
> +do { \
> + if (len < 0) \
> + return len; \
I don't think any callers that set 'len' will ever set len < 0. Or, if
they will, I didn't see it documented. It -is- good defense, I guess,
but perhaps a warning would be nice?
> + total += len; \
> + len = min(len, size); \
> + size -= len; \
> + str += len; \
> +} while (0)
> +
> +/**
> + * aa_modename_snprint - print the mode name of a profile or label to a
> 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 print the mode of (NOT NULL)
> + *
> + * Returns: size of name written or would be written if larger than
> + * available buffer
> + *
> + * Note: will print every mode name visible (mode1)(mode2)(mode3)
> + * this is likely not what is desired for most interfaces
> + * use aa_mode_snprint to get the standard mode format
> + */
> +static int aa_modename_snprint(char *str, size_t size, struct aa_namespace
> *ns,
> + struct aa_label *label)
> +{
> + struct aa_profile *profile;
> + int i, total = 0;
> + size_t len;
> +
> + label_for_each(i, label, profile) {
> + const char *modestr;
> + if (!aa_ns_visible(ns, profile->ns))
> + continue;
> + /* no mode for 'unconfined' */
> + if (profile_unconfined(profile) &&
> + profile == profile->ns->unconfined)
> + break;
> + modestr = aa_profile_mode_names[profile->mode];
> + len = snprintf(str, size, "(%s)", modestr);
> + update_for_len(total, len, size, str);
> + }
> +
> + return total;
> +}
> +
> +/**
> + * aa_modechr_snprint - print the mode chr of a profile or labels to a 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 print the mode chr of (NOT NULL)
> + *
> + * Returns: size of mode string written or would be written if larger than
> + * available buffer
> + *
> + * Note: will print the chr of every visible profile (123)
> + * this is likely not what is desired for most interfaces
> + * use aa_mode_snprint to get the standard mode format
> + */
> +static int aa_modechr_snprint(char *str, size_t size, struct aa_namespace
> *ns,
> + struct aa_label *label)
> +{
> + struct aa_profile *profile;
> + int i, total = 0;
> + size_t len;
> +
> + len = snprintf(str, size, "(");
> + update_for_len(total, len, size, str);
> + label_for_each(i, label, profile) {
> + const char *modestr;
> + if (!aa_ns_visible(ns, profile->ns))
> + continue;
> + modestr = aa_profile_mode_names[profile->mode];
> + /* just the first char of the modestr */
> + len = snprintf(str, size, "%c", *modestr);
> + update_for_len(total, len, size, str);
> + }
> + len = snprintf(str, size, ")");
> + update_for_len(total, len, size, str);
> +
> + return total;
> +}
> +
> +/**
> + * aa_mode_snprint - print the mode of a profile or label to a 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 print the mode of (NOT NULL)
> + * @count: number of label entries to be printed (<= 0 if unknown)
> + *
> + * Returns: size of name written or would be written if larger than
> + * available buffer
> + *
> + * Note: dynamically switches between mode name, and mode char format as
> + * appropriate
> + * will not print anything if the label is not visible
> + */
> +static int aa_mode_snprint(char *str, size_t size, struct aa_namespace *ns,
> + struct aa_label *label, int count)
> +{
> + struct aa_profile *profile;
> + int i;
> +
> + if (count <= 0) {
> + count = 0;
> + label_for_each(i, label, profile) {
> + if (aa_ns_visible(ns, profile->ns))
> + count++;
> + }
> + }
> +
> + if (count == 0)
> + return 0;
> +
> + if (count == 1)
> + return aa_modename_snprint(str, size, ns, label);
> +
> + return aa_modechr_snprint(str, size, ns, label);
> +}
> +
> +/**
> + * aa_snprint_profile - print a profile name to a 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)
> + * @profile: profile 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: will not print anything if the profile is not visible
> + */
> +int aa_profile_snprint(char *str, size_t size, struct aa_namespace *ns,
> + struct aa_profile *profile, bool mode)
> +{
> + const char *ns_name = aa_ns_name(ns, profile->ns);
> +
> + AA_WARN(!str && size != 0);
> + AA_WARN(!ns);
> + AA_WARN(!profile);
> +
> + if (!ns_name)
> + return 0;
> +
> + if (mode && profile != profile->ns->unconfined) {
> + const char *modestr = aa_profile_mode_names[profile->mode];
> + if (strlen(ns_name))
> + return snprintf(str, size, ":%s://%s (%s)", ns_name,
> + profile->base.hname, modestr);
> + return snprintf(str, size, "%s (%s)", profile->base.hname,
> + modestr);
> + }
> +
> + if (strlen(ns_name))
> + return snprintf(str, size, ":%s://%s", ns_name,
> + profile->base.hname);
> + return snprintf(str, size, "%s", profile->base.hname);
> +}
> +
> +/**
> + * 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?
Also "visble".
> + */
> +int aa_label_snprint(char *str, size_t size, struct aa_namespace *ns,
> + struct aa_label *label, bool mode)
> +{
> + struct aa_profile *profile;
> + int i, count = 0, total = 0;
> + size_t len;
> +
> + AA_WARN(!str && size != 0);
> + AA_WARN(!ns);
> + AA_WARN(!label);
> +
> + label_for_each(i, label, profile) {
> + if (aa_ns_visible(ns, profile->ns)) {
> + if (count > 0) {
> + len = snprintf(str, size, "//&");
> + update_for_len(total, len, size, str);
> + }
> + len = aa_profile_snprint(str, size, ns, profile, false);
> + update_for_len(total, len, size, str);
> + count++;
> + }
> + }
> +
> + if (count == 0)
> + return snprintf(str, size, aa_hidden_ns_name);
> +
> + /* count == 1 && ... is for backwards compat where the mode
> + * is not displayed for 'unconfined' in the current ns
> + */
> + if (mode &&
> + !(count == 1 && labels_ns(label) == ns &&
> + labels_profile(label) == ns->unconfined)) {
> + len = snprintf(str, size, " ");
> + update_for_len(total, len, size, str);
> + len = aa_mode_snprint(str, size, ns, label, count);
> + update_for_len(total, len, size, str);
> + }
> +
> + return total;
> +}
> +#undef update_for_len
> +
> +/**
> + * aa_label_asprint - allocate a string buffer and print label into it
> + * @strp: buffer to write to. (MAY BE NULL if @size == 0)
@size isn't a parameter in this function.. strp is allocated here,
that'd be nice to report too.
> + * @ns: namespace profile is being viewed from (NOT NULL)
> + * @label: label to view (NOT NULL)
> + * @mode: whether to include the mode string
> + * @gfp: kernel memory allocation type
> + *
> + * Returns: size of name written or would be written if larger than
> + * available buffer
> + */
> +int aa_label_asprint(char **strp, struct aa_namespace *ns,
> + struct aa_label *label, bool mode, gfp_t gfp)
> +{
> + int size;
> +
> + AA_WARN(!strp);
> + AA_WARN(!ns);
> + AA_WARN(!label);
> +
> + size = aa_label_snprint(NULL, 0, ns, label, mode);
> + if (size < 0)
> + return size;
> +
> + *strp = kmalloc(size + 1, gfp);
> + if (!*strp)
> + return -ENOMEM;
> + return aa_label_snprint(*strp, size + 1, ns, label, mode);
> +}
> +
> +/**
> + * aa_label_acntsprint - allocate a __counted string buffer and print label
> + * @strp: buffer to write to. (MAY BE NULL if @size == 0)
> + * @ns: namespace profile is being viewed from (NOT NULL)
> + * @label: label to view (NOT NULL)
> + * @mode: whether to include the mode string
> + * @gfp: kernel memory allocation type
> + *
> + * Returns: size of name written or would be written if larger than
> + * available buffer
> + */
> +int aa_label_acntsprint(char __counted **strp, struct aa_namespace *ns,
> + struct aa_label *label, bool mode, gfp_t gfp)
> +{
> + int size;
> +
> + AA_WARN(!strp);
> + AA_WARN(!ns);
> + AA_WARN(!label);
> +
> + size = aa_label_snprint(NULL, 0, ns, label, mode);
> + if (size < 0)
> + return size;
> +
> + *strp = aa_str_alloc(size + 1, gfp);
> + if (!*strp)
> + return -ENOMEM;
> + return aa_label_snprint(*strp, size + 1, ns, label, mode);
> +}
> +
> +
> +void aa_label_audit(struct audit_buffer *ab, struct aa_namespace *ns,
> + struct aa_label *label, bool mode, gfp_t gfp)
> +{
> + const char *str;
> + char *name = NULL;
> + int len;
> +
> + AA_WARN(!ab);
> + AA_WARN(!ns);
> + AA_WARN(!label);
> +
> + if (label_name_visible(ns, label)) {
> + str = (char *) label->hname;
> + len = strlen(str);
> + } else {
> + labelstats_inc(audit_name_alloc);
> + len = aa_label_asprint(&name, ns, label, mode, gfp);
> + if (len == -1) {
> + labelstats_inc(audit_name_fail);
> + printk("<label error>");
printk is probably the wrong choice here.
> + return;
> + }
> + str = name;
> + }
> +
> + if (audit_string_contains_control(str, len))
> + audit_log_n_hex(ab, str, len);
> + else
> + audit_log_n_string(ab, str, len);
> +
> + kfree(name);
> +}
> +
> +void aa_label_seq_print(struct seq_file *f, struct aa_namespace *ns,
> + struct aa_label *label, bool mode, gfp_t gfp)
> +{
> + AA_WARN(!f);
> + AA_WARN(!ns);
> + AA_WARN(!label);
> +
> + if (!label_name_visible(ns, label)) {
> + char *str;
> + int len;
> +
> + labelstats_inc(seq_print_name_alloc);
> + len = aa_label_asprint(&str, ns, label, mode, gfp);
> + if (len == -1) {
> + labelstats_inc(seq_print_name_fail);
> + printk("<label error>");
printk is probably wrong here, as well.
> + return;
> + }
> + seq_printf(f, "%s", str);
> + kfree(str);
> + } else
> + seq_printf(f, "%s", label->hname);
> +}
> +
> +void aa_label_printk(struct aa_namespace *ns, struct aa_label *label, bool
> mode,
> + gfp_t gfp)
> +{
> + char *str;
> + int len;
> +
> + AA_WARN(!ns);
> + AA_WARN(!label);
> +
> + 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.
> + return;
> + }
> + printk("%s", str);
> + kfree(str);
> + } else
> + printk("%s", label->hname);
> +}
> +
> +
> +static int label_count_str_entries(const char *str)
> +{
> + const char *split;
> + int count = 1;
> +
> + AA_WARN(!str);
> +
> + for (split = strstr(str, "//&"); split; split = strstr(str, "//&")) {
> + count++;
> + str = split + 3;
> + }
> +
> + return count;
> +}
> +
> +/**
> + * aa_sort_and_merge_profiles - canonical sort and merge a list of profiles
> + * @n: number of refcounted profiles in the list (@n > 0)
> + * @ps: list of profiles to sort and merge
> + *
> + * Returns: the number of duplicates eliminated == references put
> + */
> +static int aa_sort_and_merge_profiles(int n, struct aa_profile **ps)
> +{
> + int i, dups = 0;
> +
> + AA_WARN(n < 1);
> + AA_WARN(!ps);
> +
> + /* label lists are usually small so just use insertion sort */
> + for (i = 1; i < n; i++) {
> + struct aa_profile *tmp = ps[i];
> + int pos, j;
> +
> + for (pos = i - 1 - dups; pos >= 0; pos--) {
> + int res = profile_cmp(ps[pos], tmp);
> + if (res == 0) {
> + aa_put_profile(tmp);
> + dups++;
> + continue;
> + } else if (res < 0)
> + break;
> + }
> + pos++;
> +
> + for (j = i - dups; j > pos; j--)
> + ps[j] = ps[j - 1];
> +
> + ps[pos] = tmp;
> + }
> +
> + return dups;
> +}
I'm a touch surprised the need for merging would arise; is it due to
rename replace loads?
> +
> +/**
> + * aa_label_parse - parse, validate and convert a text string to a label
> + * @base: base namespace to use for lookups (NOT NULL)
> + * @str: null terminated text string (NOT NULL)
> + * @gfp: allocation type
> + *
> + * Returns: the matching refcounted label if present
> + * else ERRPTR
> + */
> +struct aa_label *aa_label_parse(struct aa_namespace *base, char *str, gfp_t
> gfp)
> +{
> + struct aa_profile *profile;
> + struct aa_label *l, *label;
> + int i, len, unconf;
> + char *split;
> +
> + AA_WARN(!base);
> + AA_WARN(!str);
> +
> + len = label_count_str_entries(str);
> + label = aa_label_alloc(len, gfp);
> + if (!label)
> + return ERR_PTR(-ENOMEM);
> +
> + for (split = strstr(str, "//&"), i = 0; split && i < len; i++) {
> + *split = 0;
> + label->ent[i] = aa_fqlookupn_profile(base, str, split - str);
> + if (!label->ent[i])
> + goto fail;
> + str = split + 3;
> + split = strstr(str, "//&");
> + }
> + label->ent[i] = aa_fqlookupn_profile(base, str, strlen(str));
> + if (!label->ent[i])
> + goto fail;
> +
> + i = aa_sort_and_merge_profiles(len, &label->ent[0]);
> + label->size -= i;
> +
> + unconf = 1;
> + label_for_each(i, label, profile) {
> + if (!profile_unconfined(profile)) {
> + unconf = 0;
> + break;
> + }
> + }
> +
> + if (unconf)
> + label->flags = FLAG_UNCONFINED;
> +
> + l = aa_label_find(labels_set(label), label);
> + aa_put_label(label);
> + return l;
> +
> +fail:
> + aa_label_free(label);
> + return ERR_PTR(-ENOENT);
> +}
> +/**
> * aa_label_insert - insert label @l into @ls or return existing label
> * @ls - labelset to insert @l into
> * @l - label to insert
> diff --git a/security/apparmor/policy.c b/security/apparmor/policy.c
> index d77a5c5..331bd02 100644
> --- a/security/apparmor/policy.c
> +++ b/security/apparmor/policy.c
> @@ -93,6 +93,9 @@
> /* root profile namespace */
> struct aa_namespace *root_ns;
>
> +/* Note: mode names must be unique in the first character because of
> + * modechrs used to print modes on compound labels on some interfaces
> + */
> const char *const aa_profile_mode_names[] = {
> "enforce",
> "complain",
> @@ -897,6 +900,25 @@ struct aa_profile *aa_lookup_profile(struct aa_namespace
> *ns, const char *hname)
> return aa_lookupn_profile(ns, hname, strlen(hname));
> }
>
> +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?
> /**
> * replacement_allowed - test to see if replacement is allowed
> diff --git a/security/apparmor/procattr.c b/security/apparmor/procattr.c
> index 82666b0..515c5e2 100644
> --- a/security/apparmor/procattr.c
> +++ b/security/apparmor/procattr.c
> @@ -35,50 +35,27 @@
> */
> int aa_getprocattr(struct aa_label *label, char **string)
> {
> struct aa_namespace *ns = labels_ns(label);
> struct aa_namespace *current_ns = labels_ns(__aa_current_label());
> + int len;
>
> + if (!aa_ns_visible(current_ns, ns))
> + return -EACCES;
There's some funny indenting here.. maybe it's just funny because I
deleted all the - lines to read this here...
> + len = aa_label_snprint(NULL, 0, current_ns, label, true);
> + if (len < 0)
> + return len;
Again, I don't think this condition will ever be true; I think it'd be
worth throwing in a warning.
> + *string = kmalloc(len + 2, GFP_KERNEL);
> + if (!*string)
> return -ENOMEM;
>
> + len = aa_label_snprint(*string, len + 2, current_ns, label, true);
> + if (len < 0)
> + return len;
> + (*string)[len] = '\n';
> + (*string)[len + 1] = 0;
> +
> + return len + 1;
> }
Thanks John
signature.asc
Description: Digital signature
-- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
