Re: [PATCH v5 3/3] Allows reading back the current IMA policy;
On Tue, 2015-11-10 at 18:01 +0200, Petko Manolov wrote:
> On 15-11-09 09:30:58, Mimi Zohar wrote:
> > On Mon, 2015-11-02 at 00:39 +0200, Petko Manolov wrote:
> >
> > > +
> > > +#ifdef CONFIG_IMA_READ_POLICY
> > > +enum {
> > > + mask_err = -1,
> > > + mask_exec = 1, mask_write, mask_read, mask_append
> > > +};
> > > +
> > > +static match_table_t mask_tokens = {
> > > + {mask_exec, "MAY_EXEC"},
> > > + {mask_write, "MAY_WRITE"},
> > > + {mask_read, "MAY_READ"},
> > > + {mask_append, "MAY_APPEND"},
> > > + {mask_err, NULL}
> > > +};
> > > +
> > > +enum {
> > > + func_err = -1,
> > > + func_file = 1, func_mmap, func_bprm,
> > > + func_module, func_firmware, func_post
> > > +};
> > > +
> > > +static match_table_t func_tokens = {
> > > + {func_file, "FILE_CHECK"},
> > > + {func_mmap, "MMAP_CHECK"},
> > > + {func_bprm, "BPRM_CHECK"},
> > > + {func_module, "MODULE_CHECK"},
> > > + {func_firmware, "FIRMWARE_CHECK"},
> > > + {func_post, "POST_SETATTR"},
> > > + {func_err, NULL}
> > > +};
> >
> > Why are we using match_table_t? Why not define an array of strings which
> > corresponds to the function hooks or use the __stringify macro?
>
> Because you used match_table_t in your code. Having too many style
> differences
> does not help it's readability.
We're using match_token() for parsing the policy, which requires
match_table_t.
> > static const char *ima_hooks_string[] = {"", "FILE_CHECK", "MMAP_CHECK",
> > "BPRM_CHECK", "MODULE_CHECK", "FIRMWARE_CHECK", "POST_SETATTR"};
> >
> > In the first case, to display the function hook string would be
> > "ima_hooks_string[func]". Using __stringify requires the hook name (eg.
> > __stringify(FILE_CHECK)).
> >
> > In either case, there would be a lot less code.
>
> It is always speed vs size. It is going to be more code or more data. It is
> a
> matter of taste which one we'll chose.
>
> If we look at ima_policy.c after the original IMA policy read patch we'll end
> up
> with a source that is littered with strings like "fowner=%s", "fowner" and
> "fowner=%d". I assumed you want this cleaned up, which i did.
>
> Another example, "FILE_CHECK" was used just once prior to introducing of
> policy
> read. Now we use it twice. Do we want to rely on GCC literals optimization
> and
> use the same string multiple times or hand-optimize it? What do we do with
> almost the same strings like "fowner=%d", "fowner=%s" and "fowner"?
>
> The answer to the above will determine whether we use an array of strings or
> __stringify. I kind of lean towards the first option but as a maintainer the
> choice is up to you.
>From what I've seen playing with __stringify, it has its own drawbacks.
I would probably use an enumeration of strings arrays and remove the
switch/case statements as much as possible.
Mimi
--
To unsubscribe from this list: send the line "unsubscribe
linux-security-module" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 3/3] Allows reading back the current IMA policy;
On 15-11-09 09:30:58, Mimi Zohar wrote:
> On Mon, 2015-11-02 at 00:39 +0200, Petko Manolov wrote:
>
> > +
> > +#ifdef CONFIG_IMA_READ_POLICY
> > +enum {
> > + mask_err = -1,
> > + mask_exec = 1, mask_write, mask_read, mask_append
> > +};
> > +
> > +static match_table_t mask_tokens = {
> > + {mask_exec, "MAY_EXEC"},
> > + {mask_write, "MAY_WRITE"},
> > + {mask_read, "MAY_READ"},
> > + {mask_append, "MAY_APPEND"},
> > + {mask_err, NULL}
> > +};
> > +
> > +enum {
> > + func_err = -1,
> > + func_file = 1, func_mmap, func_bprm,
> > + func_module, func_firmware, func_post
> > +};
> > +
> > +static match_table_t func_tokens = {
> > + {func_file, "FILE_CHECK"},
> > + {func_mmap, "MMAP_CHECK"},
> > + {func_bprm, "BPRM_CHECK"},
> > + {func_module, "MODULE_CHECK"},
> > + {func_firmware, "FIRMWARE_CHECK"},
> > + {func_post, "POST_SETATTR"},
> > + {func_err, NULL}
> > +};
>
> Why are we using match_table_t? Why not define an array of strings which
> corresponds to the function hooks or use the __stringify macro?
Because you used match_table_t in your code. Having too many style differences
does not help it's readability.
> static const char *ima_hooks_string[] = {"", "FILE_CHECK", "MMAP_CHECK",
> "BPRM_CHECK", "MODULE_CHECK", "FIRMWARE_CHECK", "POST_SETATTR"};
>
> In the first case, to display the function hook string would be
> "ima_hooks_string[func]". Using __stringify requires the hook name (eg.
> __stringify(FILE_CHECK)).
>
> In either case, there would be a lot less code.
It is always speed vs size. It is going to be more code or more data. It is a
matter of taste which one we'll chose.
If we look at ima_policy.c after the original IMA policy read patch we'll end
up
with a source that is littered with strings like "fowner=%s", "fowner" and
"fowner=%d". I assumed you want this cleaned up, which i did.
Another example, "FILE_CHECK" was used just once prior to introducing of policy
read. Now we use it twice. Do we want to rely on GCC literals optimization
and
use the same string multiple times or hand-optimize it? What do we do with
almost the same strings like "fowner=%d", "fowner=%s" and "fowner"?
The answer to the above will determine whether we use an array of strings or
__stringify. I kind of lean towards the first option but as a maintainer the
choice is up to you.
Petko
--
To unsubscribe from this list: send the line "unsubscribe
linux-security-module" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 3/3] Allows reading back the current IMA policy;
On Mon, 2015-11-02 at 00:39 +0200, Petko Manolov wrote:
> +
> +#ifdef CONFIG_IMA_READ_POLICY
> +enum {
> + mask_err = -1,
> + mask_exec = 1, mask_write, mask_read, mask_append
> +};
> +
> +static match_table_t mask_tokens = {
> + {mask_exec, "MAY_EXEC"},
> + {mask_write, "MAY_WRITE"},
> + {mask_read, "MAY_READ"},
> + {mask_append, "MAY_APPEND"},
> + {mask_err, NULL}
> +};
> +
> +enum {
> + func_err = -1,
> + func_file = 1, func_mmap, func_bprm,
> + func_module, func_firmware, func_post
> +};
> +
> +static match_table_t func_tokens = {
> + {func_file, "FILE_CHECK"},
> + {func_mmap, "MMAP_CHECK"},
> + {func_bprm, "BPRM_CHECK"},
> + {func_module, "MODULE_CHECK"},
> + {func_firmware, "FIRMWARE_CHECK"},
> + {func_post, "POST_SETATTR"},
> + {func_err, NULL}
> +};
Why are we using match_table_t? Why not define an array of strings
which corresponds to the function hooks or use the __stringify macro?
static const char *ima_hooks_string[] = {"", "FILE_CHECK", "MMAP_CHECK",
"BPRM_CHECK", "MODULE_CHECK", "FIRMWARE_CHECK", "POST_SETATTR"};
In the first case, to display the function hook string would be
"ima_hooks_string[func]". Using __stringify requires the hook name (eg.
__stringify(FILE_CHECK)).
In either case, there would be a lot less code.
Mimi
--
To unsubscribe from this list: send the line "unsubscribe
linux-security-module" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
