| Message ID | 20220614132333.143042-1-xiujianfeng@huawei.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | [-next] selinux: Fix memleak in security_read_policy | expand |
On Tue, Jun 14, 2022 at 9:25 AM Xiu Jianfeng <xiujianfeng@huawei.com> wrote: > > In this function, it directly returns the result of __security_read_policy > without freeing the allocated memory in *data, cause memory leak issue, > so free the memory if __security_read_policy failed. > > Signed-off-by: Xiu Jianfeng <xiujianfeng@huawei.com> > --- > security/selinux/ss/services.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) This is another case where there is not actually a memory leak as the only caller of security_read_policy() is sel_open_policy() which will free the buffer it passes to security_read_policy() on error. If you want you could add a comment to security_read_policy() indicating that the caller is responsible for freeing the memory.
On Wed, Jun 15, 2022 at 4:03 AM Paul Moore <paul@paul-moore.com> wrote: > On Tue, Jun 14, 2022 at 9:25 AM Xiu Jianfeng <xiujianfeng@huawei.com> wrote: > > > > In this function, it directly returns the result of __security_read_policy > > without freeing the allocated memory in *data, cause memory leak issue, > > so free the memory if __security_read_policy failed. > > > > Signed-off-by: Xiu Jianfeng <xiujianfeng@huawei.com> > > --- > > security/selinux/ss/services.c | 9 ++++++++- > > 1 file changed, 8 insertions(+), 1 deletion(-) > > This is another case where there is not actually a memory leak as the > only caller of security_read_policy() is sel_open_policy() which will > free the buffer it passes to security_read_policy() on error. > > If you want you could add a comment to security_read_policy() > indicating that the caller is responsible for freeing the memory. Can we please not have two almost identical functions with different cleanup conventions? Please let's either make both functions guarantee cleanup on error or neither of them (adapting the caller(s) and comments accordingly).
On Wed, Jun 15, 2022 at 6:04 AM Ondrej Mosnacek <omosnace@redhat.com> wrote: > On Wed, Jun 15, 2022 at 4:03 AM Paul Moore <paul@paul-moore.com> wrote: > > On Tue, Jun 14, 2022 at 9:25 AM Xiu Jianfeng <xiujianfeng@huawei.com> wrote: > > > > > > In this function, it directly returns the result of __security_read_policy > > > without freeing the allocated memory in *data, cause memory leak issue, > > > so free the memory if __security_read_policy failed. > > > > > > Signed-off-by: Xiu Jianfeng <xiujianfeng@huawei.com> > > > --- > > > security/selinux/ss/services.c | 9 ++++++++- > > > 1 file changed, 8 insertions(+), 1 deletion(-) > > > > This is another case where there is not actually a memory leak as the > > only caller of security_read_policy() is sel_open_policy() which will > > free the buffer it passes to security_read_policy() on error. > > > > If you want you could add a comment to security_read_policy() > > indicating that the caller is responsible for freeing the memory. > > Can we please not have two almost identical functions with different > cleanup conventions? Please let's either make both functions guarantee > cleanup on error or neither of them (adapting the caller(s) and > comments accordingly). Priorities Ondrej, priorities. Every patch posted to the list has a time and effort cost associated with it, and between reviewing other more important patches and working on a proper SCTP/SELinux fix, I simply don't have the cycles to spend doing the back-and-forth on a patch like this to fix a memory leak that doesn't exist. It definitely isn't because I don't think the code could be improved, it is just that there are only so many hours in a day and I need to prioritize actual bugs and important new features that people want merged. ... oh, and I need to reply to the complaints too, that's always the highlight of my day.
diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c index 69b2734311a6..78afda6a36b8 100644 --- a/security/selinux/ss/services.c +++ b/security/selinux/ss/services.c @@ -4018,6 +4018,7 @@ static int __security_read_policy(struct selinux_policy *policy, int security_read_policy(struct selinux_state *state, void **data, size_t *len) { + int err; struct selinux_policy *policy; policy = rcu_dereference_protected( @@ -4030,7 +4031,13 @@ int security_read_policy(struct selinux_state *state, if (!*data) return -ENOMEM; - return __security_read_policy(policy, *data, len); + err = __security_read_policy(policy, *data, len); + if (err) { + vfree(*data); + *data = NULL; + *len = 0; + } + return err; } /**
In this function, it directly returns the result of __security_read_policy without freeing the allocated memory in *data, cause memory leak issue, so free the memory if __security_read_policy failed. Signed-off-by: Xiu Jianfeng <xiujianfeng@huawei.com> --- security/selinux/ss/services.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)