In selinux_add_mnt_opt(), 'val' is allcoted by kmemdup_nul(). It returns
NULL when fails. So 'val' should be checked.
Signed-off-by: Gen Zhang <[email protected]>
---
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 3ec702c..4797c63 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -1052,8 +1052,11 @@ static int selinux_add_mnt_opt(const char *option, const char *val, int len,
if (token == Opt_error)
return -EINVAL;
- if (token != Opt_seclabel)
- val = kmemdup_nul(val, len, GFP_KERNEL);
+ if (token != Opt_seclabel) {
+ val = kmemdup_nul(val, len, GFP_KERNEL);
+ if (!val)
+ return -ENOMEM;
+ }
rc = selinux_add_opt(token, val, mnt_opts);
if (unlikely(rc)) {
kfree(val);
On Thu, May 30, 2019 at 10:06 AM Gen Zhang <[email protected]> wrote:
> In selinux_add_mnt_opt(), 'val' is allcoted by kmemdup_nul(). It returns
> NULL when fails. So 'val' should be checked.
>
> Signed-off-by: Gen Zhang <[email protected]>
Please add a Fixes tag here, too:
Fixes: 757cbe597fe8 ("LSM: new method: ->sb_add_mnt_opt()")
> ---
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 3ec702c..4797c63 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -1052,8 +1052,11 @@ static int selinux_add_mnt_opt(const char *option, const char *val, int len,
> if (token == Opt_error)
> return -EINVAL;
>
> - if (token != Opt_seclabel)
> - val = kmemdup_nul(val, len, GFP_KERNEL);
> + if (token != Opt_seclabel) {
> + val = kmemdup_nul(val, len, GFP_KERNEL);
> + if (!val)
> + return -ENOMEM;
There is one extra tab character in the above three lines ^^^
> + }
> rc = selinux_add_opt(token, val, mnt_opts);
> if (unlikely(rc)) {
> kfree(val);
Thanks,
--
Ondrej Mosnacek <omosnace at redhat dot com>
Software Engineer, Security Technologies
Red Hat, Inc.
In selinux_add_mnt_opt(), 'val' is allcoted by kmemdup_nul(). It returns
NULL when fails. So 'val' should be checked.
Signed-off-by: Gen Zhang <[email protected]>
Fixes: 757cbe597fe8 ("LSM: new method: ->sb_add_mnt_opt()")
---
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 3ec702c..4797c63 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -1052,8 +1052,11 @@ static int selinux_add_mnt_opt(const char *option, const char *val, int len,
if (token == Opt_error)
return -EINVAL;
- if (token != Opt_seclabel)
- val = kmemdup_nul(val, len, GFP_KERNEL);
+ if (token != Opt_seclabel) {
+ val = kmemdup_nul(val, len, GFP_KERNEL);
+ if (!val)
+ return -ENOMEM;
+ }
rc = selinux_add_opt(token, val, mnt_opts);
if (unlikely(rc)) {
kfree(val);
Hello!
On 30.05.2019 11:06, Gen Zhang wrote:
> In selinux_add_mnt_opt(), 'val' is allcoted by kmemdup_nul(). It returns
Allocated?
> NULL when fails. So 'val' should be checked.
>
> Signed-off-by: Gen Zhang <[email protected]>
[...]
MBR, Sergei
On Thu, May 30, 2019 at 12:11:33PM +0300, Sergei Shtylyov wrote:
> Hello!
>
> On 30.05.2019 11:06, Gen Zhang wrote:
>
> >In selinux_add_mnt_opt(), 'val' is allcoted by kmemdup_nul(). It returns
>
> Allocated?
Thanks for your reply, Sergei. I used 'allocated' because kmemdup_nul()
does some allocation in its implementation. And its docs descrips:
"Return: newly allocated copy of @s with NUL-termination or %NULL in
case of error". I think it is proper to use 'allocated' here. But it
could be 'assigned', which is better, right?
Thanks
Gen
>
> >NULL when fails. So 'val' should be checked.
> >
> >Signed-off-by: Gen Zhang <[email protected]>
> [...]
>
> MBR, Sergei
On 30.05.2019 12:18, Gen Zhang wrote:
>> On 30.05.2019 11:06, Gen Zhang wrote:
>>
>>> In selinux_add_mnt_opt(), 'val' is allcoted by kmemdup_nul(). It returns
>>
>> Allocated?
> Thanks for your reply, Sergei. I used 'allocated' because kmemdup_nul()
> does some allocation in its implementation. And its docs descrips:
Describes?
> "Return: newly allocated copy of @s with NUL-termination or %NULL in
> case of error". I think it is proper to use 'allocated' here. But it
> could be 'assigned', which is better, right?
I was only trying to point out the typos in this word. :-)
> Thanks
> Gen
>>
>>> NULL when fails. So 'val' should be checked.
>>>
>>> Signed-off-by: Gen Zhang <[email protected]>
>> [...]
MBR, Sergei
On Thu, May 30, 2019 at 12:22:15PM +0300, Sergei Shtylyov wrote:
> On 30.05.2019 12:18, Gen Zhang wrote:
>
> >>On 30.05.2019 11:06, Gen Zhang wrote:
> >>
> >>>In selinux_add_mnt_opt(), 'val' is allcoted by kmemdup_nul(). It returns
> >>
> >> Allocated?
>
> >Thanks for your reply, Sergei. I used 'allocated' because kmemdup_nul()
> >does some allocation in its implementation. And its docs descrips:
>
> Describes?
>
> >"Return: newly allocated copy of @s with NUL-termination or %NULL in
> >case of error". I think it is proper to use 'allocated' here. But it
> >could be 'assigned', which is better, right?
>
> I was only trying to point out the typos in this word. :-)
>
> >Thanks
> >Gen
> >>
> >>>NULL when fails. So 'val' should be checked.
> >>>
> >>>Signed-off-by: Gen Zhang <[email protected]>
> >>[...]
>
> MBR, Sergei
Well, my mistake. Thanks for your comments, Sergei!
Thanks
Gen
On Thu, May 30, 2019 at 4:55 AM Gen Zhang <[email protected]> wrote:
>
> In selinux_add_mnt_opt(), 'val' is allcoted by kmemdup_nul(). It returns
> NULL when fails. So 'val' should be checked.
>
> Signed-off-by: Gen Zhang <[email protected]>
> Fixes: 757cbe597fe8 ("LSM: new method: ->sb_add_mnt_opt()")
Previous comments regarding "selinux:" instead of "hooks:" apply here as well.
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 3ec702c..4797c63 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -1052,8 +1052,11 @@ static int selinux_add_mnt_opt(const char *option, const char *val, int len,
> if (token == Opt_error)
> return -EINVAL;
>
> - if (token != Opt_seclabel)
> - val = kmemdup_nul(val, len, GFP_KERNEL);
> + if (token != Opt_seclabel) {
> + val = kmemdup_nul(val, len, GFP_KERNEL);
> + if (!val)
> + return -ENOMEM;
It looks like this code is only ever called by NFS, which will
eventually clean up mnt_opts via security_free_mnt_opts(), but since
the selinux_add_opt() error handler below cleans up mnt_opts it might
be safer to do the same here in case this function is called multiple
times to add multiple options.
> + }
> rc = selinux_add_opt(token, val, mnt_opts);
> if (unlikely(rc)) {
> kfree(val);
--
paul moore
http://www.paul-moore.com
On Fri, May 31, 2019 at 11:55:23AM -0400, Paul Moore wrote:
> On Thu, May 30, 2019 at 4:55 AM Gen Zhang <[email protected]> wrote:
> >
> > In selinux_add_mnt_opt(), 'val' is allcoted by kmemdup_nul(). It returns
> > NULL when fails. So 'val' should be checked.
> >
> > Signed-off-by: Gen Zhang <[email protected]>
> > Fixes: 757cbe597fe8 ("LSM: new method: ->sb_add_mnt_opt()")
>
> Previous comments regarding "selinux:" instead of "hooks:" apply here as well.
>
Thanks for your comments, Paul. I will make some changes.
> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > index 3ec702c..4797c63 100644
> > --- a/security/selinux/hooks.c
> > +++ b/security/selinux/hooks.c
> > @@ -1052,8 +1052,11 @@ static int selinux_add_mnt_opt(const char *option, const char *val, int len,
> > if (token == Opt_error)
> > return -EINVAL;
> >
> > - if (token != Opt_seclabel)
> > - val = kmemdup_nul(val, len, GFP_KERNEL);
> > + if (token != Opt_seclabel) {
> > + val = kmemdup_nul(val, len, GFP_KERNEL);
> > + if (!val)
> > + return -ENOMEM;
>
> It looks like this code is only ever called by NFS, which will
> eventually clean up mnt_opts via security_free_mnt_opts(), but since
> the selinux_add_opt() error handler below cleans up mnt_opts it might
> be safer to do the same here in case this function is called multiple
> times to add multiple options.
>
> > + }
> > rc = selinux_add_opt(token, val, mnt_opts);
> > if (unlikely(rc)) {
> > kfree(val);
>
> --
> paul moore
> http://www.paul-moore.com
Thanks
Gen