| Message ID | 20230315224704.2672-8-casey@schaufler-ca.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | LSM: Three basic syscalls | expand |
On Wed, Mar 15, 2023 at 6:50 PM Casey Schaufler <casey@schaufler-ca.com> wrote: > > Add lsm_name_to_attr(), which translates a text string to a > LSM_ATTR value if one is available. > > Add lsm_fill_user_ctx(), which fills a struct lsm_ctx, including > the trailing attribute value. > > All are used in module specific components of LSM system calls. > > Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> > --- > include/linux/security.h | 13 ++++++++++ > security/lsm_syscalls.c | 51 ++++++++++++++++++++++++++++++++++++++++ > security/security.c | 31 ++++++++++++++++++++++++ > 3 files changed, 95 insertions(+) ... > diff --git a/security/lsm_syscalls.c b/security/lsm_syscalls.c > index 6efbe244d304..55d849ad5d6e 100644 > --- a/security/lsm_syscalls.c > +++ b/security/lsm_syscalls.c > @@ -17,6 +17,57 @@ > #include <linux/lsm_hooks.h> > #include <uapi/linux/lsm.h> > > +struct attr_map { > + char *name; > + u64 attr; > +}; > + > +static const struct attr_map lsm_attr_names[] = { > + { > + .name = "current", > + .attr = LSM_ATTR_CURRENT, > + }, > + { > + .name = "exec", > + .attr = LSM_ATTR_EXEC, > + }, > + { > + .name = "fscreate", > + .attr = LSM_ATTR_FSCREATE, > + }, > + { > + .name = "keycreate", > + .attr = LSM_ATTR_KEYCREATE, > + }, > + { > + .name = "prev", > + .attr = LSM_ATTR_PREV, > + }, > + { > + .name = "sockcreate", > + .attr = LSM_ATTR_SOCKCREATE, > + }, > +}; > + > +/** > + * lsm_name_to_attr - map an LSM attribute name to its ID > + * @name: name of the attribute > + * > + * Look the given @name up in the table of know attribute names. > + * > + * Returns the LSM attribute value associated with @name, or 0 if > + * there is no mapping. > + */ > +u64 lsm_name_to_attr(const char *name) > +{ > + int i; > + > + for (i = 0; i < ARRAY_SIZE(lsm_attr_names); i++) > + if (!strcmp(name, lsm_attr_names[i].name)) > + return lsm_attr_names[i].attr; I'm pretty sure this is the only place where @lsm_attr_names is used, right? If true, when coupled with the idea that these syscalls are going to close the door on new LSM attributes in procfs I think we can just put the mapping directly in this function via a series of if-statements. > + return 0; > +} > + > /** > * sys_lsm_set_self_attr - Set current task's security module attribute > * @attr: which attribute to set > diff --git a/security/security.c b/security/security.c > index 2c57fe28c4f7..f7b814a3940c 100644 > --- a/security/security.c > +++ b/security/security.c > @@ -753,6 +753,37 @@ static int lsm_superblock_alloc(struct super_block *sb) > return 0; > } > > +/** > + * lsm_fill_user_ctx - Fill a user space lsm_ctx structure > + * @ctx: an LSM context to be filled > + * @context: the new context value > + * @context_size: the size of the new context value > + * @id: LSM id > + * @flags: LSM defined flags > + * > + * Fill all of the fields in a user space lsm_ctx structure. > + * Caller is assumed to have verified that @ctx has enough space > + * for @context. > + * Returns 0 on success, -EFAULT on a copyout error. > + */ > +int lsm_fill_user_ctx(struct lsm_ctx __user *ctx, void *context, > + size_t context_size, u64 id, u64 flags) > +{ > + struct lsm_ctx local; > + void __user *vc = ctx; > + > + local.id = id; > + local.flags = flags; > + local.ctx_len = context_size; > + local.len = context_size + sizeof(local); > + vc += sizeof(local); See my prior comments about void pointer math. > + if (copy_to_user(ctx, &local, sizeof(local))) > + return -EFAULT; > + if (context_size > 0 && copy_to_user(vc, context, context_size)) > + return -EFAULT; Should we handle the padding in this function? > + return 0; > +} -- paul-moore.com
On 3/29/2023 6:13 PM, Paul Moore wrote: > On Wed, Mar 15, 2023 at 6:50 PM Casey Schaufler <casey@schaufler-ca.com> wrote: >> Add lsm_name_to_attr(), which translates a text string to a >> LSM_ATTR value if one is available. >> >> Add lsm_fill_user_ctx(), which fills a struct lsm_ctx, including >> the trailing attribute value. >> >> All are used in module specific components of LSM system calls. >> >> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> >> --- >> include/linux/security.h | 13 ++++++++++ >> security/lsm_syscalls.c | 51 ++++++++++++++++++++++++++++++++++++++++ >> security/security.c | 31 ++++++++++++++++++++++++ >> 3 files changed, 95 insertions(+) > .. > >> diff --git a/security/lsm_syscalls.c b/security/lsm_syscalls.c >> index 6efbe244d304..55d849ad5d6e 100644 >> --- a/security/lsm_syscalls.c >> +++ b/security/lsm_syscalls.c >> @@ -17,6 +17,57 @@ >> #include <linux/lsm_hooks.h> >> #include <uapi/linux/lsm.h> >> >> +struct attr_map { >> + char *name; >> + u64 attr; >> +}; >> + >> +static const struct attr_map lsm_attr_names[] = { >> + { >> + .name = "current", >> + .attr = LSM_ATTR_CURRENT, >> + }, >> + { >> + .name = "exec", >> + .attr = LSM_ATTR_EXEC, >> + }, >> + { >> + .name = "fscreate", >> + .attr = LSM_ATTR_FSCREATE, >> + }, >> + { >> + .name = "keycreate", >> + .attr = LSM_ATTR_KEYCREATE, >> + }, >> + { >> + .name = "prev", >> + .attr = LSM_ATTR_PREV, >> + }, >> + { >> + .name = "sockcreate", >> + .attr = LSM_ATTR_SOCKCREATE, >> + }, >> +}; >> + >> +/** >> + * lsm_name_to_attr - map an LSM attribute name to its ID >> + * @name: name of the attribute >> + * >> + * Look the given @name up in the table of know attribute names. >> + * >> + * Returns the LSM attribute value associated with @name, or 0 if >> + * there is no mapping. >> + */ >> +u64 lsm_name_to_attr(const char *name) >> +{ >> + int i; >> + >> + for (i = 0; i < ARRAY_SIZE(lsm_attr_names); i++) >> + if (!strcmp(name, lsm_attr_names[i].name)) >> + return lsm_attr_names[i].attr; > I'm pretty sure this is the only place where @lsm_attr_names is used, > right? If true, when coupled with the idea that these syscalls are > going to close the door on new LSM attributes in procfs I think we can > just put the mapping directly in this function via a series of > if-statements. Ick. You're not wrong, but the hard coded if-statement approach goes against all sorts of coding principles. I'll do it, but I can't say I like it. >> + return 0; >> +} >> + >> /** >> * sys_lsm_set_self_attr - Set current task's security module attribute >> * @attr: which attribute to set >> diff --git a/security/security.c b/security/security.c >> index 2c57fe28c4f7..f7b814a3940c 100644 >> --- a/security/security.c >> +++ b/security/security.c >> @@ -753,6 +753,37 @@ static int lsm_superblock_alloc(struct super_block *sb) >> return 0; >> } >> >> +/** >> + * lsm_fill_user_ctx - Fill a user space lsm_ctx structure >> + * @ctx: an LSM context to be filled >> + * @context: the new context value >> + * @context_size: the size of the new context value >> + * @id: LSM id >> + * @flags: LSM defined flags >> + * >> + * Fill all of the fields in a user space lsm_ctx structure. >> + * Caller is assumed to have verified that @ctx has enough space >> + * for @context. >> + * Returns 0 on success, -EFAULT on a copyout error. >> + */ >> +int lsm_fill_user_ctx(struct lsm_ctx __user *ctx, void *context, >> + size_t context_size, u64 id, u64 flags) >> +{ >> + struct lsm_ctx local; >> + void __user *vc = ctx; >> + >> + local.id = id; >> + local.flags = flags; >> + local.ctx_len = context_size; >> + local.len = context_size + sizeof(local); >> + vc += sizeof(local); > See my prior comments about void pointer math. > >> + if (copy_to_user(ctx, &local, sizeof(local))) >> + return -EFAULT; >> + if (context_size > 0 && copy_to_user(vc, context, context_size)) >> + return -EFAULT; > Should we handle the padding in this function? This function fills in a lsm_ctx. The padding, if any, is in addition to the lsm_ctx, not part of it. >> + return 0; >> +} > -- > paul-moore.com
On Thu, Mar 30, 2023 at 4:42 PM Casey Schaufler <casey@schaufler-ca.com> wrote: > On 3/29/2023 6:13 PM, Paul Moore wrote: > > On Wed, Mar 15, 2023 at 6:50 PM Casey Schaufler <casey@schaufler-ca.com> wrote: > >> Add lsm_name_to_attr(), which translates a text string to a > >> LSM_ATTR value if one is available. > >> > >> Add lsm_fill_user_ctx(), which fills a struct lsm_ctx, including > >> the trailing attribute value. > >> > >> All are used in module specific components of LSM system calls. > >> > >> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> > >> --- > >> include/linux/security.h | 13 ++++++++++ > >> security/lsm_syscalls.c | 51 ++++++++++++++++++++++++++++++++++++++++ > >> security/security.c | 31 ++++++++++++++++++++++++ > >> 3 files changed, 95 insertions(+) > > .. > > > >> diff --git a/security/lsm_syscalls.c b/security/lsm_syscalls.c > >> index 6efbe244d304..55d849ad5d6e 100644 > >> --- a/security/lsm_syscalls.c > >> +++ b/security/lsm_syscalls.c > >> @@ -17,6 +17,57 @@ > >> #include <linux/lsm_hooks.h> > >> #include <uapi/linux/lsm.h> > >> > >> +struct attr_map { > >> + char *name; > >> + u64 attr; > >> +}; > >> + > >> +static const struct attr_map lsm_attr_names[] = { > >> + { > >> + .name = "current", > >> + .attr = LSM_ATTR_CURRENT, > >> + }, > >> + { > >> + .name = "exec", > >> + .attr = LSM_ATTR_EXEC, > >> + }, > >> + { > >> + .name = "fscreate", > >> + .attr = LSM_ATTR_FSCREATE, > >> + }, > >> + { > >> + .name = "keycreate", > >> + .attr = LSM_ATTR_KEYCREATE, > >> + }, > >> + { > >> + .name = "prev", > >> + .attr = LSM_ATTR_PREV, > >> + }, > >> + { > >> + .name = "sockcreate", > >> + .attr = LSM_ATTR_SOCKCREATE, > >> + }, > >> +}; > >> + > >> +/** > >> + * lsm_name_to_attr - map an LSM attribute name to its ID > >> + * @name: name of the attribute > >> + * > >> + * Look the given @name up in the table of know attribute names. > >> + * > >> + * Returns the LSM attribute value associated with @name, or 0 if > >> + * there is no mapping. > >> + */ > >> +u64 lsm_name_to_attr(const char *name) > >> +{ > >> + int i; > >> + > >> + for (i = 0; i < ARRAY_SIZE(lsm_attr_names); i++) > >> + if (!strcmp(name, lsm_attr_names[i].name)) > >> + return lsm_attr_names[i].attr; > > I'm pretty sure this is the only place where @lsm_attr_names is used, > > right? If true, when coupled with the idea that these syscalls are > > going to close the door on new LSM attributes in procfs I think we can > > just put the mapping directly in this function via a series of > > if-statements. > > Ick. You're not wrong, but the hard coded if-statement approach goes > against all sorts of coding principles. I'll do it, but I can't say I > like it. If it helps any, I understand and am sympathetic. I guess I've gotten to that point where in addition to "code elegance", I'm also very concerned about defending against "code abuse", and something like an nicely defined mapping array is ripe for someone to come along and use that to justify further use of the attribute string names in some other function/API. If you want to stick with the array - I have no problem with that - make it local to lsm_name_to_attr(). > >> +/** > >> + * lsm_fill_user_ctx - Fill a user space lsm_ctx structure > >> + * @ctx: an LSM context to be filled > >> + * @context: the new context value > >> + * @context_size: the size of the new context value > >> + * @id: LSM id > >> + * @flags: LSM defined flags > >> + * > >> + * Fill all of the fields in a user space lsm_ctx structure. > >> + * Caller is assumed to have verified that @ctx has enough space > >> + * for @context. > >> + * Returns 0 on success, -EFAULT on a copyout error. > >> + */ > >> +int lsm_fill_user_ctx(struct lsm_ctx __user *ctx, void *context, > >> + size_t context_size, u64 id, u64 flags) > >> +{ > >> + struct lsm_ctx local; > >> + void __user *vc = ctx; > >> + > >> + local.id = id; > >> + local.flags = flags; > >> + local.ctx_len = context_size; > >> + local.len = context_size + sizeof(local); > >> + vc += sizeof(local); > > See my prior comments about void pointer math. > > > >> + if (copy_to_user(ctx, &local, sizeof(local))) > >> + return -EFAULT; > >> + if (context_size > 0 && copy_to_user(vc, context, context_size)) > >> + return -EFAULT; > > Should we handle the padding in this function? > > This function fills in a lsm_ctx. The padding, if any, is in addition to > the lsm_ctx, not part of it. Okay, so where is the padding managed? I may have missed it, but I don't recall seeing it anywhere in this patchset ...
On 3/30/2023 4:28 PM, Paul Moore wrote: > On Thu, Mar 30, 2023 at 4:42 PM Casey Schaufler <casey@schaufler-ca.com> wrote: >> On 3/29/2023 6:13 PM, Paul Moore wrote: >>> On Wed, Mar 15, 2023 at 6:50 PM Casey Schaufler <casey@schaufler-ca.com> wrote: >>>> Add lsm_name_to_attr(), which translates a text string to a >>>> LSM_ATTR value if one is available. >>>> >>>> Add lsm_fill_user_ctx(), which fills a struct lsm_ctx, including >>>> the trailing attribute value. >>>> >>>> All are used in module specific components of LSM system calls. >>>> >>>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> >>>> --- >>>> include/linux/security.h | 13 ++++++++++ >>>> security/lsm_syscalls.c | 51 ++++++++++++++++++++++++++++++++++++++++ >>>> security/security.c | 31 ++++++++++++++++++++++++ >>>> 3 files changed, 95 insertions(+) >>> .. >>> >>>> diff --git a/security/lsm_syscalls.c b/security/lsm_syscalls.c >>>> index 6efbe244d304..55d849ad5d6e 100644 >>>> --- a/security/lsm_syscalls.c >>>> +++ b/security/lsm_syscalls.c >>>> @@ -17,6 +17,57 @@ >>>> #include <linux/lsm_hooks.h> >>>> #include <uapi/linux/lsm.h> >>>> >>>> +struct attr_map { >>>> + char *name; >>>> + u64 attr; >>>> +}; >>>> + >>>> +static const struct attr_map lsm_attr_names[] = { >>>> + { >>>> + .name = "current", >>>> + .attr = LSM_ATTR_CURRENT, >>>> + }, >>>> + { >>>> + .name = "exec", >>>> + .attr = LSM_ATTR_EXEC, >>>> + }, >>>> + { >>>> + .name = "fscreate", >>>> + .attr = LSM_ATTR_FSCREATE, >>>> + }, >>>> + { >>>> + .name = "keycreate", >>>> + .attr = LSM_ATTR_KEYCREATE, >>>> + }, >>>> + { >>>> + .name = "prev", >>>> + .attr = LSM_ATTR_PREV, >>>> + }, >>>> + { >>>> + .name = "sockcreate", >>>> + .attr = LSM_ATTR_SOCKCREATE, >>>> + }, >>>> +}; >>>> + >>>> +/** >>>> + * lsm_name_to_attr - map an LSM attribute name to its ID >>>> + * @name: name of the attribute >>>> + * >>>> + * Look the given @name up in the table of know attribute names. >>>> + * >>>> + * Returns the LSM attribute value associated with @name, or 0 if >>>> + * there is no mapping. >>>> + */ >>>> +u64 lsm_name_to_attr(const char *name) >>>> +{ >>>> + int i; >>>> + >>>> + for (i = 0; i < ARRAY_SIZE(lsm_attr_names); i++) >>>> + if (!strcmp(name, lsm_attr_names[i].name)) >>>> + return lsm_attr_names[i].attr; >>> I'm pretty sure this is the only place where @lsm_attr_names is used, >>> right? If true, when coupled with the idea that these syscalls are >>> going to close the door on new LSM attributes in procfs I think we can >>> just put the mapping directly in this function via a series of >>> if-statements. >> Ick. You're not wrong, but the hard coded if-statement approach goes >> against all sorts of coding principles. I'll do it, but I can't say I >> like it. > If it helps any, I understand and am sympathetic. I guess I've gotten > to that point where in addition to "code elegance", I'm also very > concerned about defending against "code abuse", and something like an > nicely defined mapping array is ripe for someone to come along and use > that to justify further use of the attribute string names in some > other function/API. > > If you want to stick with the array - I have no problem with that - > make it local to lsm_name_to_attr(). > >>>> +/** >>>> + * lsm_fill_user_ctx - Fill a user space lsm_ctx structure >>>> + * @ctx: an LSM context to be filled >>>> + * @context: the new context value >>>> + * @context_size: the size of the new context value >>>> + * @id: LSM id >>>> + * @flags: LSM defined flags >>>> + * >>>> + * Fill all of the fields in a user space lsm_ctx structure. >>>> + * Caller is assumed to have verified that @ctx has enough space >>>> + * for @context. >>>> + * Returns 0 on success, -EFAULT on a copyout error. >>>> + */ >>>> +int lsm_fill_user_ctx(struct lsm_ctx __user *ctx, void *context, >>>> + size_t context_size, u64 id, u64 flags) >>>> +{ >>>> + struct lsm_ctx local; >>>> + void __user *vc = ctx; >>>> + >>>> + local.id = id; >>>> + local.flags = flags; >>>> + local.ctx_len = context_size; >>>> + local.len = context_size + sizeof(local); >>>> + vc += sizeof(local); >>> See my prior comments about void pointer math. >>> >>>> + if (copy_to_user(ctx, &local, sizeof(local))) >>>> + return -EFAULT; >>>> + if (context_size > 0 && copy_to_user(vc, context, context_size)) >>>> + return -EFAULT; >>> Should we handle the padding in this function? >> This function fills in a lsm_ctx. The padding, if any, is in addition to >> the lsm_ctx, not part of it. > Okay, so where is the padding managed? I may have missed it, but I > don't recall seeing it anywhere in this patchset ... Padding isn't being managed. There has been talk about using padding to expand the API, but there is no use for it now. Or is there?
On Fri, Mar 31, 2023 at 12:56 PM Casey Schaufler <casey@schaufler-ca.com> wrote: > On 3/30/2023 4:28 PM, Paul Moore wrote: > > On Thu, Mar 30, 2023 at 4:42 PM Casey Schaufler <casey@schaufler-ca.com> wrote: > >> On 3/29/2023 6:13 PM, Paul Moore wrote: > >>> On Wed, Mar 15, 2023 at 6:50 PM Casey Schaufler <casey@schaufler-ca.com> wrote: > >>>> Add lsm_name_to_attr(), which translates a text string to a > >>>> LSM_ATTR value if one is available. > >>>> > >>>> Add lsm_fill_user_ctx(), which fills a struct lsm_ctx, including > >>>> the trailing attribute value. > >>>> > >>>> All are used in module specific components of LSM system calls. > >>>> > >>>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> > >>>> --- > >>>> include/linux/security.h | 13 ++++++++++ > >>>> security/lsm_syscalls.c | 51 ++++++++++++++++++++++++++++++++++++++++ > >>>> security/security.c | 31 ++++++++++++++++++++++++ > >>>> 3 files changed, 95 insertions(+) > >>> .. > >>> > >>>> diff --git a/security/lsm_syscalls.c b/security/lsm_syscalls.c > >>>> index 6efbe244d304..55d849ad5d6e 100644 > >>>> --- a/security/lsm_syscalls.c > >>>> +++ b/security/lsm_syscalls.c > >>>> @@ -17,6 +17,57 @@ > >>>> #include <linux/lsm_hooks.h> > >>>> #include <uapi/linux/lsm.h> > >>>> > >>>> +struct attr_map { > >>>> + char *name; > >>>> + u64 attr; > >>>> +}; > >>>> + > >>>> +static const struct attr_map lsm_attr_names[] = { > >>>> + { > >>>> + .name = "current", > >>>> + .attr = LSM_ATTR_CURRENT, > >>>> + }, > >>>> + { > >>>> + .name = "exec", > >>>> + .attr = LSM_ATTR_EXEC, > >>>> + }, > >>>> + { > >>>> + .name = "fscreate", > >>>> + .attr = LSM_ATTR_FSCREATE, > >>>> + }, > >>>> + { > >>>> + .name = "keycreate", > >>>> + .attr = LSM_ATTR_KEYCREATE, > >>>> + }, > >>>> + { > >>>> + .name = "prev", > >>>> + .attr = LSM_ATTR_PREV, > >>>> + }, > >>>> + { > >>>> + .name = "sockcreate", > >>>> + .attr = LSM_ATTR_SOCKCREATE, > >>>> + }, > >>>> +}; > >>>> + > >>>> +/** > >>>> + * lsm_name_to_attr - map an LSM attribute name to its ID > >>>> + * @name: name of the attribute > >>>> + * > >>>> + * Look the given @name up in the table of know attribute names. > >>>> + * > >>>> + * Returns the LSM attribute value associated with @name, or 0 if > >>>> + * there is no mapping. > >>>> + */ > >>>> +u64 lsm_name_to_attr(const char *name) > >>>> +{ > >>>> + int i; > >>>> + > >>>> + for (i = 0; i < ARRAY_SIZE(lsm_attr_names); i++) > >>>> + if (!strcmp(name, lsm_attr_names[i].name)) > >>>> + return lsm_attr_names[i].attr; > >>> I'm pretty sure this is the only place where @lsm_attr_names is used, > >>> right? If true, when coupled with the idea that these syscalls are > >>> going to close the door on new LSM attributes in procfs I think we can > >>> just put the mapping directly in this function via a series of > >>> if-statements. > >> Ick. You're not wrong, but the hard coded if-statement approach goes > >> against all sorts of coding principles. I'll do it, but I can't say I > >> like it. > > If it helps any, I understand and am sympathetic. I guess I've gotten > > to that point where in addition to "code elegance", I'm also very > > concerned about defending against "code abuse", and something like an > > nicely defined mapping array is ripe for someone to come along and use > > that to justify further use of the attribute string names in some > > other function/API. > > > > If you want to stick with the array - I have no problem with that - > > make it local to lsm_name_to_attr(). > > > >>>> +/** > >>>> + * lsm_fill_user_ctx - Fill a user space lsm_ctx structure > >>>> + * @ctx: an LSM context to be filled > >>>> + * @context: the new context value > >>>> + * @context_size: the size of the new context value > >>>> + * @id: LSM id > >>>> + * @flags: LSM defined flags > >>>> + * > >>>> + * Fill all of the fields in a user space lsm_ctx structure. > >>>> + * Caller is assumed to have verified that @ctx has enough space > >>>> + * for @context. > >>>> + * Returns 0 on success, -EFAULT on a copyout error. > >>>> + */ > >>>> +int lsm_fill_user_ctx(struct lsm_ctx __user *ctx, void *context, > >>>> + size_t context_size, u64 id, u64 flags) > >>>> +{ > >>>> + struct lsm_ctx local; > >>>> + void __user *vc = ctx; > >>>> + > >>>> + local.id = id; > >>>> + local.flags = flags; > >>>> + local.ctx_len = context_size; > >>>> + local.len = context_size + sizeof(local); > >>>> + vc += sizeof(local); > >>> See my prior comments about void pointer math. > >>> > >>>> + if (copy_to_user(ctx, &local, sizeof(local))) > >>>> + return -EFAULT; > >>>> + if (context_size > 0 && copy_to_user(vc, context, context_size)) > >>>> + return -EFAULT; > >>> Should we handle the padding in this function? > >> This function fills in a lsm_ctx. The padding, if any, is in addition to > >> the lsm_ctx, not part of it. > > Okay, so where is the padding managed? I may have missed it, but I > > don't recall seeing it anywhere in this patchset ... > > Padding isn't being managed. There has been talk about using padding to > expand the API, but there is no use for it now. Or is there? I think two separate ideas are getting conflated, likely because the 'len' field is involved in both. THe first issue is padding at the end of the lsm_ctx struct to ensure that the next array element is aligned. The second issue is the potential for extending the lsm_ctx struct on a per-LSM basis through creative use of the 'flags' and 'len' fields; in this case additional information could be stashed at the end of the lsm_ctx struct after the 'ctx' field. The latter issue (extending the lsm_ctx) isn't something we want to jump into, but it is something the syscall/struct API would allow, and I don't want to exclude it as a possible future solution to a yet unknown future problem. The former issue (padding array elements) isn't a strict requirement as the syscall/struct API works either way, but it seems like a good thing to do.
On 3/31/2023 12:24 PM, Paul Moore wrote: > On Fri, Mar 31, 2023 at 12:56 PM Casey Schaufler <casey@schaufler-ca.com> wrote: >> On 3/30/2023 4:28 PM, Paul Moore wrote: >>> On Thu, Mar 30, 2023 at 4:42 PM Casey Schaufler <casey@schaufler-ca.com> wrote: >>>> On 3/29/2023 6:13 PM, Paul Moore wrote: >>>>> On Wed, Mar 15, 2023 at 6:50 PM Casey Schaufler <casey@schaufler-ca.com> wrote: >>>>>> Add lsm_name_to_attr(), which translates a text string to a >>>>>> LSM_ATTR value if one is available. >>>>>> >>>>>> Add lsm_fill_user_ctx(), which fills a struct lsm_ctx, including >>>>>> the trailing attribute value. >>>>>> >>>>>> All are used in module specific components of LSM system calls. >>>>>> >>>>>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> >>>>>> --- >>>>>> include/linux/security.h | 13 ++++++++++ >>>>>> security/lsm_syscalls.c | 51 ++++++++++++++++++++++++++++++++++++++++ >>>>>> security/security.c | 31 ++++++++++++++++++++++++ >>>>>> 3 files changed, 95 insertions(+) >>>>> .. >>>>> >>>>>> diff --git a/security/lsm_syscalls.c b/security/lsm_syscalls.c >>>>>> index 6efbe244d304..55d849ad5d6e 100644 >>>>>> --- a/security/lsm_syscalls.c >>>>>> +++ b/security/lsm_syscalls.c >>>>>> @@ -17,6 +17,57 @@ >>>>>> #include <linux/lsm_hooks.h> >>>>>> #include <uapi/linux/lsm.h> >>>>>> >>>>>> +struct attr_map { >>>>>> + char *name; >>>>>> + u64 attr; >>>>>> +}; >>>>>> + >>>>>> +static const struct attr_map lsm_attr_names[] = { >>>>>> + { >>>>>> + .name = "current", >>>>>> + .attr = LSM_ATTR_CURRENT, >>>>>> + }, >>>>>> + { >>>>>> + .name = "exec", >>>>>> + .attr = LSM_ATTR_EXEC, >>>>>> + }, >>>>>> + { >>>>>> + .name = "fscreate", >>>>>> + .attr = LSM_ATTR_FSCREATE, >>>>>> + }, >>>>>> + { >>>>>> + .name = "keycreate", >>>>>> + .attr = LSM_ATTR_KEYCREATE, >>>>>> + }, >>>>>> + { >>>>>> + .name = "prev", >>>>>> + .attr = LSM_ATTR_PREV, >>>>>> + }, >>>>>> + { >>>>>> + .name = "sockcreate", >>>>>> + .attr = LSM_ATTR_SOCKCREATE, >>>>>> + }, >>>>>> +}; >>>>>> + >>>>>> +/** >>>>>> + * lsm_name_to_attr - map an LSM attribute name to its ID >>>>>> + * @name: name of the attribute >>>>>> + * >>>>>> + * Look the given @name up in the table of know attribute names. >>>>>> + * >>>>>> + * Returns the LSM attribute value associated with @name, or 0 if >>>>>> + * there is no mapping. >>>>>> + */ >>>>>> +u64 lsm_name_to_attr(const char *name) >>>>>> +{ >>>>>> + int i; >>>>>> + >>>>>> + for (i = 0; i < ARRAY_SIZE(lsm_attr_names); i++) >>>>>> + if (!strcmp(name, lsm_attr_names[i].name)) >>>>>> + return lsm_attr_names[i].attr; >>>>> I'm pretty sure this is the only place where @lsm_attr_names is used, >>>>> right? If true, when coupled with the idea that these syscalls are >>>>> going to close the door on new LSM attributes in procfs I think we can >>>>> just put the mapping directly in this function via a series of >>>>> if-statements. >>>> Ick. You're not wrong, but the hard coded if-statement approach goes >>>> against all sorts of coding principles. I'll do it, but I can't say I >>>> like it. >>> If it helps any, I understand and am sympathetic. I guess I've gotten >>> to that point where in addition to "code elegance", I'm also very >>> concerned about defending against "code abuse", and something like an >>> nicely defined mapping array is ripe for someone to come along and use >>> that to justify further use of the attribute string names in some >>> other function/API. >>> >>> If you want to stick with the array - I have no problem with that - >>> make it local to lsm_name_to_attr(). >>> >>>>>> +/** >>>>>> + * lsm_fill_user_ctx - Fill a user space lsm_ctx structure >>>>>> + * @ctx: an LSM context to be filled >>>>>> + * @context: the new context value >>>>>> + * @context_size: the size of the new context value >>>>>> + * @id: LSM id >>>>>> + * @flags: LSM defined flags >>>>>> + * >>>>>> + * Fill all of the fields in a user space lsm_ctx structure. >>>>>> + * Caller is assumed to have verified that @ctx has enough space >>>>>> + * for @context. >>>>>> + * Returns 0 on success, -EFAULT on a copyout error. >>>>>> + */ >>>>>> +int lsm_fill_user_ctx(struct lsm_ctx __user *ctx, void *context, >>>>>> + size_t context_size, u64 id, u64 flags) >>>>>> +{ >>>>>> + struct lsm_ctx local; >>>>>> + void __user *vc = ctx; >>>>>> + >>>>>> + local.id = id; >>>>>> + local.flags = flags; >>>>>> + local.ctx_len = context_size; >>>>>> + local.len = context_size + sizeof(local); >>>>>> + vc += sizeof(local); >>>>> See my prior comments about void pointer math. >>>>> >>>>>> + if (copy_to_user(ctx, &local, sizeof(local))) >>>>>> + return -EFAULT; >>>>>> + if (context_size > 0 && copy_to_user(vc, context, context_size)) >>>>>> + return -EFAULT; >>>>> Should we handle the padding in this function? >>>> This function fills in a lsm_ctx. The padding, if any, is in addition to >>>> the lsm_ctx, not part of it. >>> Okay, so where is the padding managed? I may have missed it, but I >>> don't recall seeing it anywhere in this patchset ... >> Padding isn't being managed. There has been talk about using padding to >> expand the API, but there is no use for it now. Or is there? > I think two separate ideas are getting conflated, likely because the > 'len' field is involved in both. > > THe first issue is padding at the end of the lsm_ctx struct to ensure > that the next array element is aligned. The second issue is the > potential for extending the lsm_ctx struct on a per-LSM basis through > creative use of the 'flags' and 'len' fields; in this case additional > information could be stashed at the end of the lsm_ctx struct after > the 'ctx' field. The latter issue (extending the lsm_ctx) isn't > something we want to jump into, but it is something the syscall/struct > API would allow, and I don't want to exclude it as a possible future > solution to a yet unknown future problem. The former issue (padding > array elements) isn't a strict requirement as the syscall/struct API > works either way, but it seems like a good thing to do. Reasonable. Thanks for the clarification.
On 15/03/2023 23:47, Casey Schaufler wrote: > Add lsm_name_to_attr(), which translates a text string to a > LSM_ATTR value if one is available. > > Add lsm_fill_user_ctx(), which fills a struct lsm_ctx, including > the trailing attribute value. > > All are used in module specific components of LSM system calls. > > Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> > --- > include/linux/security.h | 13 ++++++++++ > security/lsm_syscalls.c | 51 ++++++++++++++++++++++++++++++++++++++++ > security/security.c | 31 ++++++++++++++++++++++++ > 3 files changed, 95 insertions(+) [...] > diff --git a/security/security.c b/security/security.c > index 2c57fe28c4f7..f7b814a3940c 100644 > --- a/security/security.c > +++ b/security/security.c > @@ -753,6 +753,37 @@ static int lsm_superblock_alloc(struct super_block *sb) > return 0; > } > > +/** > + * lsm_fill_user_ctx - Fill a user space lsm_ctx structure > + * @ctx: an LSM context to be filled > + * @context: the new context value > + * @context_size: the size of the new context value > + * @id: LSM id > + * @flags: LSM defined flags > + * > + * Fill all of the fields in a user space lsm_ctx structure. > + * Caller is assumed to have verified that @ctx has enough space > + * for @context. > + * Returns 0 on success, -EFAULT on a copyout error. > + */ > +int lsm_fill_user_ctx(struct lsm_ctx __user *ctx, void *context, > + size_t context_size, u64 id, u64 flags) > +{ > + struct lsm_ctx local; > + void __user *vc = ctx; > + > + local.id = id; > + local.flags = flags; > + local.ctx_len = context_size; > + local.len = context_size + sizeof(local); > + vc += sizeof(local); > + if (copy_to_user(ctx, &local, sizeof(local))) > + return -EFAULT; > + if (context_size > 0 && copy_to_user(vc, context, context_size)) > + return -EFAULT; Can we do a single copy_to_user() call? That would avoid inconsistent user space data, could speed up a bit the operation, and make the code easier to understand. To use the stack, we need to know the maximum size of context_size for all use cases, which seems reasonable and can be checked at build time (on each LSM side, and potentially with specific context type passed as enum instead of context_size) and run time (for this generic helper). > + return 0; > +} > + > /* > * The default value of the LSM hook is defined in linux/lsm_hook_defs.h and > * can be accessed with:
On 03/04/2023 11:47, Mickaël Salaün wrote: > > On 15/03/2023 23:47, Casey Schaufler wrote: >> Add lsm_name_to_attr(), which translates a text string to a >> LSM_ATTR value if one is available. >> >> Add lsm_fill_user_ctx(), which fills a struct lsm_ctx, including >> the trailing attribute value. >> >> All are used in module specific components of LSM system calls. >> >> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> >> --- >> include/linux/security.h | 13 ++++++++++ >> security/lsm_syscalls.c | 51 ++++++++++++++++++++++++++++++++++++++++ >> security/security.c | 31 ++++++++++++++++++++++++ >> 3 files changed, 95 insertions(+) > > [...] > >> diff --git a/security/security.c b/security/security.c >> index 2c57fe28c4f7..f7b814a3940c 100644 >> --- a/security/security.c >> +++ b/security/security.c >> @@ -753,6 +753,37 @@ static int lsm_superblock_alloc(struct super_block *sb) >> return 0; >> } >> >> +/** >> + * lsm_fill_user_ctx - Fill a user space lsm_ctx structure >> + * @ctx: an LSM context to be filled >> + * @context: the new context value >> + * @context_size: the size of the new context value >> + * @id: LSM id >> + * @flags: LSM defined flags >> + * >> + * Fill all of the fields in a user space lsm_ctx structure. >> + * Caller is assumed to have verified that @ctx has enough space >> + * for @context. >> + * Returns 0 on success, -EFAULT on a copyout error. >> + */ >> +int lsm_fill_user_ctx(struct lsm_ctx __user *ctx, void *context, >> + size_t context_size, u64 id, u64 flags) >> +{ >> + struct lsm_ctx local; >> + void __user *vc = ctx; >> + >> + local.id = id; >> + local.flags = flags; >> + local.ctx_len = context_size; >> + local.len = context_size + sizeof(local); >> + vc += sizeof(local); >> + if (copy_to_user(ctx, &local, sizeof(local))) >> + return -EFAULT; >> + if (context_size > 0 && copy_to_user(vc, context, context_size)) >> + return -EFAULT; > > Can we do a single copy_to_user() call? That would avoid inconsistent > user space data, could speed up a bit the operation, and make the code > easier to understand. To use the stack, we need to know the maximum size > of context_size for all use cases, which seems reasonable and can be > checked at build time (on each LSM side, and potentially with specific > context type passed as enum instead of context_size) and run time (for > this generic helper). Well, actually the context_size should be inferred from id, and the "local" size should be defined and check at build time against all context ID sizes. > > >> + return 0; >> +} >> + >> /* >> * The default value of the LSM hook is defined in linux/lsm_hook_defs.h and >> * can be accessed with:
On 03/04/2023 11:54, Mickaël Salaün wrote: > > On 03/04/2023 11:47, Mickaël Salaün wrote: >> >> On 15/03/2023 23:47, Casey Schaufler wrote: >>> Add lsm_name_to_attr(), which translates a text string to a >>> LSM_ATTR value if one is available. >>> >>> Add lsm_fill_user_ctx(), which fills a struct lsm_ctx, including >>> the trailing attribute value. >>> >>> All are used in module specific components of LSM system calls. >>> >>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> >>> --- >>> include/linux/security.h | 13 ++++++++++ >>> security/lsm_syscalls.c | 51 ++++++++++++++++++++++++++++++++++++++++ >>> security/security.c | 31 ++++++++++++++++++++++++ >>> 3 files changed, 95 insertions(+) >> >> [...] >> >>> diff --git a/security/security.c b/security/security.c >>> index 2c57fe28c4f7..f7b814a3940c 100644 >>> --- a/security/security.c >>> +++ b/security/security.c >>> @@ -753,6 +753,37 @@ static int lsm_superblock_alloc(struct super_block *sb) >>> return 0; >>> } >>> >>> +/** >>> + * lsm_fill_user_ctx - Fill a user space lsm_ctx structure >>> + * @ctx: an LSM context to be filled >>> + * @context: the new context value >>> + * @context_size: the size of the new context value >>> + * @id: LSM id >>> + * @flags: LSM defined flags >>> + * >>> + * Fill all of the fields in a user space lsm_ctx structure. >>> + * Caller is assumed to have verified that @ctx has enough space >>> + * for @context. >>> + * Returns 0 on success, -EFAULT on a copyout error. >>> + */ >>> +int lsm_fill_user_ctx(struct lsm_ctx __user *ctx, void *context, >>> + size_t context_size, u64 id, u64 flags) >>> +{ >>> + struct lsm_ctx local; >>> + void __user *vc = ctx; >>> + >>> + local.id = id; >>> + local.flags = flags; >>> + local.ctx_len = context_size; >>> + local.len = context_size + sizeof(local); >>> + vc += sizeof(local); >>> + if (copy_to_user(ctx, &local, sizeof(local))) >>> + return -EFAULT; >>> + if (context_size > 0 && copy_to_user(vc, context, context_size)) >>> + return -EFAULT; >> >> Can we do a single copy_to_user() call? That would avoid inconsistent >> user space data, could speed up a bit the operation, and make the code >> easier to understand. To use the stack, we need to know the maximum size >> of context_size for all use cases, which seems reasonable and can be >> checked at build time (on each LSM side, and potentially with specific >> context type passed as enum instead of context_size) and run time (for >> this generic helper). > > Well, actually the context_size should be inferred from id, and the > "local" size should be defined and check at build time against all > context ID sizes. @ctx_len should already be known by user space according to the LSM ID and the requested attribute. @len should already be known by user space because lsm_ctx is part of the ABI. The only reason I can think of the rationale for @len and @ctx_len is that struct lsm_ctx could gain more fields. If this happen, they would then need to be inserted before @ctx. This would make this struct lsm_ctx too flexible and complex for user space to parse correctly (e.g. for strace, gdb). I don't see where we could use @flags instead of relying on a new attribute type. I think security_getselfattr() and lsm_fill_user_ctx() could be changed to avoid each LSM to pass their own ID to lsm_fill_user_ctx(). We could have a lsm_get_attr_size(lsm_id, attr) helper (called by security_getselfattr) to group these relations, based on fixed values, exposed in the UAPI, and checked at build time with the size of the related LSM-specific attribute type. This would also allow to factor out the total size calculation needed before calling the getselfattr() implementers, and then rely on a common consistent behavior. That could also be used to not call getselfattr() implementers if they don't handle a specific attribute, and then remove their related error handling for this case. For now, the getselfattr() hook (not the related syscall) doesn't need to pass a "flags" argument to each LSM because there is no use of it. > >> >> >>> + return 0; >>> +} >>> + >>> /* >>> * The default value of the LSM hook is defined in linux/lsm_hook_defs.h and >>> * can be accessed with:
On 4/3/2023 2:47 AM, Mickaël Salaün wrote: > > On 15/03/2023 23:47, Casey Schaufler wrote: >> Add lsm_name_to_attr(), which translates a text string to a >> LSM_ATTR value if one is available. >> >> Add lsm_fill_user_ctx(), which fills a struct lsm_ctx, including >> the trailing attribute value. >> >> All are used in module specific components of LSM system calls. >> >> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> >> --- >> include/linux/security.h | 13 ++++++++++ >> security/lsm_syscalls.c | 51 ++++++++++++++++++++++++++++++++++++++++ >> security/security.c | 31 ++++++++++++++++++++++++ >> 3 files changed, 95 insertions(+) > > [...] > >> diff --git a/security/security.c b/security/security.c >> index 2c57fe28c4f7..f7b814a3940c 100644 >> --- a/security/security.c >> +++ b/security/security.c >> @@ -753,6 +753,37 @@ static int lsm_superblock_alloc(struct >> super_block *sb) >> return 0; >> } >> +/** >> + * lsm_fill_user_ctx - Fill a user space lsm_ctx structure >> + * @ctx: an LSM context to be filled >> + * @context: the new context value >> + * @context_size: the size of the new context value >> + * @id: LSM id >> + * @flags: LSM defined flags >> + * >> + * Fill all of the fields in a user space lsm_ctx structure. >> + * Caller is assumed to have verified that @ctx has enough space >> + * for @context. >> + * Returns 0 on success, -EFAULT on a copyout error. >> + */ >> +int lsm_fill_user_ctx(struct lsm_ctx __user *ctx, void *context, >> + size_t context_size, u64 id, u64 flags) >> +{ >> + struct lsm_ctx local; >> + void __user *vc = ctx; >> + >> + local.id = id; >> + local.flags = flags; >> + local.ctx_len = context_size; >> + local.len = context_size + sizeof(local); >> + vc += sizeof(local); >> + if (copy_to_user(ctx, &local, sizeof(local))) >> + return -EFAULT; >> + if (context_size > 0 && copy_to_user(vc, context, context_size)) >> + return -EFAULT; > > Can we do a single copy_to_user() call? It would be possible, but would require allocating memory and copying the context. I don't see that as an improvement. > That would avoid inconsistent user space data, could speed up a bit > the operation, and make the code easier to understand. To use the > stack, we need to know the maximum size of context_size for all use > cases, which seems reasonable and can be checked at build time (on > each LSM side, and potentially with specific context type passed as > enum instead of context_size) and run time (for this generic helper). Knowning the maximum size of attributes for all LSMs and hard coding that here would make maintaining this code really painful. > > >> + return 0; >> +} >> + >> /* >> * The default value of the LSM hook is defined in >> linux/lsm_hook_defs.h and >> * can be accessed with:
On 4/3/2023 2:54 AM, Mickaël Salaün wrote: > > On 03/04/2023 11:47, Mickaël Salaün wrote: >> >> On 15/03/2023 23:47, Casey Schaufler wrote: >>> Add lsm_name_to_attr(), which translates a text string to a >>> LSM_ATTR value if one is available. >>> >>> Add lsm_fill_user_ctx(), which fills a struct lsm_ctx, including >>> the trailing attribute value. >>> >>> All are used in module specific components of LSM system calls. >>> >>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> >>> --- >>> include/linux/security.h | 13 ++++++++++ >>> security/lsm_syscalls.c | 51 >>> ++++++++++++++++++++++++++++++++++++++++ >>> security/security.c | 31 ++++++++++++++++++++++++ >>> 3 files changed, 95 insertions(+) >> >> [...] >> >>> diff --git a/security/security.c b/security/security.c >>> index 2c57fe28c4f7..f7b814a3940c 100644 >>> --- a/security/security.c >>> +++ b/security/security.c >>> @@ -753,6 +753,37 @@ static int lsm_superblock_alloc(struct >>> super_block *sb) >>> return 0; >>> } >>> +/** >>> + * lsm_fill_user_ctx - Fill a user space lsm_ctx structure >>> + * @ctx: an LSM context to be filled >>> + * @context: the new context value >>> + * @context_size: the size of the new context value >>> + * @id: LSM id >>> + * @flags: LSM defined flags >>> + * >>> + * Fill all of the fields in a user space lsm_ctx structure. >>> + * Caller is assumed to have verified that @ctx has enough space >>> + * for @context. >>> + * Returns 0 on success, -EFAULT on a copyout error. >>> + */ >>> +int lsm_fill_user_ctx(struct lsm_ctx __user *ctx, void *context, >>> + size_t context_size, u64 id, u64 flags) >>> +{ >>> + struct lsm_ctx local; >>> + void __user *vc = ctx; >>> + >>> + local.id = id; >>> + local.flags = flags; >>> + local.ctx_len = context_size; >>> + local.len = context_size + sizeof(local); >>> + vc += sizeof(local); >>> + if (copy_to_user(ctx, &local, sizeof(local))) >>> + return -EFAULT; >>> + if (context_size > 0 && copy_to_user(vc, context, context_size)) >>> + return -EFAULT; >> >> Can we do a single copy_to_user() call? That would avoid inconsistent >> user space data, could speed up a bit the operation, and make the code >> easier to understand. To use the stack, we need to know the maximum size >> of context_size for all use cases, which seems reasonable and can be >> checked at build time (on each LSM side, and potentially with specific >> context type passed as enum instead of context_size) and run time (for >> this generic helper). > > Well, actually the context_size should be inferred from id, and the > "local" size should be defined and check at build time against all > context ID sizes. Again, no, I don't see this as an improvement. > >> >> >>> + return 0; >>> +} >>> + >>> /* >>> * The default value of the LSM hook is defined in >>> linux/lsm_hook_defs.h and >>> * can be accessed with:
On 03/04/2023 20:03, Casey Schaufler wrote: > On 4/3/2023 2:47 AM, Mickaël Salaün wrote: >> >> On 15/03/2023 23:47, Casey Schaufler wrote: >>> Add lsm_name_to_attr(), which translates a text string to a >>> LSM_ATTR value if one is available. >>> >>> Add lsm_fill_user_ctx(), which fills a struct lsm_ctx, including >>> the trailing attribute value. >>> >>> All are used in module specific components of LSM system calls. >>> >>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> >>> --- >>> include/linux/security.h | 13 ++++++++++ >>> security/lsm_syscalls.c | 51 ++++++++++++++++++++++++++++++++++++++++ >>> security/security.c | 31 ++++++++++++++++++++++++ >>> 3 files changed, 95 insertions(+) >> >> [...] >> >>> diff --git a/security/security.c b/security/security.c >>> index 2c57fe28c4f7..f7b814a3940c 100644 >>> --- a/security/security.c >>> +++ b/security/security.c >>> @@ -753,6 +753,37 @@ static int lsm_superblock_alloc(struct >>> super_block *sb) >>> return 0; >>> } >>> +/** >>> + * lsm_fill_user_ctx - Fill a user space lsm_ctx structure >>> + * @ctx: an LSM context to be filled >>> + * @context: the new context value >>> + * @context_size: the size of the new context value >>> + * @id: LSM id >>> + * @flags: LSM defined flags >>> + * >>> + * Fill all of the fields in a user space lsm_ctx structure. >>> + * Caller is assumed to have verified that @ctx has enough space >>> + * for @context. >>> + * Returns 0 on success, -EFAULT on a copyout error. >>> + */ >>> +int lsm_fill_user_ctx(struct lsm_ctx __user *ctx, void *context, >>> + size_t context_size, u64 id, u64 flags) >>> +{ >>> + struct lsm_ctx local; >>> + void __user *vc = ctx; >>> + >>> + local.id = id; >>> + local.flags = flags; >>> + local.ctx_len = context_size; >>> + local.len = context_size + sizeof(local); >>> + vc += sizeof(local); >>> + if (copy_to_user(ctx, &local, sizeof(local))) >>> + return -EFAULT; >>> + if (context_size > 0 && copy_to_user(vc, context, context_size)) >>> + return -EFAULT; >> >> Can we do a single copy_to_user() call? > > It would be possible, but would require allocating memory and copying > the context. I don't see that as an improvement. > >> That would avoid inconsistent user space data, could speed up a bit >> the operation, and make the code easier to understand. To use the >> stack, we need to know the maximum size of context_size for all use >> cases, which seems reasonable and can be checked at build time (on >> each LSM side, and potentially with specific context type passed as >> enum instead of context_size) and run time (for this generic helper). > > Knowning the maximum size of attributes for all LSMs and hard coding > that here would make maintaining this code really painful. Hmm, I forgot about variable-length strings, but maybe a reasonable common maximum size (that could fit on the stack) could be found? > >> >> >>> + return 0; >>> +} >>> + >>> /* >>> * The default value of the LSM hook is defined in >>> linux/lsm_hook_defs.h and >>> * can be accessed with:
On 4/3/2023 11:06 AM, Mickaël Salaün wrote: > > On 03/04/2023 20:03, Casey Schaufler wrote: >> On 4/3/2023 2:47 AM, Mickaël Salaün wrote: >>> >>> On 15/03/2023 23:47, Casey Schaufler wrote: >>>> Add lsm_name_to_attr(), which translates a text string to a >>>> LSM_ATTR value if one is available. >>>> >>>> Add lsm_fill_user_ctx(), which fills a struct lsm_ctx, including >>>> the trailing attribute value. >>>> >>>> All are used in module specific components of LSM system calls. >>>> >>>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> >>>> --- >>>> include/linux/security.h | 13 ++++++++++ >>>> security/lsm_syscalls.c | 51 >>>> ++++++++++++++++++++++++++++++++++++++++ >>>> security/security.c | 31 ++++++++++++++++++++++++ >>>> 3 files changed, 95 insertions(+) >>> >>> [...] >>> >>>> diff --git a/security/security.c b/security/security.c >>>> index 2c57fe28c4f7..f7b814a3940c 100644 >>>> --- a/security/security.c >>>> +++ b/security/security.c >>>> @@ -753,6 +753,37 @@ static int lsm_superblock_alloc(struct >>>> super_block *sb) >>>> return 0; >>>> } >>>> +/** >>>> + * lsm_fill_user_ctx - Fill a user space lsm_ctx structure >>>> + * @ctx: an LSM context to be filled >>>> + * @context: the new context value >>>> + * @context_size: the size of the new context value >>>> + * @id: LSM id >>>> + * @flags: LSM defined flags >>>> + * >>>> + * Fill all of the fields in a user space lsm_ctx structure. >>>> + * Caller is assumed to have verified that @ctx has enough space >>>> + * for @context. >>>> + * Returns 0 on success, -EFAULT on a copyout error. >>>> + */ >>>> +int lsm_fill_user_ctx(struct lsm_ctx __user *ctx, void *context, >>>> + size_t context_size, u64 id, u64 flags) >>>> +{ >>>> + struct lsm_ctx local; >>>> + void __user *vc = ctx; >>>> + >>>> + local.id = id; >>>> + local.flags = flags; >>>> + local.ctx_len = context_size; >>>> + local.len = context_size + sizeof(local); >>>> + vc += sizeof(local); >>>> + if (copy_to_user(ctx, &local, sizeof(local))) >>>> + return -EFAULT; >>>> + if (context_size > 0 && copy_to_user(vc, context, context_size)) >>>> + return -EFAULT; >>> >>> Can we do a single copy_to_user() call? >> >> It would be possible, but would require allocating memory and copying >> the context. I don't see that as an improvement. >> >>> That would avoid inconsistent user space data, could speed up a bit >>> the operation, and make the code easier to understand. To use the >>> stack, we need to know the maximum size of context_size for all use >>> cases, which seems reasonable and can be checked at build time (on >>> each LSM side, and potentially with specific context type passed as >>> enum instead of context_size) and run time (for this generic helper). >> >> Knowning the maximum size of attributes for all LSMs and hard coding >> that here would make maintaining this code really painful. > > Hmm, I forgot about variable-length strings, but maybe a reasonable > common maximum size (that could fit on the stack) could be found? Putting a maximum size limit on LSM attributes just to reduce the number of copy_to_user() calls in this helper function does not make a whole lot of sense to me. > >> >>> >>> >>>> + return 0; >>>> +} >>>> + >>>> /* >>>> * The default value of the LSM hook is defined in >>>> linux/lsm_hook_defs.h and >>>> * can be accessed with:
diff --git a/include/linux/security.h b/include/linux/security.h index 329cd9d2be50..a5e860d332b5 100644 --- a/include/linux/security.h +++ b/include/linux/security.h @@ -263,6 +263,7 @@ int unregister_blocking_lsm_notifier(struct notifier_block *nb); /* prototypes */ extern int security_init(void); extern int early_security_init(void); +extern u64 lsm_name_to_attr(const char *name); /* Security operations */ int security_binder_set_context_mgr(const struct cred *mgr); @@ -491,6 +492,8 @@ int security_inode_notifysecctx(struct inode *inode, void *ctx, u32 ctxlen); int security_inode_setsecctx(struct dentry *dentry, void *ctx, u32 ctxlen); int security_inode_getsecctx(struct inode *inode, void **ctx, u32 *ctxlen); int security_locked_down(enum lockdown_reason what); +int lsm_fill_user_ctx(struct lsm_ctx __user *ctx, void *context, + size_t context_size, u64 id, u64 flags); #else /* CONFIG_SECURITY */ static inline int call_blocking_lsm_notifier(enum lsm_event event, void *data) @@ -508,6 +511,11 @@ static inline int unregister_blocking_lsm_notifier(struct notifier_block *nb) return 0; } +static inline u64 lsm_name_to_attr(const char *name) +{ + return 0; +} + static inline void security_free_mnt_opts(void **mnt_opts) { } @@ -1420,6 +1428,11 @@ static inline int security_locked_down(enum lockdown_reason what) { return 0; } +static inline int lsm_fill_user_ctx(struct lsm_ctx __user *ctx, void *context, + size_t context_size, u64 id, u64 flags) +{ + return 0; +} #endif /* CONFIG_SECURITY */ #if defined(CONFIG_SECURITY) && defined(CONFIG_WATCH_QUEUE) diff --git a/security/lsm_syscalls.c b/security/lsm_syscalls.c index 6efbe244d304..55d849ad5d6e 100644 --- a/security/lsm_syscalls.c +++ b/security/lsm_syscalls.c @@ -17,6 +17,57 @@ #include <linux/lsm_hooks.h> #include <uapi/linux/lsm.h> +struct attr_map { + char *name; + u64 attr; +}; + +static const struct attr_map lsm_attr_names[] = { + { + .name = "current", + .attr = LSM_ATTR_CURRENT, + }, + { + .name = "exec", + .attr = LSM_ATTR_EXEC, + }, + { + .name = "fscreate", + .attr = LSM_ATTR_FSCREATE, + }, + { + .name = "keycreate", + .attr = LSM_ATTR_KEYCREATE, + }, + { + .name = "prev", + .attr = LSM_ATTR_PREV, + }, + { + .name = "sockcreate", + .attr = LSM_ATTR_SOCKCREATE, + }, +}; + +/** + * lsm_name_to_attr - map an LSM attribute name to its ID + * @name: name of the attribute + * + * Look the given @name up in the table of know attribute names. + * + * Returns the LSM attribute value associated with @name, or 0 if + * there is no mapping. + */ +u64 lsm_name_to_attr(const char *name) +{ + int i; + + for (i = 0; i < ARRAY_SIZE(lsm_attr_names); i++) + if (!strcmp(name, lsm_attr_names[i].name)) + return lsm_attr_names[i].attr; + return 0; +} + /** * sys_lsm_set_self_attr - Set current task's security module attribute * @attr: which attribute to set diff --git a/security/security.c b/security/security.c index 2c57fe28c4f7..f7b814a3940c 100644 --- a/security/security.c +++ b/security/security.c @@ -753,6 +753,37 @@ static int lsm_superblock_alloc(struct super_block *sb) return 0; } +/** + * lsm_fill_user_ctx - Fill a user space lsm_ctx structure + * @ctx: an LSM context to be filled + * @context: the new context value + * @context_size: the size of the new context value + * @id: LSM id + * @flags: LSM defined flags + * + * Fill all of the fields in a user space lsm_ctx structure. + * Caller is assumed to have verified that @ctx has enough space + * for @context. + * Returns 0 on success, -EFAULT on a copyout error. + */ +int lsm_fill_user_ctx(struct lsm_ctx __user *ctx, void *context, + size_t context_size, u64 id, u64 flags) +{ + struct lsm_ctx local; + void __user *vc = ctx; + + local.id = id; + local.flags = flags; + local.ctx_len = context_size; + local.len = context_size + sizeof(local); + vc += sizeof(local); + if (copy_to_user(ctx, &local, sizeof(local))) + return -EFAULT; + if (context_size > 0 && copy_to_user(vc, context, context_size)) + return -EFAULT; + return 0; +} + /* * The default value of the LSM hook is defined in linux/lsm_hook_defs.h and * can be accessed with:
Add lsm_name_to_attr(), which translates a text string to a LSM_ATTR value if one is available. Add lsm_fill_user_ctx(), which fills a struct lsm_ctx, including the trailing attribute value. All are used in module specific components of LSM system calls. Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> --- include/linux/security.h | 13 ++++++++++ security/lsm_syscalls.c | 51 ++++++++++++++++++++++++++++++++++++++++ security/security.c | 31 ++++++++++++++++++++++++ 3 files changed, 95 insertions(+)