| Message ID | 20240128100035.82170-1-debug.penguin32@gmail.com (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | selinux: Use kfree_sensitive for certain code paths of security | expand |
On 1/28/2024 2:00 AM, Ronald Monthero wrote: > As with kfree_sensitive() it does kfree() and memzero_explicit() > internally to clear sensitive data. The patch includes some of > the code paths to free data such as keys, hash table and > scontext and tcontext of selinux, which would benefit > from kfree_sensitive() to replace kfree() What about the data being freed makes it "sensitive"? Generally security attributes are not themselves considered sensitive. > > Signed-off-by: Ronald Monthero <debug.penguin32@gmail.com> > --- > security/selinux/avc.c | 4 ++-- > security/selinux/ima.c | 2 +- > security/selinux/selinuxfs.c | 16 ++++++++-------- > security/selinux/ss/conditional.c | 4 ++-- > security/selinux/ss/hashtab.c | 2 +- > security/selinux/ss/policydb.c | 6 +++--- > 6 files changed, 17 insertions(+), 17 deletions(-) > > diff --git a/security/selinux/avc.c b/security/selinux/avc.c > index 32eb67fb3e42..c6d7f52a11c1 100644 > --- a/security/selinux/avc.c > +++ b/security/selinux/avc.c > @@ -723,8 +723,8 @@ static void avc_audit_post_callback(struct audit_buffer *ab, void *a) > audit_log_format(ab, " permissive=%u", sad->result ? 0 : 1); > > trace_selinux_audited(sad, scontext, tcontext, tclass); > - kfree(tcontext); > - kfree(scontext); > + kfree_sensitive(tcontext); > + kfree_sensitive(scontext); > > /* in case of invalid context report also the actual context string */ > rc = security_sid_to_context_inval(sad->ssid, &scontext, > diff --git a/security/selinux/ima.c b/security/selinux/ima.c > index aa34da9b0aeb..b5d923d272a0 100644 > --- a/security/selinux/ima.c > +++ b/security/selinux/ima.c > @@ -86,7 +86,7 @@ void selinux_ima_measure_state_locked(void) > state_str, strlen(state_str), false, > NULL, 0); > > - kfree(state_str); > + kfree_sensitive(state_str); > > /* > * Measure SELinux policy only after initialization is completed. > diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c > index 6c596ae7fef9..6c2490dbfaa6 100644 > --- a/security/selinux/selinuxfs.c > +++ b/security/selinux/selinuxfs.c > @@ -183,7 +183,7 @@ static ssize_t sel_write_enforce(struct file *file, const char __user *buf, > } > length = count; > out: > - kfree(page); > + kfree_sensitive(page); > return length; > } > #else > @@ -299,7 +299,7 @@ static ssize_t sel_write_disable(struct file *file, const char __user *buf, > } > > out: > - kfree(page); > + kfree_sensitive(page); > return length; > } > > @@ -725,7 +725,7 @@ static ssize_t sel_write_checkreqprot(struct file *file, const char __user *buf, > selinux_ima_measure_state(); > > out: > - kfree(page); > + kfree_sensitive(page); > return length; > } > static const struct file_operations sel_checkreqprot_ops = { > @@ -1290,7 +1290,7 @@ static ssize_t sel_write_bool(struct file *filep, const char __user *buf, > > out: > mutex_unlock(&selinux_state.policy_mutex); > - kfree(page); > + kfree_sensitive(page); > return length; > } > > @@ -1342,7 +1342,7 @@ static ssize_t sel_commit_bools_write(struct file *filep, > > out: > mutex_unlock(&selinux_state.policy_mutex); > - kfree(page); > + kfree_sensitive(page); > return length; > } > > @@ -1481,7 +1481,7 @@ static ssize_t sel_write_avc_cache_threshold(struct file *file, > > ret = count; > out: > - kfree(page); > + kfree_sensitive(page); > return ret; > } > > @@ -1831,7 +1831,7 @@ static int sel_make_perm_files(struct selinux_policy *newpolicy, > out: > for (i = 0; i < nperms; i++) > kfree(perms[i]); > - kfree(perms); > + kfree_sensitive(perms); > return rc; > } > > @@ -1900,7 +1900,7 @@ static int sel_make_classes(struct selinux_policy *newpolicy, > out: > for (i = 0; i < nclasses; i++) > kfree(classes[i]); > - kfree(classes); > + kfree_sensitive(classes); > return rc; > } > > diff --git a/security/selinux/ss/conditional.c b/security/selinux/ss/conditional.c > index 81ff676f209a..51409a74eb74 100644 > --- a/security/selinux/ss/conditional.c > +++ b/security/selinux/ss/conditional.c > @@ -176,8 +176,8 @@ int cond_init_bool_indexes(struct policydb *p) > > int cond_destroy_bool(void *key, void *datum, void *p) > { > - kfree(key); > - kfree(datum); > + kfree_sensitive(key); > + kfree_sensitive(datum); > return 0; > } > > diff --git a/security/selinux/ss/hashtab.c b/security/selinux/ss/hashtab.c > index c05d8346a94a..73b867fa4726 100644 > --- a/security/selinux/ss/hashtab.c > +++ b/security/selinux/ss/hashtab.c > @@ -79,7 +79,7 @@ void hashtab_destroy(struct hashtab *h) > h->htable[i] = NULL; > } > > - kfree(h->htable); > + kfree_sensitive(h->htable); > h->htable = NULL; > } > > diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c > index 595a435ea9c8..fd58a6a1ef8f 100644 > --- a/security/selinux/ss/policydb.c > +++ b/security/selinux/ss/policydb.c > @@ -179,8 +179,8 @@ static const struct policydb_compat_info *policydb_lookup_compat(unsigned int ve > > static int perm_destroy(void *key, void *datum, void *p) > { > - kfree(key); > - kfree(datum); > + kfree_sensitive(key); > + kfree_sensitive(datum); > return 0; > } > > @@ -369,7 +369,7 @@ static void ocontext_destroy(struct ocontext *c, unsigned int i) > if (i == OCON_ISID || i == OCON_FS || > i == OCON_NETIF || i == OCON_FSUSE) > kfree(c->u.name); > - kfree(c); > + kfree_sensitive(c); > } > > /*
On Sun, Jan 28, 2024 at 5:02 AM Ronald Monthero <debug.penguin32@gmail.com> wrote: > > As with kfree_sensitive() it does kfree() and memzero_explicit() > internally to clear sensitive data. The patch includes some of > the code paths to free data such as keys, hash table and > scontext and tcontext of selinux, which would benefit > from kfree_sensitive() to replace kfree() > > Signed-off-by: Ronald Monthero <debug.penguin32@gmail.com> > --- > security/selinux/avc.c | 4 ++-- > security/selinux/ima.c | 2 +- > security/selinux/selinuxfs.c | 16 ++++++++-------- > security/selinux/ss/conditional.c | 4 ++-- > security/selinux/ss/hashtab.c | 2 +- > security/selinux/ss/policydb.c | 6 +++--- > 6 files changed, 17 insertions(+), 17 deletions(-) I agree with Casey's comment, I'm not seeing anything in the SELinux code that would warrant the use of kfree_sensitive().
Thanks Paul and Casey for taking the time to review. I'll let that be, as it may not be needed until further particularity. BR, ronald On Tue, Jan 30, 2024 at 7:58 AM Paul Moore <paul@paul-moore.com> wrote: > > On Sun, Jan 28, 2024 at 5:02 AM Ronald Monthero > <debug.penguin32@gmail.com> wrote: > > > > As with kfree_sensitive() it does kfree() and memzero_explicit() > > internally to clear sensitive data. The patch includes some of > > the code paths to free data such as keys, hash table and > > scontext and tcontext of selinux, which would benefit > > from kfree_sensitive() to replace kfree() > > > > Signed-off-by: Ronald Monthero <debug.penguin32@gmail.com> > > --- > > security/selinux/avc.c | 4 ++-- > > security/selinux/ima.c | 2 +- > > security/selinux/selinuxfs.c | 16 ++++++++-------- > > security/selinux/ss/conditional.c | 4 ++-- > > security/selinux/ss/hashtab.c | 2 +- > > security/selinux/ss/policydb.c | 6 +++--- > > 6 files changed, 17 insertions(+), 17 deletions(-) > > I agree with Casey's comment, I'm not seeing anything in the SELinux > code that would warrant the use of kfree_sensitive(). > > -- > paul-moore.com
diff --git a/security/selinux/avc.c b/security/selinux/avc.c index 32eb67fb3e42..c6d7f52a11c1 100644 --- a/security/selinux/avc.c +++ b/security/selinux/avc.c @@ -723,8 +723,8 @@ static void avc_audit_post_callback(struct audit_buffer *ab, void *a) audit_log_format(ab, " permissive=%u", sad->result ? 0 : 1); trace_selinux_audited(sad, scontext, tcontext, tclass); - kfree(tcontext); - kfree(scontext); + kfree_sensitive(tcontext); + kfree_sensitive(scontext); /* in case of invalid context report also the actual context string */ rc = security_sid_to_context_inval(sad->ssid, &scontext, diff --git a/security/selinux/ima.c b/security/selinux/ima.c index aa34da9b0aeb..b5d923d272a0 100644 --- a/security/selinux/ima.c +++ b/security/selinux/ima.c @@ -86,7 +86,7 @@ void selinux_ima_measure_state_locked(void) state_str, strlen(state_str), false, NULL, 0); - kfree(state_str); + kfree_sensitive(state_str); /* * Measure SELinux policy only after initialization is completed. diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c index 6c596ae7fef9..6c2490dbfaa6 100644 --- a/security/selinux/selinuxfs.c +++ b/security/selinux/selinuxfs.c @@ -183,7 +183,7 @@ static ssize_t sel_write_enforce(struct file *file, const char __user *buf, } length = count; out: - kfree(page); + kfree_sensitive(page); return length; } #else @@ -299,7 +299,7 @@ static ssize_t sel_write_disable(struct file *file, const char __user *buf, } out: - kfree(page); + kfree_sensitive(page); return length; } @@ -725,7 +725,7 @@ static ssize_t sel_write_checkreqprot(struct file *file, const char __user *buf, selinux_ima_measure_state(); out: - kfree(page); + kfree_sensitive(page); return length; } static const struct file_operations sel_checkreqprot_ops = { @@ -1290,7 +1290,7 @@ static ssize_t sel_write_bool(struct file *filep, const char __user *buf, out: mutex_unlock(&selinux_state.policy_mutex); - kfree(page); + kfree_sensitive(page); return length; } @@ -1342,7 +1342,7 @@ static ssize_t sel_commit_bools_write(struct file *filep, out: mutex_unlock(&selinux_state.policy_mutex); - kfree(page); + kfree_sensitive(page); return length; } @@ -1481,7 +1481,7 @@ static ssize_t sel_write_avc_cache_threshold(struct file *file, ret = count; out: - kfree(page); + kfree_sensitive(page); return ret; } @@ -1831,7 +1831,7 @@ static int sel_make_perm_files(struct selinux_policy *newpolicy, out: for (i = 0; i < nperms; i++) kfree(perms[i]); - kfree(perms); + kfree_sensitive(perms); return rc; } @@ -1900,7 +1900,7 @@ static int sel_make_classes(struct selinux_policy *newpolicy, out: for (i = 0; i < nclasses; i++) kfree(classes[i]); - kfree(classes); + kfree_sensitive(classes); return rc; } diff --git a/security/selinux/ss/conditional.c b/security/selinux/ss/conditional.c index 81ff676f209a..51409a74eb74 100644 --- a/security/selinux/ss/conditional.c +++ b/security/selinux/ss/conditional.c @@ -176,8 +176,8 @@ int cond_init_bool_indexes(struct policydb *p) int cond_destroy_bool(void *key, void *datum, void *p) { - kfree(key); - kfree(datum); + kfree_sensitive(key); + kfree_sensitive(datum); return 0; } diff --git a/security/selinux/ss/hashtab.c b/security/selinux/ss/hashtab.c index c05d8346a94a..73b867fa4726 100644 --- a/security/selinux/ss/hashtab.c +++ b/security/selinux/ss/hashtab.c @@ -79,7 +79,7 @@ void hashtab_destroy(struct hashtab *h) h->htable[i] = NULL; } - kfree(h->htable); + kfree_sensitive(h->htable); h->htable = NULL; } diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c index 595a435ea9c8..fd58a6a1ef8f 100644 --- a/security/selinux/ss/policydb.c +++ b/security/selinux/ss/policydb.c @@ -179,8 +179,8 @@ static const struct policydb_compat_info *policydb_lookup_compat(unsigned int ve static int perm_destroy(void *key, void *datum, void *p) { - kfree(key); - kfree(datum); + kfree_sensitive(key); + kfree_sensitive(datum); return 0; } @@ -369,7 +369,7 @@ static void ocontext_destroy(struct ocontext *c, unsigned int i) if (i == OCON_ISID || i == OCON_FS || i == OCON_NETIF || i == OCON_FSUSE) kfree(c->u.name); - kfree(c); + kfree_sensitive(c); } /*
As with kfree_sensitive() it does kfree() and memzero_explicit() internally to clear sensitive data. The patch includes some of the code paths to free data such as keys, hash table and scontext and tcontext of selinux, which would benefit from kfree_sensitive() to replace kfree() Signed-off-by: Ronald Monthero <debug.penguin32@gmail.com> --- security/selinux/avc.c | 4 ++-- security/selinux/ima.c | 2 +- security/selinux/selinuxfs.c | 16 ++++++++-------- security/selinux/ss/conditional.c | 4 ++-- security/selinux/ss/hashtab.c | 2 +- security/selinux/ss/policydb.c | 6 +++--- 6 files changed, 17 insertions(+), 17 deletions(-)