| Message ID | 20240711111908.3817636-8-xukuohai@huaweicloud.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | Add return value range check for BPF LSM | expand |
On Jul 11, 2024 Xu Kuohai <xukuohai@huaweicloud.com> wrote: > > To be consistent with most LSM hooks, convert the return value of > hook setprocattr to 0 or a negative error code. > > Before: > - Hook setprocattr returns the number of bytes written on success > or a negative error code on failure. > > After: > - Hook setprocattr returns 0 on success or a negative error code > on failure. An output parameter @wbytes is introduced to hold > the number of bytes written on success. > > Signed-off-by: Xu Kuohai <xukuohai@huawei.com> > --- > fs/proc/base.c | 5 +++-- > include/linux/lsm_hook_defs.h | 3 ++- > include/linux/security.h | 5 +++-- > security/apparmor/lsm.c | 10 +++++++--- > security/security.c | 8 +++++--- > security/selinux/hooks.c | 11 ++++++++--- > security/smack/smack_lsm.c | 14 ++++++++++---- > 7 files changed, 38 insertions(+), 18 deletions(-) The security_setprocattr() hook is another odd case that we probably just want to leave alone for two reasons: 1. With the move to LSM syscalls for getting/setting a task's LSM attributes we are "freezing" the procfs API and not adding any new entries to it. 2. The BPF LSM doesn't currently register any procfs entries. I'd suggest leaving security_setprocattr() as-is and blocking it in the BPF verifier, I can't see any reason why a BPF LSM would need this hook. > diff --git a/fs/proc/base.c b/fs/proc/base.c > index 72a1acd03675..9e1cf6cc674d 100644 > --- a/fs/proc/base.c > +++ b/fs/proc/base.c > @@ -2740,6 +2740,7 @@ static ssize_t proc_pid_attr_write(struct file * file, const char __user * buf, > { > struct inode * inode = file_inode(file); > struct task_struct *task; > + size_t wbytes; > void *page; > int rv; > > @@ -2785,12 +2786,12 @@ static ssize_t proc_pid_attr_write(struct file * file, const char __user * buf, > > rv = security_setprocattr(PROC_I(inode)->op.lsmid, > file->f_path.dentry->d_name.name, page, > - count); > + count, &wbytes); > mutex_unlock(¤t->signal->cred_guard_mutex); > out_free: > kfree(page); > out: > - return rv; > + return rv < 0 ? rv : wbytes; > } > > static const struct file_operations proc_pid_attr_operations = { > diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h > index dbc16f14f42f..2628514bb19c 100644 > --- a/include/linux/lsm_hook_defs.h > +++ b/include/linux/lsm_hook_defs.h > @@ -287,7 +287,8 @@ LSM_HOOK(int, -EOPNOTSUPP, setselfattr, unsigned int attr, > struct lsm_ctx *ctx, u32 size, u32 flags) > LSM_HOOK(int, -EINVAL, getprocattr, struct task_struct *p, const char *name, > char **value) > -LSM_HOOK(int, -EINVAL, setprocattr, const char *name, void *value, size_t size) > +LSM_HOOK(int, -EINVAL, setprocattr, const char *name, void *value, size_t size, > + size_t *wbytes) > LSM_HOOK(int, 0, ismaclabel, const char *name) > LSM_HOOK(int, -EOPNOTSUPP, secid_to_secctx, u32 secid, char **secdata, > u32 *seclen) > diff --git a/include/linux/security.h b/include/linux/security.h > index 96a63e132abf..1f1a9696e65d 100644 > --- a/include/linux/security.h > +++ b/include/linux/security.h > @@ -496,7 +496,8 @@ int security_setselfattr(unsigned int attr, struct lsm_ctx __user *ctx, > u32 size, u32 flags); > int security_getprocattr(struct task_struct *p, int lsmid, const char *name, > char **value); > -int security_setprocattr(int lsmid, const char *name, void *value, size_t size); > +int security_setprocattr(int lsmid, const char *name, void *value, size_t size, > + size_t *wbytes); > int security_netlink_send(struct sock *sk, struct sk_buff *skb); > int security_ismaclabel(const char *name); > int security_secid_to_secctx(u32 secid, char **secdata, u32 *seclen); > @@ -1440,7 +1441,7 @@ static inline int security_getprocattr(struct task_struct *p, int lsmid, > } > > static inline int security_setprocattr(int lsmid, char *name, void *value, > - size_t size) > + size_t size, size_t *wbytes) > { > return -EINVAL; > } > diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c > index 72dd09993f28..6c8b1f8c5781 100644 > --- a/security/apparmor/lsm.c > +++ b/security/apparmor/lsm.c > @@ -939,13 +939,17 @@ static int apparmor_setselfattr(unsigned int attr, struct lsm_ctx *ctx, > } > > static int apparmor_setprocattr(const char *name, void *value, > - size_t size) > + size_t size, size_t *wbytes) > { > + int rc = -EINVAL; > int attr = lsm_name_to_attr(name); > > if (attr) > - return do_setattr(attr, value, size); > - return -EINVAL; > + rc = do_setattr(attr, value, size); > + if (rc < 0) > + return rc; > + *wbytes = rc; > + return 0; > } > > /** > diff --git a/security/security.c b/security/security.c > index 095e78efcb32..9685096dbf16 100644 > --- a/security/security.c > +++ b/security/security.c > @@ -4141,20 +4141,22 @@ int security_getprocattr(struct task_struct *p, int lsmid, const char *name, > * @name: attribute name > * @value: attribute value > * @size: attribute value size > + * @wbytes: bytes written on success > * > * Write (set) the current task's attribute @name to @value, size @size if > * allowed. > * > - * Return: Returns bytes written on success, a negative value otherwise. > + * Return: Returns 0 on success, a negative error code otherwise. > */ > -int security_setprocattr(int lsmid, const char *name, void *value, size_t size) > +int security_setprocattr(int lsmid, const char *name, void *value, size_t size, > + size_t *wbytes) > { > struct security_hook_list *hp; > > hlist_for_each_entry(hp, &security_hook_heads.setprocattr, list) { > if (lsmid != 0 && lsmid != hp->lsmid->id) > continue; > - return hp->hook.setprocattr(name, value, size); > + return hp->hook.setprocattr(name, value, size, wbytes); > } > return LSM_RET_DEFAULT(setprocattr); > } > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > index 0d35bb93baca..7a73f3710025 100644 > --- a/security/selinux/hooks.c > +++ b/security/selinux/hooks.c > @@ -6589,13 +6589,18 @@ static int selinux_getprocattr(struct task_struct *p, > return -EINVAL; > } > > -static int selinux_setprocattr(const char *name, void *value, size_t size) > +static int selinux_setprocattr(const char *name, void *value, size_t size, > + size_t *wbytes) > { > + int rc = -EINVAL; > int attr = lsm_name_to_attr(name); > > if (attr) > - return selinux_lsm_setattr(attr, value, size); > - return -EINVAL; > + rc = selinux_lsm_setattr(attr, value, size); > + if (rc < 0) > + return rc; > + *wbytes = rc; > + return 0; > } > > static int selinux_ismaclabel(const char *name) > diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c > index 63d9c5f456c1..4265f2639106 100644 > --- a/security/smack/smack_lsm.c > +++ b/security/smack/smack_lsm.c > @@ -3797,19 +3797,25 @@ static int smack_setselfattr(unsigned int attr, struct lsm_ctx *ctx, > * @name: the name of the attribute in /proc/.../attr > * @value: the value to set > * @size: the size of the value > + * @wbytes: the length of the smack label written > * > * Sets the Smack value of the task. Only setting self > * is permitted and only with privilege > * > - * Returns the length of the smack label or an error code > + * Returns 0 on success or a negative error code > */ > -static int smack_setprocattr(const char *name, void *value, size_t size) > +static int smack_setprocattr(const char *name, void *value, size_t size, > + size_t *wbytes) > { > + int rc = -EINVAL; > int attr = lsm_name_to_attr(name); > > if (attr != LSM_ATTR_UNDEF) > - return do_setattr(attr, value, size); > - return -EINVAL; > + rc = do_setattr(attr, value, size); > + if (rc < 0) > + return rc; > + *wbytes = rc; > + return 0; > } > > /** > -- > 2.30.2 -- paul-moore.com
On 7/19/2024 10:08 AM, Paul Moore wrote: > On Jul 11, 2024 Xu Kuohai <xukuohai@huaweicloud.com> wrote: >> >> To be consistent with most LSM hooks, convert the return value of >> hook setprocattr to 0 or a negative error code. >> >> Before: >> - Hook setprocattr returns the number of bytes written on success >> or a negative error code on failure. >> >> After: >> - Hook setprocattr returns 0 on success or a negative error code >> on failure. An output parameter @wbytes is introduced to hold >> the number of bytes written on success. >> >> Signed-off-by: Xu Kuohai <xukuohai@huawei.com> >> --- >> fs/proc/base.c | 5 +++-- >> include/linux/lsm_hook_defs.h | 3 ++- >> include/linux/security.h | 5 +++-- >> security/apparmor/lsm.c | 10 +++++++--- >> security/security.c | 8 +++++--- >> security/selinux/hooks.c | 11 ++++++++--- >> security/smack/smack_lsm.c | 14 ++++++++++---- >> 7 files changed, 38 insertions(+), 18 deletions(-) > > The security_setprocattr() hook is another odd case that we probably > just want to leave alone for two reasons: > > 1. With the move to LSM syscalls for getting/setting a task's LSM > attributes we are "freezing" the procfs API and not adding any new > entries to it. > > 2. The BPF LSM doesn't currently register any procfs entries. > > I'd suggest leaving security_setprocattr() as-is and blocking it in > the BPF verifier, I can't see any reason why a BPF LSM would need > this hook. > OK, I'll drop this patch in the next version.
diff --git a/fs/proc/base.c b/fs/proc/base.c index 72a1acd03675..9e1cf6cc674d 100644 --- a/fs/proc/base.c +++ b/fs/proc/base.c @@ -2740,6 +2740,7 @@ static ssize_t proc_pid_attr_write(struct file * file, const char __user * buf, { struct inode * inode = file_inode(file); struct task_struct *task; + size_t wbytes; void *page; int rv; @@ -2785,12 +2786,12 @@ static ssize_t proc_pid_attr_write(struct file * file, const char __user * buf, rv = security_setprocattr(PROC_I(inode)->op.lsmid, file->f_path.dentry->d_name.name, page, - count); + count, &wbytes); mutex_unlock(¤t->signal->cred_guard_mutex); out_free: kfree(page); out: - return rv; + return rv < 0 ? rv : wbytes; } static const struct file_operations proc_pid_attr_operations = { diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h index dbc16f14f42f..2628514bb19c 100644 --- a/include/linux/lsm_hook_defs.h +++ b/include/linux/lsm_hook_defs.h @@ -287,7 +287,8 @@ LSM_HOOK(int, -EOPNOTSUPP, setselfattr, unsigned int attr, struct lsm_ctx *ctx, u32 size, u32 flags) LSM_HOOK(int, -EINVAL, getprocattr, struct task_struct *p, const char *name, char **value) -LSM_HOOK(int, -EINVAL, setprocattr, const char *name, void *value, size_t size) +LSM_HOOK(int, -EINVAL, setprocattr, const char *name, void *value, size_t size, + size_t *wbytes) LSM_HOOK(int, 0, ismaclabel, const char *name) LSM_HOOK(int, -EOPNOTSUPP, secid_to_secctx, u32 secid, char **secdata, u32 *seclen) diff --git a/include/linux/security.h b/include/linux/security.h index 96a63e132abf..1f1a9696e65d 100644 --- a/include/linux/security.h +++ b/include/linux/security.h @@ -496,7 +496,8 @@ int security_setselfattr(unsigned int attr, struct lsm_ctx __user *ctx, u32 size, u32 flags); int security_getprocattr(struct task_struct *p, int lsmid, const char *name, char **value); -int security_setprocattr(int lsmid, const char *name, void *value, size_t size); +int security_setprocattr(int lsmid, const char *name, void *value, size_t size, + size_t *wbytes); int security_netlink_send(struct sock *sk, struct sk_buff *skb); int security_ismaclabel(const char *name); int security_secid_to_secctx(u32 secid, char **secdata, u32 *seclen); @@ -1440,7 +1441,7 @@ static inline int security_getprocattr(struct task_struct *p, int lsmid, } static inline int security_setprocattr(int lsmid, char *name, void *value, - size_t size) + size_t size, size_t *wbytes) { return -EINVAL; } diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c index 72dd09993f28..6c8b1f8c5781 100644 --- a/security/apparmor/lsm.c +++ b/security/apparmor/lsm.c @@ -939,13 +939,17 @@ static int apparmor_setselfattr(unsigned int attr, struct lsm_ctx *ctx, } static int apparmor_setprocattr(const char *name, void *value, - size_t size) + size_t size, size_t *wbytes) { + int rc = -EINVAL; int attr = lsm_name_to_attr(name); if (attr) - return do_setattr(attr, value, size); - return -EINVAL; + rc = do_setattr(attr, value, size); + if (rc < 0) + return rc; + *wbytes = rc; + return 0; } /** diff --git a/security/security.c b/security/security.c index 095e78efcb32..9685096dbf16 100644 --- a/security/security.c +++ b/security/security.c @@ -4141,20 +4141,22 @@ int security_getprocattr(struct task_struct *p, int lsmid, const char *name, * @name: attribute name * @value: attribute value * @size: attribute value size + * @wbytes: bytes written on success * * Write (set) the current task's attribute @name to @value, size @size if * allowed. * - * Return: Returns bytes written on success, a negative value otherwise. + * Return: Returns 0 on success, a negative error code otherwise. */ -int security_setprocattr(int lsmid, const char *name, void *value, size_t size) +int security_setprocattr(int lsmid, const char *name, void *value, size_t size, + size_t *wbytes) { struct security_hook_list *hp; hlist_for_each_entry(hp, &security_hook_heads.setprocattr, list) { if (lsmid != 0 && lsmid != hp->lsmid->id) continue; - return hp->hook.setprocattr(name, value, size); + return hp->hook.setprocattr(name, value, size, wbytes); } return LSM_RET_DEFAULT(setprocattr); } diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index 0d35bb93baca..7a73f3710025 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -6589,13 +6589,18 @@ static int selinux_getprocattr(struct task_struct *p, return -EINVAL; } -static int selinux_setprocattr(const char *name, void *value, size_t size) +static int selinux_setprocattr(const char *name, void *value, size_t size, + size_t *wbytes) { + int rc = -EINVAL; int attr = lsm_name_to_attr(name); if (attr) - return selinux_lsm_setattr(attr, value, size); - return -EINVAL; + rc = selinux_lsm_setattr(attr, value, size); + if (rc < 0) + return rc; + *wbytes = rc; + return 0; } static int selinux_ismaclabel(const char *name) diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c index 63d9c5f456c1..4265f2639106 100644 --- a/security/smack/smack_lsm.c +++ b/security/smack/smack_lsm.c @@ -3797,19 +3797,25 @@ static int smack_setselfattr(unsigned int attr, struct lsm_ctx *ctx, * @name: the name of the attribute in /proc/.../attr * @value: the value to set * @size: the size of the value + * @wbytes: the length of the smack label written * * Sets the Smack value of the task. Only setting self * is permitted and only with privilege * - * Returns the length of the smack label or an error code + * Returns 0 on success or a negative error code */ -static int smack_setprocattr(const char *name, void *value, size_t size) +static int smack_setprocattr(const char *name, void *value, size_t size, + size_t *wbytes) { + int rc = -EINVAL; int attr = lsm_name_to_attr(name); if (attr != LSM_ATTR_UNDEF) - return do_setattr(attr, value, size); - return -EINVAL; + rc = do_setattr(attr, value, size); + if (rc < 0) + return rc; + *wbytes = rc; + return 0; } /**