Re: [PATCH] ipv6_sockglue: Fix a missing-check bug in ip6_ra_control()

[Gen Zhang]

On Mon, Jul 01, 2019 at 10:57:36AM +0200, Jiri Slaby wrote:
> On 24. 05. 19, 5:19, Gen Zhang wrote:
> > In function ip6_ra_control(), the pointer new_ra is allocated a memory 
> > space via kmalloc(). And it is used in the following codes. However, 
> > when there is a memory allocation error, kmalloc() fails. Thus null 
> > pointer dereference may happen. And it will cause the kernel to crash. 
> > Therefore, we should check the return value and handle the error.
> > 
> > Signed-off-by: Gen Zhang 
> > 
> > ---
> > diff --git a/net/ipv6/ipv6_sockglue.c b/net/ipv6/ipv6_sockglue.c
> > index 40f21fe..0a3d035 100644
> > --- a/net/ipv6/ipv6_sockglue.c
> > +++ b/net/ipv6/ipv6_sockglue.c
> > @@ -68,6 +68,8 @@ int ip6_ra_control(struct sock *sk, int sel)
> > return -ENOPROTOOPT;
> >  
> > new_ra = (sel >= 0) ? kmalloc(sizeof(*new_ra), GFP_KERNEL) : NULL;
> > +   if (sel >= 0 && !new_ra)
> > +   return -ENOMEM;
> >  
> > write_lock_bh(_ra_lock);
> > for (rap = _ra_chain; (ra = *rap) != NULL; rap = >next) {
> > 
> 
> Was this really an omission? There is (!new_ra) handling below the for loop:
> if (!new_ra) {
> write_unlock_bh(_ra_lock);
> return -ENOBUFS;
> }
> 
> It used to handle both (sel >= 0) and (sel == 0) cases and it used to
> return ENOBUFS in case of failure. For (sel >= 0) it also could at least
> return EADDRINUSE when a collision was found -- even if memory was
> exhausted.
> 
> In anyway, how could this lead to a pointer dereference? And why/how did
> this get a CVE number?
> 
> thanks,
> -- 
> js
> suse labs
This CVE is already disputed by other maintainers and marked *DISPUTED*
on the website.

Thanks
Gen

Re: [PATCH] tegra-hsp: fix a missing-check bug in tegra_hsp_doorbell_create()

[Gen Zhang]

On Sun, Jun 09, 2019 at 12:11:44AM +0800, Gen Zhang wrote:
> On Thu, May 30, 2019 at 09:19:20AM +0800, Gen Zhang wrote:
> > In tegra_hsp_doorbell_create(), 'db->name' is allocated by 
> > devm_kstrdup_const(). It returns NULL when fails. So 'db->name' should
> > be checked.
> > 
> > Signed-off-by: Gen Zhang 
> > Acked-by: Thierry Reding 
> > ---
> > diff --git a/drivers/mailbox/tegra-hsp.c b/drivers/mailbox/tegra-hsp.c
> > index 11fc9fd..b613c46 100644
> > --- a/drivers/mailbox/tegra-hsp.c
> > +++ b/drivers/mailbox/tegra-hsp.c
> > @@ -292,6 +292,8 @@ tegra_hsp_doorbell_create(struct tegra_hsp *hsp, const 
> > char *name,
> > db->channel.hsp = hsp;
> >  
> > db->name = devm_kstrdup_const(hsp->dev, name, GFP_KERNEL);
> > +   if (!db->name)
> > +   return ERR_PTR(-ENOMEM);
> > db->master = master;
> > db->index = index;
> >  
> > ---
> Can anyone look into this patch? It's acked by Thierry Reding 
> .
> 
> Thanks
> Gen
Could anyone look into this patch? It's not updated for about 20 days.
And I am really on a deadline to get this patch applied.

Thanks
Gen

[PATCH v5] selinux: fix a missing-check bug in selinux_sb_eat_lsm_opts()

[Gen Zhang]

In selinux_sb_eat_lsm_opts(), 'arg' is allocated by kmemdup_nul(). It
returns NULL when fails. So 'arg' should be checked. And 'mnt_opts' 
should be freed when error.

Signed-off-by: Gen Zhang 
Fixes: 99dbbb593fe6 ("selinux: rewrite selinux_sb_eat_lsm_opts()")
---
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 3ec702c..7a56f2a 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -2616,10 +2616,11 @@ static int selinux_sb_eat_lsm_opts(char *options, void 
**mnt_opts)
char *from = options;
char *to = options;
bool first = true;
+   int rc;
 
while (1) {
int len = opt_len(from);
-   int token, rc;
+   int token;
char *arg = NULL;
 
token = match_opt_prefix(from, len, );
@@ -2635,15 +2636,15 @@ static int selinux_sb_eat_lsm_opts(char *options, void 
**mnt_opts)
*q++ = c;
}
arg = kmemdup_nul(arg, q - arg, GFP_KERNEL);
+   if (!arg) {
+   rc = -ENOMEM;
+   goto free_opt;
+   }
}
rc = selinux_add_opt(token, arg, mnt_opts);
if (unlikely(rc)) {
kfree(arg);
-   if (*mnt_opts) {
-   selinux_free_mnt_opts(*mnt_opts);
-   *mnt_opts = NULL;
-   }
-   return rc;
+   goto free_opt;
}
} else {
if (!first) {   // copy with preceding comma
@@ -2661,6 +2662,13 @@ static int selinux_sb_eat_lsm_opts(char *options, void 
**mnt_opts)
}
*to = '\0';
return 0;
+
+free_opt:
+   if (*mnt_opts) {
+   selinux_free_mnt_opts(*mnt_opts);
+   *mnt_opts = NULL;
+   }
+   return rc;
 }
 
 static int selinux_sb_remount(struct super_block *sb, void *mnt_opts)

[PATCH v5] selinux: fix a missing-check bug in selinux_sb_eat_lsm_opts()

[Gen Zhang]

In selinux_sb_eat_lsm_opts(), 'arg' is allocated by kmemdup_nul(). It
returns NULL when fails. So 'arg' should be checked. And 'mnt_opts' 
should be freed when error.

Signed-off-by: Gen Zhang 
Fixes: 99dbbb593fe6 ("selinux: rewrite selinux_sb_eat_lsm_opts()")
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 3ec702c..7a56f2a 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -2616,10 +2616,11 @@ static int selinux_sb_eat_lsm_opts(char *options, void 
**mnt_opts)
char *from = options;
char *to = options;
bool first = true;
+   int rc;
 
while (1) {
int len = opt_len(from);
-   int token, rc;
+   int token;
char *arg = NULL;
 
token = match_opt_prefix(from, len, );
@@ -2635,15 +2636,15 @@ static int selinux_sb_eat_lsm_opts(char *options, void 
**mnt_opts)
*q++ = c;
}
arg = kmemdup_nul(arg, q - arg, GFP_KERNEL);
+   if (!arg) {
+   rc = -ENOMEM;
+   goto free_opt;
+   }
}
rc = selinux_add_opt(token, arg, mnt_opts);
if (unlikely(rc)) {
kfree(arg);
-   if (*mnt_opts) {
-   selinux_free_mnt_opts(*mnt_opts);
-   *mnt_opts = NULL;
-   }
-   return rc;
+   goto free_opt;
}
} else {
if (!first) {   // copy with preceding comma
@@ -2661,6 +2662,13 @@ static int selinux_sb_eat_lsm_opts(char *options, void 
**mnt_opts)
}
*to = '\0';
return 0;
+
+free_opt:
+   if (*mnt_opts) {
+   selinux_free_mnt_opts(*mnt_opts);
+   *mnt_opts = NULL;
+   }
+   return rc;
 }
 
 static int selinux_sb_remount(struct super_block *sb, void *mnt_opts)

Re: [PATCH v4] vt: fix a missing-check bug in con_init()

[Gen Zhang]

On Wed, Jun 12, 2019 at 03:38:38PM +0200, Greg KH wrote:
> On Wed, Jun 12, 2019 at 09:15:06PM +0800, Gen Zhang wrote:
> > In function con_init(), the pointer variable vc_cons[currcons].d, vc and 
> > vc->vc_screenbuf is allocated by kzalloc(). However, kzalloc() returns 
> > NULL when fails. Therefore, we should check the return value and handle 
> > the error.
> > 
> > Signed-off-by: Gen Zhang 
> > ---
> 
> What changed from v1, v2, and v3?
Thanks for your timely response. I am not a native English speaker, so
I am not sure I understand this correctly. Does this mean that I should
use "v5", rather than "v4"? 
> 
> That always goes below the --- line.
And I can't see what goes wrong with "---". Could you please make some
explaination?

Thanks
Gen
> 
> v5 please.
> 
> thanks,
> 
> greg k-h

[PATCH v4] selinux: fix a missing-check bug in selinux_add_mnt_opt( )

[Gen Zhang]

In selinux_add_mnt_opt(), 'val' is allocated by kmemdup_nul(). It returns
NULL when fails. So 'val' should be checked. And 'mnt_opts' should be 
freed when error.

Signed-off-by: Gen Zhang 
Fixes: 757cbe597fe8 ("LSM: new method: ->sb_add_mnt_opt()")
---
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 3ec702c..b4b888e 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -1052,15 +1052,24 @@ 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) {
+   rc = -ENOMEM;
+   goto free_opt;
+   }
+   }
rc = selinux_add_opt(token, val, mnt_opts);
if (unlikely(rc)) {
kfree(val);
-   if (*mnt_opts) {
-   selinux_free_mnt_opts(*mnt_opts);
-   *mnt_opts = NULL;
-   }
+   goto free_opt;
+   }
+   return rc;
+
+free_opt:
+   if (*mnt_opts) {
+   selinux_free_mnt_opts(*mnt_opts);
+   *mnt_opts = NULL;
}
return rc;
 }

[PATCH v4] vt: fix a missing-check bug in con_init()

[Gen Zhang]

In function con_init(), the pointer variable vc_cons[currcons].d, vc and 
vc->vc_screenbuf is allocated by kzalloc(). However, kzalloc() returns 
NULL when fails. Therefore, we should check the return value and handle 
the error.

Signed-off-by: Gen Zhang 
---
diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
index 5c0ca1c..dc40e29 100644
--- a/drivers/tty/vt/vt.c
+++ b/drivers/tty/vt/vt.c
@@ -3357,10 +3357,14 @@ static int __init con_init(void)
 
for (currcons = 0; currcons < MIN_NR_CONSOLES; currcons++) {
vc_cons[currcons].d = vc = kzalloc(sizeof(struct vc_data), 
GFP_NOWAIT);
+   if (!vc)
+   goto vc_err;
INIT_WORK(_cons[currcons].SAK_work, vc_SAK);
tty_port_init(>port);
visual_init(vc, currcons, 1);
vc->vc_screenbuf = kzalloc(vc->vc_screenbuf_size, GFP_NOWAIT);
+   if (!vc->vc_screenbuf)
+   goto vc_screenbuf_err;
vc_init(vc, vc->vc_rows, vc->vc_cols,
currcons || !vc->vc_sw->con_save_screen);
}
@@ -3382,6 +3386,16 @@ static int __init con_init(void)
register_console(_console_driver);
 #endif
return 0;
+vc_err:
+   while (currcons > 0) {
+   currcons--;
+   kfree(vc_cons[currcons].d->vc_screenbuf);
+vc_screenbuf_err:
+   kfree(vc_cons[currcons].d);
+   vc_cons[currcons].d = NULL;
+   }
+   console_unlock();
+   return -ENOMEM;
 }
 console_initcall(con_init);
 

Re: [PATCH] knav_qmss_queue: fix a missing-check bug in knav_pool_create()

[Gen Zhang]

On Tue, Jun 11, 2019 at 10:54:15AM +0100, Marc Zyngier wrote:
> Hi Gen,
> 
> No idea why I'm being cc'd on this but hey... ;-)
I copied email address ftom thid commit:-)
https://github.com/torvalds/linux/commit/832ad0e3da4510fd17f98804abe512ea9a747035#diff-f2a24befc247191f4b81af93e9336785
> 
> On 11/06/2019 10:37, Gen Zhang wrote:
> > On Thu, May 30, 2019 at 11:39:49AM +0800, Gen Zhang wrote:
> >> In knav_pool_create(), 'pool->name' is allocated by kstrndup(). It
> >> returns NULL when fails. So 'pool->name' should be checked. And free
> >> 'pool' when error.
> >>
> >> Signed-off-by: Gen Zhang 
> >> ---
> >> diff --git a/drivers/soc/ti/knav_qmss_queue.c 
> >> b/drivers/soc/ti/knav_qmss_queue.c
> >> index 8b41837..0f8cb28 100644
> >> --- a/drivers/soc/ti/knav_qmss_queue.c
> >> +++ b/drivers/soc/ti/knav_qmss_queue.c
> >> @@ -814,6 +814,12 @@ void *knav_pool_create(const char *name,
> >>}
> >>  
> >>pool->name = kstrndup(name, KNAV_NAME_SIZE - 1, GFP_KERNEL);
> >> +  if (!pool->name) {
> >> +  dev_err(kdev->dev, "failed to duplicate for pool(%s)\n",
> >> +  name);
> 
> There is no need to output anything, the kernel will be loud enough if
> you run out of memory.
Thanks for your comments.
> 
> >> +  ret = -ENOMEM;
> >> +  goto err_name;
> >> +  }
> >>pool->kdev = kdev;
> >>pool->dev = kdev->dev;
> >>  
> >> @@ -864,6 +870,7 @@ void *knav_pool_create(const char *name,
> >>mutex_unlock(_dev_lock);
> >>  err:
> >>kfree(pool->name);
> >> +err_name:
> 
> kfree(NULL) is perfectly valid, there is no need to create a second
> label. Just branch to the existing error label.
Sure, better not to add redundant codes.
> 
> >>devm_kfree(kdev->dev, pool);
> >>return ERR_PTR(ret);
> >>  }
> > Can anyone look into this patch?
> > 
> > Thanks
> > Gen
> > 
> 
> The real question is whether this is actually an error at all.
> pool->name doesn't seem to be used for anything but debug information,
> and the printing code can perfectly accommodate a NULL pointer.
That sounds reasonable. This patch just fixes a *theoretical* bug.

Thanks
Gen
> 
> Thanks,
> 
>   M.
> -- 
> Jazz is not dead. It just smells funny...

Re: [PATCH] knav_qmss_queue: fix a missing-check bug in knav_pool_create()

[Gen Zhang]

On Thu, May 30, 2019 at 11:39:49AM +0800, Gen Zhang wrote:
> In knav_pool_create(), 'pool->name' is allocated by kstrndup(). It
> returns NULL when fails. So 'pool->name' should be checked. And free
> 'pool' when error.
> 
> Signed-off-by: Gen Zhang 
> ---
> diff --git a/drivers/soc/ti/knav_qmss_queue.c 
> b/drivers/soc/ti/knav_qmss_queue.c
> index 8b41837..0f8cb28 100644
> --- a/drivers/soc/ti/knav_qmss_queue.c
> +++ b/drivers/soc/ti/knav_qmss_queue.c
> @@ -814,6 +814,12 @@ void *knav_pool_create(const char *name,
>   }
>  
>   pool->name = kstrndup(name, KNAV_NAME_SIZE - 1, GFP_KERNEL);
> + if (!pool->name) {
> + dev_err(kdev->dev, "failed to duplicate for pool(%s)\n",
> + name);
> + ret = -ENOMEM;
> + goto err_name;
> + }
>   pool->kdev = kdev;
>   pool->dev = kdev->dev;
>  
> @@ -864,6 +870,7 @@ void *knav_pool_create(const char *name,
>   mutex_unlock(_dev_lock);
>  err:
>   kfree(pool->name);
> +err_name:
>   devm_kfree(kdev->dev, pool);
>   return ERR_PTR(ret);
>  }
Can anyone look into this patch?

Thanks
Gen

Re: [PATCH v4] selinux: lsm: fix a missing-check bug in selinux_sb_eat_lsm_o pts()

[Gen Zhang]

On Mon, Jun 10, 2019 at 04:20:28PM -0400, Paul Moore wrote:
> On Fri, Jun 7, 2019 at 4:41 AM Ondrej Mosnacek  wrote:
> >
> > On Thu, Jun 6, 2019 at 10:55 AM Gen Zhang  wrote:
> > > In selinux_sb_eat_lsm_opts(), 'arg' is allocated by kmemdup_nul(). It
> > > returns NULL when fails. So 'arg' should be checked. And 'mnt_opts'
> > > should be freed when error.
> > >
> > > Signed-off-by: Gen Zhang 
> > > Fixes: 99dbbb593fe6 ("selinux: rewrite selinux_sb_eat_lsm_opts()")
> >
> > My comments about the subject and an empty line before label apply
> > here as well, but Paul can fix both easily when applying ...
> 
> Since we've been discussing general best practices for submitting
> patches in this thread (and the other related thread), I wanted to
> (re)clarify my thoughts around maintainers fixing patches when merging
> them upstream.
> 
> When in doubt, do not ever rely on the upstream maintainer fixing your
> patch while merging it, and if problems do arise during review, it is
> best to not ask the maintainer to fix them for you, but for you to fix
> them instead (you are the patch author after all!).  Similarly, making
> comments along the lines of "X can fix both easily when applying", is
> also a bad thing to say when reviewing patches.  It's the patch
> author's responsibility to fix the patch by address review comments,
> not the maintainer.  I'll typically let you know if you don't need to
> rework a patch(set).
> 
> That said, there are times when the maintainer will change the patch
> during merging, most of which are due to resolving merge
> conflicts/fuzz with changes already in the tree (that *is* the
> maintainer's responsibility).  Speaking for myself, sometimes I will
> also make some minor changes if the patch author is away, or
> unreliable, or if there is a hard deadline near and I'm worried that
> the updated patch might not be ready in time.  I'll also sometimes
> make the changes directly if the patch is holding up a larger, more
> important patch(set), but that is really rare.  I'm sure I've made
> changes for other reasons in the past, and I'm sure I'll make changes
> for other reasons in the future, but hopefully this will give you a
> better idea of how the process works :)
> 
> -- 
> paul moore
> www.paul-moore.com
Thanks for your comments. I will resend a patch after revising.

Thanks
Gen

Re: [PATCH v3] selinux: lsm: fix a missing-check bug in selinux_add_mnt_opt( )

[Gen Zhang]

On Mon, Jun 10, 2019 at 03:31:50PM -0400, Paul Moore wrote:
> On Fri, Jun 7, 2019 at 8:11 AM Gen Zhang  wrote:
> >
> > On Fri, Jun 07, 2019 at 10:39:05AM +0200, Ondrej Mosnacek wrote:
> > > On Thu, Jun 6, 2019 at 11:23 AM Gen Zhang  
> > > wrote:
> > > > In selinux_add_mnt_opt(), 'val' is allocated by kmemdup_nul(). It 
> > > > returns
> > > > NULL when fails. So 'val' should be checked. And 'mnt_opts' should be
> > > > freed when error.
> > > >
> > > > Signed-off-by: Gen Zhang 
> > > > Fixes: 757cbe597fe8 ("LSM: new method: ->sb_add_mnt_opt()")
> > > > ---
> > > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > > > index 3ec702c..4e4c1c6 100644
> > > > --- a/security/selinux/hooks.c
> > > > +++ b/security/selinux/hooks.c
> > > > @@ -1052,15 +1052,23 @@ 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) {
> > > > +   rc = -ENOMEM;
> > > > +   goto free_opt;
> > > > +   }
> > > > +   }
> > > > rc = selinux_add_opt(token, val, mnt_opts);
> > > > if (unlikely(rc)) {
> > > > kfree(val);
> > > > -   if (*mnt_opts) {
> > > > -   selinux_free_mnt_opts(*mnt_opts);
> > > > -   *mnt_opts = NULL;
> > > > -   }
> > > > +   goto free_opt;
> > > > +   }
> > > > +   return rc;
> > >
> > > At this point rc is guaranteed to be 0, so you can just 'return 0' for
> > > clarity. Also, I visually prefer an empty line between a return
> > > statement and a goto label, but I'm not sure what is the
> > > general/maintainer's preference.
> >
> > Am I supposed to revise and send a patch v4 for this, or let the
> > maintainer do this? :-)
> 
> First a few things from my perspective: I don't really care too much
> about the difference between returning "0" and "rc" here, one could
> argue that "0" is cleaner and that "rc" is "safer".  To me it isn't a
> big deal and generally isn't something I would even comment on unless
> there was something else in the patch that needed addressing.  I care
> a more about the style choice of having an empty line between the
> return and the start of the goto targets (vertical whitespace before
> the jump targets is good, please include it), but once again, I'm not
> sure I would comment on that.  The patch subject line is a bit
> confusing in that we already discussed when to use "selinux" and when
> to use "lsm", but I imagine there might be some confusion about using
> both so let me try and clear that up now: don't do it unless you have
> a *really* good reason to do so :)  In this case it is all SELinux
> code so there is no reason why you should be including the "lsm"
> prefix.
Thanks for your comments. I was uncertain of the meaning of "lsm". So I
used"selinux: lsm:". I am aware of that now.

Thanks
Gen
> 
> You've been pretty responsive, so if you don't mind submitting a v4
> with the changes mentioned above, that would be far more preferable to
> me making the changes.  I have some other comments about maintainer
> fixes to patches, but I'll save that for the other thread :)
> 
> -- 
> paul moore
> www.paul-moore.com

Re: [PATCH v3] vt: Fix a missing-check bug in con_init()

[Gen Zhang]

On Sat, Jun 08, 2019 at 08:15:46PM -0400, Nicolas Pitre wrote:
> On Sat, 8 Jun 2019, Greg KH wrote:
> 
> > On Sun, Jun 09, 2019 at 12:01:38AM +0800, Gen Zhang wrote:
> > > On Tue, May 28, 2019 at 08:45:29AM +0800, Gen Zhang wrote:
> > > > In function con_init(), the pointer variable vc_cons[currcons].d, vc and
> > > > vc->vc_screenbuf is allocated by kzalloc(). And they are used in the 
> > > > following codes. However, kzalloc() returns NULL when fails, and null 
> > > > pointer dereference may happen. And it will cause the kernel to crash. 
> > > > Therefore, we should check the return value and handle the error.
> > > > 
> > > > Further, since the allcoation is in a loop, we should free all the 
> > > > allocated memory in a loop.
> > > > 
> > > > Signed-off-by: Gen Zhang 
> > > > Reviewed-by: Nicolas Pitre 
> > > > ---
> > > > diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
> > > > index fdd12f8..d50f68f 100644
> > > > --- a/drivers/tty/vt/vt.c
> > > > +++ b/drivers/tty/vt/vt.c
> > > > @@ -3350,10 +3350,14 @@ static int __init con_init(void)
> > > >  
> > > > for (currcons = 0; currcons < MIN_NR_CONSOLES; currcons++) {
> > > > vc_cons[currcons].d = vc = kzalloc(sizeof(struct 
> > > > vc_data), GFP_NOWAIT);
> > > > +   if (!vc)
> > > > +   goto fail1;
> > > > INIT_WORK(_cons[currcons].SAK_work, vc_SAK);
> > > > tty_port_init(>port);
> > > > visual_init(vc, currcons, 1);
> > > > vc->vc_screenbuf = kzalloc(vc->vc_screenbuf_size, 
> > > > GFP_NOWAIT);
> > > > +   if (!vc->vc_screenbuf)
> > > > +   goto fail2;
> > > > vc_init(vc, vc->vc_rows, vc->vc_cols,
> > > > currcons || !vc->vc_sw->con_save_screen);
> > > > }
> > > > @@ -3375,6 +3379,16 @@ static int __init con_init(void)
> > > > register_console(_console_driver);
> > > >  #endif
> > > > return 0;
> > > > +fail1:
> > > > +   while (currcons > 0) {
> > > > +   currcons--;
> > > > +   kfree(vc_cons[currcons].d->vc_screenbuf);
> > > > +fail2:
> > > > +   kfree(vc_cons[currcons].d);
> > > > +   vc_cons[currcons].d = NULL;
> > > > +   }
> > 
> > Wait, will that even work?  You can jump into the middle of a while
> > loop?
> 
> Absolutely.
> 
> > Ugh, that's beyond ugly.
> 
> That was me who suggested to do it like that. To me, this is nicer than 
> the proposed alternatives. For an error path that is rather unlikely to 
> happen, I think this is a very concise and eleguant way to do it.
> 
> > And please provide "real" names for the
> > labels, "fail1" and "fail2" do not tell anything here.
> 
> That I agree with.
> 
> 
> Nicolas
Thanks for your comments. Then am I supposed to revise the patch and
send a v4 version?

Thanks
Gen

Re: [PATCH v3] vt: Fix a missing-check bug in con_init()

[Gen Zhang]

On Sat, Jun 08, 2019 at 06:21:27PM +0200, Greg KH wrote:
> On Sun, Jun 09, 2019 at 12:01:38AM +0800, Gen Zhang wrote:
> > On Tue, May 28, 2019 at 08:45:29AM +0800, Gen Zhang wrote:
> > > In function con_init(), the pointer variable vc_cons[currcons].d, vc and
> > > vc->vc_screenbuf is allocated by kzalloc(). And they are used in the 
> > > following codes. However, kzalloc() returns NULL when fails, and null 
> > > pointer dereference may happen. And it will cause the kernel to crash. 
> > > Therefore, we should check the return value and handle the error.
> > > 
> > > Further, since the allcoation is in a loop, we should free all the 
> > > allocated memory in a loop.
> > > 
> > > Signed-off-by: Gen Zhang 
> > > Reviewed-by: Nicolas Pitre 
> > > ---
> > > diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
> > > index fdd12f8..d50f68f 100644
> > > --- a/drivers/tty/vt/vt.c
> > > +++ b/drivers/tty/vt/vt.c
> > > @@ -3350,10 +3350,14 @@ static int __init con_init(void)
> > >  
> > >   for (currcons = 0; currcons < MIN_NR_CONSOLES; currcons++) {
> > >   vc_cons[currcons].d = vc = kzalloc(sizeof(struct vc_data), 
> > > GFP_NOWAIT);
> > > + if (!vc)
> > > + goto fail1;
> > >   INIT_WORK(_cons[currcons].SAK_work, vc_SAK);
> > >   tty_port_init(>port);
> > >   visual_init(vc, currcons, 1);
> > >   vc->vc_screenbuf = kzalloc(vc->vc_screenbuf_size, GFP_NOWAIT);
> > > + if (!vc->vc_screenbuf)
> > > + goto fail2;
> > >   vc_init(vc, vc->vc_rows, vc->vc_cols,
> > >   currcons || !vc->vc_sw->con_save_screen);
> > >   }
> > > @@ -3375,6 +3379,16 @@ static int __init con_init(void)
> > >   register_console(_console_driver);
> > >  #endif
> > >   return 0;
> > > +fail1:
> > > + while (currcons > 0) {
> > > + currcons--;
> > > + kfree(vc_cons[currcons].d->vc_screenbuf);
> > > +fail2:
> > > + kfree(vc_cons[currcons].d);
> > > + vc_cons[currcons].d = NULL;
> > > + }
> > > + console_unlock();
> > > + return -ENOMEM;
> > >  }
> > >  console_initcall(con_init);
> > >  
> > > ---
> > Can anyone look into this patch? It's already reviewed by Nicolas Pitre
> > .
> 
> It's in my queue.  But note, given the previous history of your patches,
> it's really low on my piority list at the moment :(
> 
> greg k-h
Anyway, I should be honored to be remembered by Greg K-H. :-)

Thanks
Gen

Re: [PATCH v3] vt: Fix a missing-check bug in con_init()

[Gen Zhang]

On Sat, Jun 08, 2019 at 06:22:19PM +0200, Greg KH wrote:
> On Sun, Jun 09, 2019 at 12:01:38AM +0800, Gen Zhang wrote:
> > On Tue, May 28, 2019 at 08:45:29AM +0800, Gen Zhang wrote:
> > > In function con_init(), the pointer variable vc_cons[currcons].d, vc and
> > > vc->vc_screenbuf is allocated by kzalloc(). And they are used in the 
> > > following codes. However, kzalloc() returns NULL when fails, and null 
> > > pointer dereference may happen. And it will cause the kernel to crash. 
> > > Therefore, we should check the return value and handle the error.
> > > 
> > > Further, since the allcoation is in a loop, we should free all the 
> > > allocated memory in a loop.
> > > 
> > > Signed-off-by: Gen Zhang 
> > > Reviewed-by: Nicolas Pitre 
> > > ---
> > > diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
> > > index fdd12f8..d50f68f 100644
> > > --- a/drivers/tty/vt/vt.c
> > > +++ b/drivers/tty/vt/vt.c
> > > @@ -3350,10 +3350,14 @@ static int __init con_init(void)
> > >  
> > >   for (currcons = 0; currcons < MIN_NR_CONSOLES; currcons++) {
> > >   vc_cons[currcons].d = vc = kzalloc(sizeof(struct vc_data), 
> > > GFP_NOWAIT);
> > > + if (!vc)
> > > + goto fail1;
> > >   INIT_WORK(_cons[currcons].SAK_work, vc_SAK);
> > >   tty_port_init(>port);
> > >   visual_init(vc, currcons, 1);
> > >   vc->vc_screenbuf = kzalloc(vc->vc_screenbuf_size, GFP_NOWAIT);
> > > + if (!vc->vc_screenbuf)
> > > + goto fail2;
> > >   vc_init(vc, vc->vc_rows, vc->vc_cols,
> > >   currcons || !vc->vc_sw->con_save_screen);
> > >   }
> > > @@ -3375,6 +3379,16 @@ static int __init con_init(void)
> > >   register_console(_console_driver);
> > >  #endif
> > >   return 0;
> > > +fail1:
> > > + while (currcons > 0) {
> > > + currcons--;
> > > + kfree(vc_cons[currcons].d->vc_screenbuf);
> > > +fail2:
> > > + kfree(vc_cons[currcons].d);
> > > + vc_cons[currcons].d = NULL;
> > > + }
> 
> Wait, will that even work?  You can jump into the middle of a while
> loop?
I felt like the same when I saw reviewer's advice to write in this way.
But I tested, and it did work.
> 
> Ugh, that's beyond ugly.  And please provide "real" names for the
> labels, "fail1" and "fail2" do not tell anything here.

Sure, I can revise that if needed.

Thanks
Gen
> 
> Also, have you actually be able to trigger this?
> 
> thanks,
> 
> greg k-h

Re: [PATCH v3] vt: Fix a missing-check bug in con_init()

[Gen Zhang]

On Sat, Jun 08, 2019 at 06:21:27PM +0200, Greg KH wrote:
> On Sun, Jun 09, 2019 at 12:01:38AM +0800, Gen Zhang wrote:
> > On Tue, May 28, 2019 at 08:45:29AM +0800, Gen Zhang wrote:
> > > In function con_init(), the pointer variable vc_cons[currcons].d, vc and
> > > vc->vc_screenbuf is allocated by kzalloc(). And they are used in the 
> > > following codes. However, kzalloc() returns NULL when fails, and null 
> > > pointer dereference may happen. And it will cause the kernel to crash. 
> > > Therefore, we should check the return value and handle the error.
> > > 
> > > Further, since the allcoation is in a loop, we should free all the 
> > > allocated memory in a loop.
> > > 
> > > Signed-off-by: Gen Zhang 
> > > Reviewed-by: Nicolas Pitre 
> > > ---
> > > diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
> > > index fdd12f8..d50f68f 100644
> > > --- a/drivers/tty/vt/vt.c
> > > +++ b/drivers/tty/vt/vt.c
> > > @@ -3350,10 +3350,14 @@ static int __init con_init(void)
> > >  
> > >   for (currcons = 0; currcons < MIN_NR_CONSOLES; currcons++) {
> > >   vc_cons[currcons].d = vc = kzalloc(sizeof(struct vc_data), 
> > > GFP_NOWAIT);
> > > + if (!vc)
> > > + goto fail1;
> > >   INIT_WORK(_cons[currcons].SAK_work, vc_SAK);
> > >   tty_port_init(>port);
> > >   visual_init(vc, currcons, 1);
> > >   vc->vc_screenbuf = kzalloc(vc->vc_screenbuf_size, GFP_NOWAIT);
> > > + if (!vc->vc_screenbuf)
> > > + goto fail2;
> > >   vc_init(vc, vc->vc_rows, vc->vc_cols,
> > >   currcons || !vc->vc_sw->con_save_screen);
> > >   }
> > > @@ -3375,6 +3379,16 @@ static int __init con_init(void)
> > >   register_console(_console_driver);
> > >  #endif
> > >   return 0;
> > > +fail1:
> > > + while (currcons > 0) {
> > > + currcons--;
> > > + kfree(vc_cons[currcons].d->vc_screenbuf);
> > > +fail2:
> > > + kfree(vc_cons[currcons].d);
> > > + vc_cons[currcons].d = NULL;
> > > + }
> > > + console_unlock();
> > > + return -ENOMEM;
> > >  }
> > >  console_initcall(con_init);
> > >  
> > > ---
> > Can anyone look into this patch? It's already reviewed by Nicolas Pitre
> > .
> 
> It's in my queue.  But note, given the previous history of your patches,
> it's really low on my piority list at the moment :(
> 
> greg k-h
What? All the patches were revised iteratively according to the 
maintainers' or reviewers' advice. I don't think you should look down
the patches from me. It seems not fair enough. :(

Thanks
Gen

Re: [PATCH] tegra-hsp: fix a missing-check bug in tegra_hsp_doorbell_create()

[Gen Zhang]

On Thu, May 30, 2019 at 09:19:20AM +0800, Gen Zhang wrote:
> In tegra_hsp_doorbell_create(), 'db->name' is allocated by 
> devm_kstrdup_const(). It returns NULL when fails. So 'db->name' should
> be checked.
> 
> Signed-off-by: Gen Zhang 
> Acked-by: Thierry Reding 
> ---
> diff --git a/drivers/mailbox/tegra-hsp.c b/drivers/mailbox/tegra-hsp.c
> index 11fc9fd..b613c46 100644
> --- a/drivers/mailbox/tegra-hsp.c
> +++ b/drivers/mailbox/tegra-hsp.c
> @@ -292,6 +292,8 @@ tegra_hsp_doorbell_create(struct tegra_hsp *hsp, const 
> char *name,
>   db->channel.hsp = hsp;
>  
>   db->name = devm_kstrdup_const(hsp->dev, name, GFP_KERNEL);
> + if (!db->name)
> + return ERR_PTR(-ENOMEM);
>   db->master = master;
>   db->index = index;
>  
> ---
Can anyone look into this patch? It's acked by Thierry Reding 
.

Thanks
Gen

Re: [PATCH v3] vt: Fix a missing-check bug in con_init()

[Gen Zhang]

On Tue, May 28, 2019 at 08:45:29AM +0800, Gen Zhang wrote:
> In function con_init(), the pointer variable vc_cons[currcons].d, vc and
> vc->vc_screenbuf is allocated by kzalloc(). And they are used in the 
> following codes. However, kzalloc() returns NULL when fails, and null 
> pointer dereference may happen. And it will cause the kernel to crash. 
> Therefore, we should check the return value and handle the error.
> 
> Further, since the allcoation is in a loop, we should free all the 
> allocated memory in a loop.
> 
> Signed-off-by: Gen Zhang 
> Reviewed-by: Nicolas Pitre 
> ---
> diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
> index fdd12f8..d50f68f 100644
> --- a/drivers/tty/vt/vt.c
> +++ b/drivers/tty/vt/vt.c
> @@ -3350,10 +3350,14 @@ static int __init con_init(void)
>  
>   for (currcons = 0; currcons < MIN_NR_CONSOLES; currcons++) {
>   vc_cons[currcons].d = vc = kzalloc(sizeof(struct vc_data), 
> GFP_NOWAIT);
> + if (!vc)
> + goto fail1;
>   INIT_WORK(_cons[currcons].SAK_work, vc_SAK);
>   tty_port_init(>port);
>   visual_init(vc, currcons, 1);
>   vc->vc_screenbuf = kzalloc(vc->vc_screenbuf_size, GFP_NOWAIT);
> + if (!vc->vc_screenbuf)
> + goto fail2;
>   vc_init(vc, vc->vc_rows, vc->vc_cols,
>   currcons || !vc->vc_sw->con_save_screen);
>   }
> @@ -3375,6 +3379,16 @@ static int __init con_init(void)
>   register_console(_console_driver);
>  #endif
>   return 0;
> +fail1:
> + while (currcons > 0) {
> + currcons--;
> + kfree(vc_cons[currcons].d->vc_screenbuf);
> +fail2:
> + kfree(vc_cons[currcons].d);
> + vc_cons[currcons].d = NULL;
> + }
> + console_unlock();
> + return -ENOMEM;
>  }
>  console_initcall(con_init);
>  
> ---
Can anyone look into this patch? It's already reviewed by Nicolas Pitre
.

Thanks
Gen

Re: [PATCH v3] selinux: lsm: fix a missing-check bug in selinux_add_mnt_opt( )

[Gen Zhang]

On Fri, Jun 07, 2019 at 10:39:05AM +0200, Ondrej Mosnacek wrote:
> On Thu, Jun 6, 2019 at 11:23 AM Gen Zhang  wrote:
> > In selinux_add_mnt_opt(), 'val' is allocated by kmemdup_nul(). It returns
> > NULL when fails. So 'val' should be checked. And 'mnt_opts' should be
> > freed when error.
> >
> > Signed-off-by: Gen Zhang 
> > Fixes: 757cbe597fe8 ("LSM: new method: ->sb_add_mnt_opt()")
> > ---
> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > index 3ec702c..4e4c1c6 100644
> > --- a/security/selinux/hooks.c
> > +++ b/security/selinux/hooks.c
> > @@ -1052,15 +1052,23 @@ 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) {
> > +   rc = -ENOMEM;
> > +   goto free_opt;
> > +   }
> > +   }
> > rc = selinux_add_opt(token, val, mnt_opts);
> > if (unlikely(rc)) {
> > kfree(val);
> > -   if (*mnt_opts) {
> > -   selinux_free_mnt_opts(*mnt_opts);
> > -   *mnt_opts = NULL;
> > -   }
> > +   goto free_opt;
> > +   }
> > +   return rc;
> 
> At this point rc is guaranteed to be 0, so you can just 'return 0' for
> clarity. Also, I visually prefer an empty line between a return
> statement and a goto label, but I'm not sure what is the
> general/maintainer's preference.
Am I supposed to revise and send a patch v4 for this, or let the 
maintainer do this? :-)

Thanks
Gen
> 
> Also, you should drop the "lsm: " from the subject - it is redundant
> and doesn't follow the SELinux convention. See `git log --oneline --
> security/selinux/` for what the subjects usually look like - mostly we
> just go with "selinux: " (or "LSM: " when
> the changes affect the shared LSM interface).
Thanks for your comments.
> 
> > +free_opt:
> > +   if (*mnt_opts) {
> > +   selinux_free_mnt_opts(*mnt_opts);
> > +   *mnt_opts = NULL;
> > }
> > return rc;
> >  }
> 
> --
> Ondrej Mosnacek 
> Software Engineer, Security Technologies
> Red Hat, Inc.

Re: [PATCH] clk: fix a missing-free bug in clk_cpy_name()

[Gen Zhang]

On Fri, Jun 07, 2019 at 11:10:37AM +0200, Jiri Slaby wrote:
> On 07. 06. 19, 3:52, Gen Zhang wrote:
> >>>>> @@ -3491,6 +3492,8 @@ static int clk_core_populate_parent_map(struct 
> >>>>> clk_core *core)
> >>>>> kfree_const(parents[i].name);
> >>>>> kfree_const(parents[i].fw_name);
> >>>>> } while (--i >= 0);
> >>>>> +   kfree_const(parent->name);
> >>>>> +   kfree_const(parent->fw_name);
> >>>>
> >>>> Both of them were just freed in the loop above, no?
> >>> for (i = 0, parent = parents; i < num_parents; i++, parent++)
> >>> Is 'parent' the same as the one from the loop above?
> >>
> >> Yes. Did it change somehow?
> > parent++?
> 
> parent++ is done after the loop body. Or what do you mean?
> 
> >>> Moreover, should 'parents[i].name' and 'parents[i].fw_name' be freed by
> >>> kfree_const()?
> >>>
> >>
> >> Yes? They're allocated with kstrdup_const() in clk_cpy_name(), or
> >> they're NULL by virtue of the kcalloc and then kfree_const() does
> >> nothing.
> > I re-examined clk_cpy_name(). They are the second parameter of 
> > clk_cpy_name(). The first parameter is allocated, not the second one.
> > So 'parent->name' and 'parent->fw_name' should be freed, not 
> > 'parents[i].name' or 'parents[i].fw_name'. Am I totally misunderstanding
> > this clk_cpy_name()? :-(
> 
> The second parameter (the source) is parent_data[i].*, not parents[i].*
> (the destination). parent->fw_name and parent->name are properly freed
> in the do {} while loop as parents[i].name and parents[i].fw_name, given
> i hasn't changed yet. I am not sure what you mean at all. Are you
> uncertain about the C code flow?
> 
> thanks,
> -- 
> js
> suse labs
Thanks your patient explainaton. I think I need some time to figure out
this part of code.

Thanks
Gen

Re: [PATCH] clk: fix a missing-free bug in clk_cpy_name()

[Gen Zhang]

On Thu, Jun 06, 2019 at 01:16:45PM -0700, Stephen Boyd wrote:
> Quoting Gen Zhang (2019-06-05 09:00:43)
> > On Wed, Jun 05, 2019 at 08:38:00AM +0200, Jiri Slaby wrote:
> > > On 31. 05. 19, 3:14, Gen Zhang wrote:
> > > > In clk_cpy_name(), '*dst_p'('parent->name'and 'parent->fw_name') and 
> > > > 'dst' are allcoted by kstrdup_const(). According to doc: "Strings 
> > > > allocated by kstrdup_const should be freed by kfree_const". So 
> > > > 'parent->name', 'parent->fw_name' and 'dst' should be freed.
> > > > 
> > > > Signed-off-by: Gen Zhang 
> > > > ---
> > > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > > > index aa51756..85c4d3f 100644
> > > > --- a/drivers/clk/clk.c
> > > > +++ b/drivers/clk/clk.c
> > > > @@ -3435,6 +3435,7 @@ static int clk_cpy_name(const char **dst_p, const 
> > > > char *src, bool must_exist)
> > > > if (!dst)
> > > > return -ENOMEM;
> > > >  
> > > > +   kfree_const(dst);
> > > 
> > > So you are now returning a freed pointer in dst_p?
> > Thanks for your reply. I re-examined the code, and this kfree is 
> > incorrect and it should be deleted.
> > > 
> > > > return 0;
> > > >  }
> > > >  
> > > > @@ -3491,6 +3492,8 @@ static int clk_core_populate_parent_map(struct 
> > > > clk_core *core)
> > > > kfree_const(parents[i].name);
> > > > kfree_const(parents[i].fw_name);
> > > > } while (--i >= 0);
> > > > +   kfree_const(parent->name);
> > > > +   kfree_const(parent->fw_name);
> > > 
> > > Both of them were just freed in the loop above, no?
> > for (i = 0, parent = parents; i < num_parents; i++, parent++)
> > Is 'parent' the same as the one from the loop above?
> 
> Yes. Did it change somehow?
parent++?
> 
> > 
> > Moreover, should 'parents[i].name' and 'parents[i].fw_name' be freed by
> > kfree_const()?
> > 
> 
> Yes? They're allocated with kstrdup_const() in clk_cpy_name(), or
> they're NULL by virtue of the kcalloc and then kfree_const() does
> nothing.
I re-examined clk_cpy_name(). They are the second parameter of 
clk_cpy_name(). The first parameter is allocated, not the second one.
So 'parent->name' and 'parent->fw_name' should be freed, not 
'parents[i].name' or 'parents[i].fw_name'. Am I totally misunderstanding
this clk_cpy_name()? :-(

Thanks
Gen
> 
> I'm having a hard time following what this patch is trying to fix. It
> looks unnecessary though so I'm going to drop it from the clk review
> queue.
> 

[PATCH v3] selinux: lsm: fix a missing-check bug in selinux_add_mnt_opt( )

[Gen Zhang]

In selinux_add_mnt_opt(), 'val' is allocated by kmemdup_nul(). It returns
NULL when fails. So 'val' should be checked. And 'mnt_opts' should be 
freed when error.

Signed-off-by: Gen Zhang 
Fixes: 757cbe597fe8 ("LSM: new method: ->sb_add_mnt_opt()")
---
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 3ec702c..4e4c1c6 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -1052,15 +1052,23 @@ 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) {
+   rc = -ENOMEM;
+   goto free_opt;
+   }
+   }
rc = selinux_add_opt(token, val, mnt_opts);
if (unlikely(rc)) {
kfree(val);
-   if (*mnt_opts) {
-   selinux_free_mnt_opts(*mnt_opts);
-   *mnt_opts = NULL;
-   }
+   goto free_opt;
+   }
+   return rc;
+free_opt:
+   if (*mnt_opts) {
+   selinux_free_mnt_opts(*mnt_opts);
+   *mnt_opts = NULL;
}
return rc;
 }

[PATCH v4] selinux: lsm: fix a missing-check bug in selinux_sb_eat_lsm_o pts()

[Gen Zhang]

In selinux_sb_eat_lsm_opts(), 'arg' is allocated by kmemdup_nul(). It
returns NULL when fails. So 'arg' should be checked. And 'mnt_opts' 
should be freed when error.

Signed-off-by: Gen Zhang 
Fixes: 99dbbb593fe6 ("selinux: rewrite selinux_sb_eat_lsm_opts()")
---
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 3ec702c..13479cd 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -2616,10 +2616,11 @@ static int selinux_sb_eat_lsm_opts(char *options, void 
**mnt_opts)
char *from = options;
char *to = options;
bool first = true;
+   int rc;
 
while (1) {
int len = opt_len(from);
-   int token, rc;
+   int token;
char *arg = NULL;
 
token = match_opt_prefix(from, len, );
@@ -2635,15 +2636,15 @@ static int selinux_sb_eat_lsm_opts(char *options, void 
**mnt_opts)
*q++ = c;
}
arg = kmemdup_nul(arg, q - arg, GFP_KERNEL);
+   if (!arg) {
+   rc = -ENOMEM;
+   goto free_opt;
+   }
}
rc = selinux_add_opt(token, arg, mnt_opts);
if (unlikely(rc)) {
kfree(arg);
-   if (*mnt_opts) {
-   selinux_free_mnt_opts(*mnt_opts);
-   *mnt_opts = NULL;
-   }
-   return rc;
+   goto free_opt;
}
} else {
if (!first) {   // copy with preceding comma
@@ -2661,6 +2662,12 @@ static int selinux_sb_eat_lsm_opts(char *options, void 
**mnt_opts)
}
*to = '\0';
return 0;
+free_opt:
+   if (*mnt_opts) {
+   selinux_free_mnt_opts(*mnt_opts);
+   *mnt_opts = NULL;
+   }
+   return rc;
 }
 
 static int selinux_sb_remount(struct super_block *sb, void *mnt_opts)

Re: [PATCH] sg: Fix a double-fetch bug in drivers/scsi/sg.c

[Gen Zhang]

On Wed, Jun 05, 2019 at 01:07:25PM -0400, Douglas Gilbert wrote:
> On 2019-06-05 2:00 a.m., Jiri Slaby wrote:
> >On 23. 05. 19, 4:38, Gen Zhang wrote:
> >>In sg_write(), the opcode of the command is fetched the first time from
> >>the userspace by __get_user(). Then the whole command, the opcode
> >>included, is fetched again from userspace by __copy_from_user().
> >>However, a malicious user can change the opcode between the two fetches.
> >>This can cause inconsistent data and potential errors as cmnd is used in
> >>the following codes.
> >>
> >>Thus we should check opcode between the two fetches to prevent this.
> >>
> >>Signed-off-by: Gen Zhang 
> >>---
> >>diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
> >>index d3f1531..a2971b8 100644
> >>--- a/drivers/scsi/sg.c
> >>+++ b/drivers/scsi/sg.c
> >>@@ -694,6 +694,8 @@ sg_write(struct file *filp, const char __user *buf, 
> >>size_t count, loff_t * ppos)
> >>hp->flags = input_size; /* structure abuse ... */
> >>hp->pack_id = old_hdr.pack_id;
> >>hp->usr_ptr = NULL;
> >>+   if (opcode != cmnd[0])
> >>+   return -EINVAL;
> >
> >Isn't it too early to check cmnd which is copied only here:
> >
> >>if (__copy_from_user(cmnd, buf, cmd_size))
> >>return -EFAULT;
> >>/*
> >>---
> >>
> 
> Hi,
> Yes, it is too early. It needs to be after that __copy_from_user(cmnd,
> buf, cmd_size) call.
Yes, it is.
> 
> To put this in context, this is a very old interface; dating from 1992
> and deprecated for almost 20 years. The fact that the first byte of
> the SCSI cdb needs to be read first to work out that size of the
> following SCSI command and optionally the offset of a data-out
> buffer that may follow the command; is one reason why that interface
> was replaced. Also the implementation did not handle SCSI variable
> length cdb_s.
> 
> Then there is the question of whether this double-fetch is exploitable?
> I cannot think of an example, but there might be (e.g. turning a READ
> command into a WRITE). But the "double-fetch" issue may be more wide
> spread. The replacement interface passes the command and data-in/-out as
> pointers while their corresponding lengths are placed in the newer
> interface structure. This assumes that the cdb and data-out won't
> change in the user space between when the write(2) is called and
> before or while the driver, using those pointers, reads the data.
> All drivers that use pointers to pass data have this "feature".
> 
> Also I'm looking at this particular double-fetch from the point of view
> of the driver rewrite I have done and is currently in the early stages
> of review [linux-scsi list: "[PATCH 00/19] sg: v4 interface, rq sharing
> + multiple rqs"] and this problem is more difficult to fix since the
> full cdb read is delayed to a common point further along the submit
> processing path. To detect a change in cbd[0] my current code would
> need to be altered to carry cdb[0] through to that common point. So
> is it worth it for such an old, deprecated and replaced interface??
> What cdb/user_permissions checking that is done, is done _after_
> the full cdb is read. So trying to get around a user exclusion of
> say WRITE(10) by first using the first byte of READ(10), won't succeed.
> 
> Doug Gilbert
> 
Thanks for your explaination. I guess this patch should be dropped for
the above reasons.

Thanks
Gen

Re: [PATCH] sg: fix a double-fetch bug in sg_write()

[Gen Zhang]

On Thu, Jun 06, 2019 at 07:01:26AM +0200, Jiri Slaby wrote:
> On 05. 06. 19, 17:35, Gen Zhang wrote:
> > On Wed, Jun 05, 2019 at 08:41:11AM +0200, Jiri Slaby wrote:
> >> On 31. 05. 19, 3:27, Gen Zhang wrote:
> >>> In sg_write(), the opcode of the command is fetched the first time from 
> >>> the userspace by __get_user(). Then the whole command, the opcode 
> >>> included, is fetched again from userspace by __copy_from_user(). 
> >>> However, a malicious user can change the opcode between the two fetches.
> >>> This can cause inconsistent data and potential errors as cmnd is used in
> >>> the following codes.
> >>>
> >>> Thus we should check opcode between the two fetches to prevent this.
> >>>
> >>> Signed-off-by: Gen Zhang 
> >>> ---
> >>> diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
> >>> index d3f1531..a2971b8 100644
> >>> --- a/drivers/scsi/sg.c
> >>> +++ b/drivers/scsi/sg.c
> >>> @@ -694,6 +694,8 @@ sg_write(struct file *filp, const char __user *buf, 
> >>> size_t count, loff_t * ppos)
> >>>   hp->flags = input_size; /* structure abuse ... */
> >>>   hp->pack_id = old_hdr.pack_id;
> >>>   hp->usr_ptr = NULL;
> >>> + if (opcode != cmnd[0])
> >>> + return -EINVAL;
> >>>   if (__copy_from_user(cmnd, buf, cmd_size))
> >>>   return -EFAULT;
> >>
> >> You are sending the same patches like a broken machine. Please STOP this
> >> and give people some time to actually review your patches! (Don't expect
> >> replies in days.)
> >>
> > Thanks for your reply. I resubmitted this one after 8-day-no-reply. I 
> > don't judge whether this is a short time period or not. I politely hope
> > that you can reply more kindly.
> 
> There is no reason to be offended. I am just asking you to wait a bit
> more before reposting. 8 days is too few. My personal experience says to
> give patches like these something close to a month, esp. during the
> merge window. The issues are present for a long time, nobody hit them
> during that timeframe, so there is no reason to haste.
Thanks for your reply, and I will keep this in mind.
> 
> > I am just a PhD candidate. All I did is submitting patches, discussing 
> > with maintainers in accordance with linux community rules for academic 
> > papers.
> 
> Yes, despite I have no idea what "linux community rules for academic
> papers" are.

I mean, these patches come from a research project prototype. Submitting
patches, and getting it applied can be demonstrated in the experiment 
part of research paper.

Thanks
Gen
> 
> > I guess that you might be busy person and hope that submitting patches 
> > didn't bother you.
> 
> It does not bother me at all. Patches are welcome, but newcomers tend to
> send new versions of patches (or reposts) too quickly. It then leads to
> wasting time of people where one person comments on one version and the
> others don't see it and reply to some other.
> 
> thanks,
> -- 
> js
> suse labs

Re: [PATCH] Revert "consolemap: Fix a memory leaking bug in drivers/tty/vt/consolemap.c"

[Gen Zhang]

On Tue, Jun 04, 2019 at 09:02:34PM +0200, Greg Kroah-Hartman wrote:
> On Tue, Jun 04, 2019 at 07:00:39PM +0100, Ben Hutchings wrote:
> > This reverts commit 84ecc2f6eb1cb12e6d44818f94fa49b50f06e6ac.
> > 
> > con_insert_unipair() is working with a sparse 3-dimensional array:
> > 
> > - p->uni_pgdir[] is the top layer
> > - p1 points to a middle layer
> > - p2 points to a bottom layer
> > 
> > If it needs to allocate a new middle layer, and then fails to allocate
> > a new bottom layer, it would previously free only p2, and now it frees
> > both p1 and p2.  But since the new middle layer was already registered
> > in the top layer, it was not leaked.
> > 
> > However, if it looks up an *existing* middle layer and then fails to
> > allocate a bottom layer, it now frees both p1 and p2 but does *not*
> > free any other bottom layers under p1.  So it *introduces* a memory
> > leak.
> > 
> > The error path also cleared the wrong index in p->uni_pgdir[],
> > introducing a use-after-free.
> > 
> > Signed-off-by: Ben Hutchings 
> 
> Now applied, thanks.
> 
> Gen, please be careful with these types of "fixes"...
Thanks for your comments. I will for sure. And I am always submutting 
patches and revising it according to the maintainers.

Thanks
Gen
> 
> thanks,
> 
> greg k-h

Re: [PATCH] wimax: debug: fix a missing-check bug in d_parse_params()

[Gen Zhang]

On Wed, Jun 05, 2019 at 08:33:31AM +0200, Jiri Slaby wrote:
> On 30. 05. 19, 11:39, Gen Zhang wrote:
> > In d_parse_params(), 'params_orig' is allocated by kstrdup(). It returns
> > NULL when fails. So 'params_orig' should be checked.
> > 
> > Signed-off-by: Gen Zhang 
> > ---
> > diff --git a/include/linux/wimax/debug.h b/include/linux/wimax/debug.h
> > index aaf24ba..bacd6cb 100644
> > --- a/include/linux/wimax/debug.h
> > +++ b/include/linux/wimax/debug.h
> > @@ -496,6 +496,11 @@ void d_parse_params(struct d_level *d_level, size_t 
> > d_level_size,
> > if (_params == NULL)
> > return;
> > params_orig = kstrdup(_params, GFP_KERNEL);
> > +   if (!params_orig) {
> > +   printk(KERN_ERR "%s: can't duplicate string '%s'\n",
> > +  tag, _params);
> 
> We use pr_err these days.
Thanks for your reply. I used printk() because at least in this file, 
printk() is used.

Should I change printk() to pr_err() and revise this patch, or should 
all printk() in this file be changed? I hope you can give me some 
specific instructions. Actually I don't know what to do when getting 
this reply...

Thanks
Gen
> 
> thanks,
> -- 
> js
> suse labs

Re: [PATCH] clk: fix a missing-free bug in clk_cpy_name()

[Gen Zhang]

On Wed, Jun 05, 2019 at 08:38:00AM +0200, Jiri Slaby wrote:
> On 31. 05. 19, 3:14, Gen Zhang wrote:
> > In clk_cpy_name(), '*dst_p'('parent->name'and 'parent->fw_name') and 
> > 'dst' are allcoted by kstrdup_const(). According to doc: "Strings 
> > allocated by kstrdup_const should be freed by kfree_const". So 
> > 'parent->name', 'parent->fw_name' and 'dst' should be freed.
> > 
> > Signed-off-by: Gen Zhang 
> > ---
> > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > index aa51756..85c4d3f 100644
> > --- a/drivers/clk/clk.c
> > +++ b/drivers/clk/clk.c
> > @@ -3435,6 +3435,7 @@ static int clk_cpy_name(const char **dst_p, const 
> > char *src, bool must_exist)
> > if (!dst)
> > return -ENOMEM;
> >  
> > +   kfree_const(dst);
> 
> So you are now returning a freed pointer in dst_p?
Thanks for your reply. I re-examined the code, and this kfree is 
incorrect and it should be deleted.
> 
> > return 0;
> >  }
> >  
> > @@ -3491,6 +3492,8 @@ static int clk_core_populate_parent_map(struct 
> > clk_core *core)
> > kfree_const(parents[i].name);
> > kfree_const(parents[i].fw_name);
> > } while (--i >= 0);
> > +   kfree_const(parent->name);
> > +   kfree_const(parent->fw_name);
> 
> Both of them were just freed in the loop above, no?
for (i = 0, parent = parents; i < num_parents; i++, parent++)
Is 'parent' the same as the one from the loop above?

Moreover, should 'parents[i].name' and 'parents[i].fw_name' be freed by
kfree_const()?

Thanks
Gen
> 
> thanks,
> -- 
> js
> suse labs

Re: [PATCH] sg: fix a double-fetch bug in sg_write()

[Gen Zhang]

On Wed, Jun 05, 2019 at 08:41:11AM +0200, Jiri Slaby wrote:
> On 31. 05. 19, 3:27, Gen Zhang wrote:
> > In sg_write(), the opcode of the command is fetched the first time from 
> > the userspace by __get_user(). Then the whole command, the opcode 
> > included, is fetched again from userspace by __copy_from_user(). 
> > However, a malicious user can change the opcode between the two fetches.
> > This can cause inconsistent data and potential errors as cmnd is used in
> > the following codes.
> > 
> > Thus we should check opcode between the two fetches to prevent this.
> > 
> > Signed-off-by: Gen Zhang 
> > ---
> > diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
> > index d3f1531..a2971b8 100644
> > --- a/drivers/scsi/sg.c
> > +++ b/drivers/scsi/sg.c
> > @@ -694,6 +694,8 @@ sg_write(struct file *filp, const char __user *buf, 
> > size_t count, loff_t * ppos)
> > hp->flags = input_size; /* structure abuse ... */
> > hp->pack_id = old_hdr.pack_id;
> > hp->usr_ptr = NULL;
> > +   if (opcode != cmnd[0])
> > +   return -EINVAL;
> > if (__copy_from_user(cmnd, buf, cmd_size))
> > return -EFAULT;
> 
> You are sending the same patches like a broken machine. Please STOP this
> and give people some time to actually review your patches! (Don't expect
> replies in days.)
> 
Thanks for your reply. I resubmitted this one after 8-day-no-reply. I 
don't judge whether this is a short time period or not. I politely hope
that you can reply more kindly.

I am just a PhD candidate. All I did is submitting patches, discussing 
with maintainers in accordance with linux community rules for academic papers.

I guess that you might be busy person and hope that submitting patches 
didn't bother you.

Thanks
Gen
> I already commented on this apparently broken one earlier...
> 
> thanks,
> -- 
> js
> suse labs

Re: [PATCH v3] selinux: lsm: fix a missing-check bug in selinux_sb_eat_lsm_opts()

[Gen Zhang]

On Sat, Jun 01, 2019 at 03:34:49AM +0100, Al Viro wrote:
> On Sat, Jun 01, 2019 at 03:25:27AM +0100, Al Viro wrote:
> > On Sat, Jun 01, 2019 at 10:15:26AM +0800, Gen Zhang wrote:
> > > In selinux_sb_eat_lsm_opts(), 'arg' is allocated by kmemdup_nul(). It
> > > returns NULL when fails. So 'arg' should be checked. And 'mnt_opts' 
> > > should be freed when error.
> > 
> > What's the latter one for?  On failure we'll get to put_fs_context()
> > pretty soon, so
> > security_free_mnt_opts(>security);
> > will be called just fine.  Leaving it allocated on failure is fine...
> 
> Actually, right now in mainline it is not (btrfs_mount_root() has
> an odd call of security_sb_eat_lsm_opts()); eventually we will be
> down to just the callers in ->parse_monolithic() instances, at which
> point the above will become correct.  At the moment it is not, so
> consider the objection withdrawn (and I really need to get some sleep,
> seeing how long did it take me to recall the context... ;-/)
Thanks for your comments. And have a good dream.

Thanks
Gen

Re: [PATCH v3] selinux: lsm: fix a missing-check bug in selinux_sb_eat_lsm_opts()

[Gen Zhang]

On Sat, Jun 01, 2019 at 03:25:27AM +0100, Al Viro wrote:
> On Sat, Jun 01, 2019 at 10:15:26AM +0800, Gen Zhang wrote:
> > In selinux_sb_eat_lsm_opts(), 'arg' is allocated by kmemdup_nul(). It
> > returns NULL when fails. So 'arg' should be checked. And 'mnt_opts' 
> > should be freed when error.
> 
> What's the latter one for?  On failure we'll get to put_fs_context()
> pretty soon, so
> security_free_mnt_opts(>security);
> will be called just fine.  Leaving it allocated on failure is fine...
Paul Moore  wrote:
>It seems like we should also check for, and potentially free *mnt_opts
>as the selinux_add_opt() error handling does just below this change,
>yes?  If that is the case we might want to move that error handling
>code to the bottom of the function and jump there on error.
I am not familiar with this part. So could you please show the function
call sequence?

Thanks
Gen

[PATCH v3] selinux: lsm: fix a missing-check bug in selinux_sb_eat_lsm_opts()

[Gen Zhang]

In selinux_sb_eat_lsm_opts(), 'arg' is allocated by kmemdup_nul(). It
returns NULL when fails. So 'arg' should be checked. And 'mnt_opts' 
should be freed when error.

Signed-off-by: Gen Zhang 
Reviewed-by: Ondrej Mosnacek 
Fixes: 99dbbb593fe6 ("selinux: rewrite selinux_sb_eat_lsm_opts()")
---
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 3ec702c..f329fc0 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -2616,6 +2616,7 @@ static int selinux_sb_eat_lsm_opts(char *options, void 
**mnt_opts)
char *from = options;
char *to = options;
bool first = true;
+   int ret;
 
while (1) {
int len = opt_len(from);
@@ -2635,15 +2636,16 @@ static int selinux_sb_eat_lsm_opts(char *options, void 
**mnt_opts)
*q++ = c;
}
arg = kmemdup_nul(arg, q - arg, GFP_KERNEL);
+   if (!arg) {
+   ret = -ENOMEM;
+   goto free_opt;
+   }
}
rc = selinux_add_opt(token, arg, mnt_opts);
if (unlikely(rc)) {
+   ret = rc;
kfree(arg);
-   if (*mnt_opts) {
-   selinux_free_mnt_opts(*mnt_opts);
-   *mnt_opts = NULL;
-   }
-   return rc;
+   goto free_opt;
}
} else {
if (!first) {   // copy with preceding comma
@@ -2661,6 +2663,12 @@ static int selinux_sb_eat_lsm_opts(char *options, void 
**mnt_opts)
}
*to = '\0';
return 0;
+free_opt:
+   if (*mnt_opts) {
+   selinux_free_mnt_opts(*mnt_opts);
+   *mnt_opts = NULL;
+   }
+   return ret;
 }
 
 static int selinux_sb_remount(struct super_block *sb, void *mnt_opts)

Re: [PATCH v2] hooks: fix a missing-check bug in selinux_sb_eat_lsm_opts()

[Gen Zhang]

On Fri, May 31, 2019 at 11:45:28AM -0400, Paul Moore wrote:
> On Thu, May 30, 2019 at 9:34 PM Gen Zhang  wrote:
> >
> > In selinux_sb_eat_lsm_opts(), 'arg' is allocated by kmemdup_nul(). It
> > returns NULL when fails. So 'arg' should be checked.
> >
> > Signed-off-by: Gen Zhang 
> > Reviewed-by: Ondrej Mosnacek 
> > Fixes: 99dbbb593fe6 ("selinux: rewrite selinux_sb_eat_lsm_opts()")
> 
> One quick note about the subject line, instead of using "hooks:" you
> should use "selinux:" since this is specific to SELinux.  If the patch
> did apply to the LSM framework as a whole, I would suggest using
> "lsm:" instead of "hooks:" as "hooks" is too ambiguous of a prefix.
> 
> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > index 3ec702c..5a9e959 100644
> > --- a/security/selinux/hooks.c
> > +++ b/security/selinux/hooks.c
> > @@ -2635,6 +2635,8 @@ static int selinux_sb_eat_lsm_opts(char *options, 
> > void **mnt_opts)
> > *q++ = c;
> > }
> > arg = kmemdup_nul(arg, q - arg, GFP_KERNEL);
> > +   if (!arg)
> > +   return -ENOMEM;
> 
> It seems like we should also check for, and potentially free *mnt_opts
> as the selinux_add_opt() error handling does just below this change,
> yes?  If that is the case we might want to move that error handling
> code to the bottom of the function and jump there on error.
> 
> > }
> > rc = selinux_add_opt(token, arg, mnt_opts);
> > if (unlikely(rc)) {
> 
> -- 
> paul moore
> www.paul-moore.com
Yes, I agree with that. And I will work on this to resubmit.

Thanks
Gen

Re: [PATCH v2] hooks: fix a missing-check bug in selinux_add_mnt_opt()

[Gen Zhang]

On Fri, May 31, 2019 at 11:55:23AM -0400, Paul Moore wrote:
> On Thu, May 30, 2019 at 4:55 AM Gen Zhang  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 
> > 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
> www.paul-moore.com
Thanks
Gen

[PATCH v2] hooks: fix a missing-check bug in selinux_sb_eat_lsm_opts()

[Gen Zhang]

In selinux_sb_eat_lsm_opts(), 'arg' is allocated by kmemdup_nul(). It
returns NULL when fails. So 'arg' should be checked.

Signed-off-by: Gen Zhang 
Reviewed-by: Ondrej Mosnacek 
Fixes: 99dbbb593fe6 ("selinux: rewrite selinux_sb_eat_lsm_opts()")
---
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 3ec702c..5a9e959 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -2635,6 +2635,8 @@ static int selinux_sb_eat_lsm_opts(char *options, void 
**mnt_opts)
*q++ = c;
}
arg = kmemdup_nul(arg, q - arg, GFP_KERNEL);
+   if (!arg)
+   return -ENOMEM;
}
rc = selinux_add_opt(token, arg, mnt_opts);
if (unlikely(rc)) {

[PATCH] sg: fix a double-fetch bug in sg_write()

[Gen Zhang]

In sg_write(), the opcode of the command is fetched the first time from 
the userspace by __get_user(). Then the whole command, the opcode 
included, is fetched again from userspace by __copy_from_user(). 
However, a malicious user can change the opcode between the two fetches.
This can cause inconsistent data and potential errors as cmnd is used in
the following codes.

Thus we should check opcode between the two fetches to prevent this.

Signed-off-by: Gen Zhang 
---
diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index d3f1531..a2971b8 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -694,6 +694,8 @@ sg_write(struct file *filp, const char __user *buf, size_t 
count, loff_t * ppos)
hp->flags = input_size; /* structure abuse ... */
hp->pack_id = old_hdr.pack_id;
hp->usr_ptr = NULL;
+   if (opcode != cmnd[0])
+   return -EINVAL;
if (__copy_from_user(cmnd, buf, cmd_size))
return -EFAULT;
/*
---

[PATCH] mdesc: fix a missing-check bug in get_vdev_port_node_info()

[Gen Zhang]

In get_vdev_port_node_info(), 'node_info->vdev_port.name' is allcoated
by kstrdup_const(), and it returns NULL when fails. So 
'node_info->vdev_port.name' should be checked.

Signed-off-by: Gen Zhang 
---
diff --git a/arch/sparc/kernel/mdesc.c b/arch/sparc/kernel/mdesc.c
index 9a26b44..8e645dd 100644
--- a/arch/sparc/kernel/mdesc.c
+++ b/arch/sparc/kernel/mdesc.c
@@ -356,6 +356,8 @@ static int get_vdev_port_node_info(struct mdesc_handle *md, 
u64 node,
 
node_info->vdev_port.id = *idp;
node_info->vdev_port.name = kstrdup_const(name, GFP_KERNEL);
+   if (!node_info->vdev_port.name)
+   return -1;
node_info->vdev_port.parent_cfg_hdl = *parent_cfg_hdlp;
 
return 0;
---

[PATCH] clk: fix a missing-free bug in clk_cpy_name()

[Gen Zhang]

In clk_cpy_name(), '*dst_p'('parent->name'and 'parent->fw_name') and 
'dst' are allcoted by kstrdup_const(). According to doc: "Strings 
allocated by kstrdup_const should be freed by kfree_const". So 
'parent->name', 'parent->fw_name' and 'dst' should be freed.

Signed-off-by: Gen Zhang 
---
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index aa51756..85c4d3f 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -3435,6 +3435,7 @@ static int clk_cpy_name(const char **dst_p, const char 
*src, bool must_exist)
if (!dst)
return -ENOMEM;
 
+   kfree_const(dst);
return 0;
 }
 
@@ -3491,6 +3492,8 @@ static int clk_core_populate_parent_map(struct clk_core 
*core)
kfree_const(parents[i].name);
kfree_const(parents[i].fw_name);
} while (--i >= 0);
+   kfree_const(parent->name);
+   kfree_const(parent->fw_name);
kfree(parents);
 
return ret;
---

[PATCH] wimax: debug: fix a missing-check bug in d_parse_params()

[Gen Zhang]

In d_parse_params(), 'params_orig' is allocated by kstrdup(). It returns
NULL when fails. So 'params_orig' should be checked.

Signed-off-by: Gen Zhang 
---
diff --git a/include/linux/wimax/debug.h b/include/linux/wimax/debug.h
index aaf24ba..bacd6cb 100644
--- a/include/linux/wimax/debug.h
+++ b/include/linux/wimax/debug.h
@@ -496,6 +496,11 @@ void d_parse_params(struct d_level *d_level, size_t 
d_level_size,
if (_params == NULL)
return;
params_orig = kstrdup(_params, GFP_KERNEL);
+   if (!params_orig) {
+   printk(KERN_ERR "%s: can't duplicate string '%s'\n",
+  tag, _params);
+   return;
+   }
params = params_orig;
while (1) {
token = strsep(, " ");

Re: [PATCH] hooks: fix a missing-check bug in selinux_add_mnt_opt()

[Gen Zhang]

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 
> >>[...]
> 
> MBR, Sergei
Well, my mistake. Thanks for your comments, Sergei!

Thanks
Gen

Re: [PATCH] hooks: fix a missing-check bug in selinux_add_mnt_opt()

[Gen Zhang]

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 
> [...]
> 
> MBR, Sergei

[PATCH] orangefs-debugfs: fix a missing-check bug in debug_string_to_mask()

[Gen Zhang]

In debug_string_to_mask(), 'strsep_fodder' is allocated by kstrdup().
It returns NULL when fails. So 'strsep_fodder' should be checked.

Signed-off-by: Gen Zhang 
---
diff --git a/fs/orangefs/orangefs-debugfs.c b/fs/orangefs/orangefs-debugfs.c
index 87b1a6f..a9a9aac 100644
--- a/fs/orangefs/orangefs-debugfs.c
+++ b/fs/orangefs/orangefs-debugfs.c
@@ -888,6 +888,8 @@ static void debug_string_to_mask(char *debug_string, void 
*mask, int type)
char *unchecked_keyword;
int i;
char *strsep_fodder = kstrdup(debug_string, GFP_KERNEL);
+   if (!strsep_fodder)
+   return;
char *original_pointer;
int element_count = 0;
struct client_debug_mask *c_mask = NULL;

[PATCH v2] hooks: fix a missing-check bug in selinux_add_mnt_opt()

[Gen Zhang]

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 
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);

[PATCH v2] hooks: fix a missing-check bug in selinux_sb_eat_lsm_opts()

[Gen Zhang]

In selinux_sb_eat_lsm_opts(), 'arg' is allocated by kmemdup_nul(). It
returns NULL when fails. So 'arg' should be checked.

Signed-off-by: Gen Zhang 
Fixes: 99dbbb593fe6 ("selinux: rewrite selinux_sb_eat_lsm_opts()")
---
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 3ec702c..5a9e959 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -2635,6 +2635,8 @@ static int selinux_sb_eat_lsm_opts(char *options, void 
**mnt_opts)
*q++ = c;
}
arg = kmemdup_nul(arg, q - arg, GFP_KERNEL);
+   if (!arg)
+   return -ENOMEM;
}
rc = selinux_add_opt(token, arg, mnt_opts);
if (unlikely(rc)) {

[PATCH] hooks: fix a missing-check bug in selinux_add_mnt_opt()

[Gen Zhang]

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 
---
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);

Re: [PATCH] hooks: fix a missing-check bug in selinux_sb_eat_lsm_opts()

[Gen Zhang]

On Wed, May 29, 2019 at 09:39:50PM -0700, William Roberts wrote:
> On Wed, May 29, 2019 at 8:55 PM Gen Zhang  wrote:
> >
> > In selinux_sb_eat_lsm_opts(), 'arg' is allocated by kmemdup_nul(). It
> > returns NULL when fails. So 'arg' should be checked.
> >
> > Signed-off-by: Gen Zhang 
> > ---
> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > index 3ec702c..5a9e959 100644
> > --- a/security/selinux/hooks.c
> > +++ b/security/selinux/hooks.c
> > @@ -2635,6 +2635,8 @@ static int selinux_sb_eat_lsm_opts(char *options, 
> > void **mnt_opts)
> > *q++ = c;
> > }
> > arg = kmemdup_nul(arg, q - arg, GFP_KERNEL);
> > +   if (!arg)
> > +   return 0;
> 
> The routine seems to return 0 on success, why would it return 0 on ENOMEM?
> 
Thanks for your reply, William. I re-examined the source code and didn't
figure out what the return value should be in this situation. Could it 
be a -ENOMEM? Do you have any idea?

Thanks
Gen
> > }
> > rc = selinux_add_opt(token, arg, mnt_opts);
> > if (unlikely(rc)) {

[PATCH] hooks: fix a missing-check bug in selinux_sb_eat_lsm_opts()

[Gen Zhang]

In selinux_sb_eat_lsm_opts(), 'arg' is allocated by kmemdup_nul(). It
returns NULL when fails. So 'arg' should be checked.

Signed-off-by: Gen Zhang 
---
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 3ec702c..5a9e959 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -2635,6 +2635,8 @@ static int selinux_sb_eat_lsm_opts(char *options, void 
**mnt_opts)
*q++ = c;
}
arg = kmemdup_nul(arg, q - arg, GFP_KERNEL);
+   if (!arg)
+   return 0;
}
rc = selinux_add_opt(token, arg, mnt_opts);
if (unlikely(rc)) {

[PATCH] knav_qmss_queue: fix a missing-check bug in knav_pool_create()

[Gen Zhang]

In knav_pool_create(), 'pool->name' is allocated by kstrndup(). It
returns NULL when fails. So 'pool->name' should be checked. And free
'pool' when error.

Signed-off-by: Gen Zhang 
---
diff --git a/drivers/soc/ti/knav_qmss_queue.c b/drivers/soc/ti/knav_qmss_queue.c
index 8b41837..0f8cb28 100644
--- a/drivers/soc/ti/knav_qmss_queue.c
+++ b/drivers/soc/ti/knav_qmss_queue.c
@@ -814,6 +814,12 @@ void *knav_pool_create(const char *name,
}
 
pool->name = kstrndup(name, KNAV_NAME_SIZE - 1, GFP_KERNEL);
+   if (!pool->name) {
+   dev_err(kdev->dev, "failed to duplicate for pool(%s)\n",
+   name);
+   ret = -ENOMEM;
+   goto err_name;
+   }
pool->kdev = kdev;
pool->dev = kdev->dev;
 
@@ -864,6 +870,7 @@ void *knav_pool_create(const char *name,
mutex_unlock(_dev_lock);
 err:
kfree(pool->name);
+err_name:
devm_kfree(kdev->dev, pool);
return ERR_PTR(ret);
 }

[PATCH] tegra-hsp: fix a missing-check bug in tegra_hsp_doorbell_create()

[Gen Zhang]

In tegra_hsp_doorbell_create(), 'db->name' is allocated by 
devm_kstrdup_const(). It returns NULL when fails. So 'db->name' should
be checked.

Signed-off-by: Gen Zhang 
Acked-by: Thierry Reding 
---
diff --git a/drivers/mailbox/tegra-hsp.c b/drivers/mailbox/tegra-hsp.c
index 11fc9fd..b613c46 100644
--- a/drivers/mailbox/tegra-hsp.c
+++ b/drivers/mailbox/tegra-hsp.c
@@ -292,6 +292,8 @@ tegra_hsp_doorbell_create(struct tegra_hsp *hsp, const char 
*name,
db->channel.hsp = hsp;
 
db->name = devm_kstrdup_const(hsp->dev, name, GFP_KERNEL);
+   if (!db->name)
+   return ERR_PTR(-ENOMEM);
db->master = master;
db->index = index;
 
---

[PATCH] mpt3sas_ctl: fix double-fetch bug in _ctl_ioctl_main()

[Gen Zhang]

In _ctl_ioctl_main(), 'ioctl_header' is fetched the first time from 
userspace. 'ioctl_header.ioc_number' is then checked. The legal result 
is saved to 'ioc'. Then, in condition MPT3COMMAND, the whole struct is
fetched again from the userspace. Then _ctl_do_mpt_command() is called,
'ioc' and 'karg' as inputs.

However, a malicious user can change the 'ioc_number' between the two 
fetches, which will cause a potential security issues.  Moreover, a 
malicious user can provide a valid 'ioc_number' to pass the check in 
first fetch, and then modify it in the second fetch.

To fix this, we need to recheck the 'ioc_number' in the second fetch.

Signed-off-by: Gen Zhang 
Ack-by: Suganath Prabu S 
---
diff --git a/drivers/scsi/mpt3sas/mpt3sas_ctl.c 
b/drivers/scsi/mpt3sas/mpt3sas_ctl.c
index b2bb47c..5181c03 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_ctl.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_ctl.c
@@ -2319,6 +2319,10 @@ _ctl_ioctl_main(struct file *file, unsigned int cmd, 
void __user *arg,
break;
}
 
+   if (karg.hdr.ioc_number != ioctl_header.ioc_number) {
+   ret = -EINVAL;
+   break;
+   }
if (_IOC_SIZE(cmd) == sizeof(struct mpt3_ioctl_command)) {
uarg = arg;
ret = _ctl_do_mpt_command(ioc, karg, >mf);

[PATCH] dlpar: Fix a missing-check bug in dlpar_parse_cc_property()

[Gen Zhang]

In dlpar_parse_cc_property(), 'prop->name' is allocated by kstrdup().
kstrdup() may return NULL, so it should be checked and handle error.
And prop should be freed if 'prop->name' is NULL.

Signed-off-by: Gen Zhang 
Acked-by: Nathan Lynch 
---
diff --git a/arch/powerpc/platforms/pseries/dlpar.c 
b/arch/powerpc/platforms/pseries/dlpar.c
index 1795804..c852024 100644
--- a/arch/powerpc/platforms/pseries/dlpar.c
+++ b/arch/powerpc/platforms/pseries/dlpar.c
@@ -61,6 +61,10 @@ static struct property *dlpar_parse_cc_property(struct 
cc_workarea *ccwa)
 
name = (char *)ccwa + be32_to_cpu(ccwa->name_offset);
prop->name = kstrdup(name, GFP_KERNEL);
+   if (!prop->name) {
+   dlpar_free_cc_property(prop);
+   return NULL;
+   }
 
prop->length = be32_to_cpu(ccwa->prop_length);
value = (char *)ccwa + be32_to_cpu(ccwa->prop_offset);
---

[PATCH] wcd9335: fix a incorrect use of kstrndup()

[Gen Zhang]

In wcd9335_codec_enable_dec(), 'widget_name' is allocated by kstrndup().
However, according to doc: "Note: Use kmemdup_nul() instead if the size
is known exactly." So we should use kmemdup_nul() here instead of
kstrndup().

Signed-off-by: Gen Zhang 
---
diff --git a/sound/soc/codecs/wcd9335.c b/sound/soc/codecs/wcd9335.c
index a04a7ce..85737fe 100644
--- a/sound/soc/codecs/wcd9335.c
+++ b/sound/soc/codecs/wcd9335.c
@@ -2734,7 +2734,7 @@ static int wcd9335_codec_enable_dec(struct 
snd_soc_dapm_widget *w,
char *dec;
u8 hpf_coff_freq;
 
-   widget_name = kstrndup(w->name, 15, GFP_KERNEL);
+   widget_name = kmemdup_nul(w->name, 15, GFP_KERNEL);
if (!widget_name)
return -ENOMEM;
 
---

[PATCH] dm-init: fix 2 incorrect use of kstrndup()

[Gen Zhang]

In drivers/md/dm-init.c, kstrndup() is incorrectly used twice.

It should be: char *kstrndup(const char *s, size_t max, gfp_t gfp);

Signed-off-by: Gen Zhang 
---
diff --git a/drivers/md/dm-init.c b/drivers/md/dm-init.c
index 352e803..526e261 100644
--- a/drivers/md/dm-init.c
+++ b/drivers/md/dm-init.c
@@ -140,8 +140,8 @@ static char __init *dm_parse_table_entry(struct dm_device 
*dev, char *str)
return ERR_PTR(-EINVAL);
}
/* target_args */
-   dev->target_args_array[n] = kstrndup(field[3], GFP_KERNEL,
-DM_MAX_STR_SIZE);
+   dev->target_args_array[n] = kstrndup(field[3], DM_MAX_STR_SIZE,
+   GFP_KERNEL);
if (!dev->target_args_array[n])
return ERR_PTR(-ENOMEM);
 
@@ -275,7 +275,7 @@ static int __init dm_init_init(void)
DMERR("Argument is too big. Limit is %d\n", DM_MAX_STR_SIZE);
return -EINVAL;
}
-   str = kstrndup(create, GFP_KERNEL, DM_MAX_STR_SIZE);
+   str = kstrndup(create, DM_MAX_STR_SIZE, GFP_KERNEL);
if (!str)
return -ENOMEM;
 
---

Re: [Question: devm_kfree] When should devm_kfree() be used?

[Gen Zhang]

On Tue, May 28, 2019 at 02:41:38PM +0200, Greg KH wrote:
> No, you are not leaking any memory if you do not call that function.
> Try it and see :)
> 
> The function is there if you just want to "free the memory now!", it's
> not necessary if you return an error as when the device is removed the
> memory will be freed then (or if probe fails, depending on the code
> path.)
> 
> thanks,
> 
> greg k-h
Thanks for your patient replies, Greg. I thinks I am getting to know 
this issue.

Thanks
Gen

Re: [PATCH] wlcore: spi: Fix a memory leaking bug in wl1271_probe()

[Gen Zhang]

On Tue, May 28, 2019 at 03:33:09PM +0300, Kalle Valo wrote:
> Yeah, I don't see how that thread proves that these patches are correct.
> 
Sure, I didn't mean that we came to an agreement that these patches are
correct.
> > Further, I e-mailed Greg K-H about when should we use devm_kmalloc().
> >
> > On Tue, May 28, 2019 at 08:32:57AM +0800, Gen Zhang wrote:
> >> devm_kmalloc() is used to allocate memory for a driver dev. Comments
> >> above the definition and doc 
> >> (https://www.kernel.org/doc/Documentation/driver-model/devres.txt) all
> >> imply that allocated the memory is automatically freed on driver attach,
> >> no matter allocation fail or not. However, I examined the code, and
> >> there are many sites that devm_kfree() is used to free devm_kmalloc().
> >> e.g. hisi_sas_debugfs_init() in drivers/scsi/hisi_sas/hisi_sas_main.c.
> >> So I am totally confused about this issue. Can anybody give me some
> >> guidance? When should we use devm_kfree()?
> > He replied: If you "know" you need to free the memory now, 
> > call devm_kfree(). If you want to wait for it to be cleaned up latter, 
> > like normal, then do not call it.
> >
> > So could please look in to this issue?
> 
> Sorry, no time to investigate this in detail. If you think the patches
> are correct you can resend them and get someone familiar with the driver
> to provide Reviewed-by, then I will apply them.
> 
> -- 
> Kalle Valo
Ok, thanks for your time. I will follow your suggestions.

Thanks
Gen

Re: [PATCH] wlcore: spi: Fix a memory leaking bug in wl1271_probe()

[Gen Zhang]

On Tue, May 28, 2019 at 11:39:22AM +, Kalle Valo wrote:
> Gen Zhang  wrote:
> 
> > In wl1271_probe(), 'glue->core' is allocated by platform_device_alloc(),
> > when this allocation fails, ENOMEM is returned. However, 'pdev_data'
> > and 'glue' are allocated by devm_kzalloc() before 'glue->core'. When
> > platform_device_alloc() returns NULL, we should also free 'pdev_data'
> > and 'glue' before wl1271_probe() ends to prevent leaking memory.
> > 
> > Similarly, we shoulf free 'pdev_data' when 'glue' is NULL. And we should
> > free 'pdev_data' and 'glue' when 'glue->reg' is error and when 'ret' is
> > error.
> > 
> > Further, we should free 'glue->core', 'pdev_data' and 'glue' when this 
> > function normally ends to prevent leaking memory.
> > 
> > Signed-off-by: Gen Zhang 
> 
> Same questions as with similar SDIO patch:
> 
> https://patchwork.kernel.org/patch/10959049/
> 
> Patch set to Changes Requested.
> 
> -- 
> https://patchwork.kernel.org/patch/10959053/
> 
> https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
> 
Thanks for your reply, Kalle. I had debate with Jon about this patch. 
You could kindly refer to lkml: https://lkml.org/lkml/2019/5/23/1547. 
And I don't think a practical conclusion is made there.

Further, I e-mailed Greg K-H about when should we use devm_kmalloc().

On Tue, May 28, 2019 at 08:32:57AM +0800, Gen Zhang wrote:
> devm_kmalloc() is used to allocate memory for a driver dev. Comments
> above the definition and doc 
> (https://www.kernel.org/doc/Documentation/driver-model/devres.txt) all
> imply that allocated the memory is automatically freed on driver attach,
> no matter allocation fail or not. However, I examined the code, and
> there are many sites that devm_kfree() is used to free devm_kmalloc().
> e.g. hisi_sas_debugfs_init() in drivers/scsi/hisi_sas/hisi_sas_main.c.
> So I am totally confused about this issue. Can anybody give me some
> guidance? When should we use devm_kfree()?
He replied: If you "know" you need to free the memory now, 
call devm_kfree(). If you want to wait for it to be cleaned up latter, 
like normal, then do not call it.

So could please look in to this issue?

Thanks
Gen

Re: [Question: devm_kfree] When should devm_kfree() be used?

[Gen Zhang]

On Tue, May 28, 2019 at 08:49:49AM +0200, Greg KH wrote:
> On Tue, May 28, 2019 at 08:32:57AM +0800, Gen Zhang wrote:
> > devm_kmalloc() is used to allocate memory for a driver dev. Comments
> > above the definition and doc 
> > (https://www.kernel.org/doc/Documentation/driver-model/devres.txt) all
> > imply that allocated the memory is automatically freed on driver attach,
> > no matter allocation fail or not. However, I examined the code, and
> > there are many sites that devm_kfree() is used to free devm_kmalloc().
> > e.g. hisi_sas_debugfs_init() in drivers/scsi/hisi_sas/hisi_sas_main.c.
> > So I am totally confused about this issue. Can anybody give me some
> > guidance? When should we use devm_kfree()?
> 
> If you "know" you need to free the memory now, call devm_kfree().  If
> you want to wait for it to be cleaned up latter, like normal, then do
> not call it.
> 
> Do you have a driver that you think uses it incorrectly?
> 
> thanks,
> 
> greg k-h
Yes, that is my question! I do have several patches about devm_kfree() 
and debate with maintainers. e.g. you can kindly refer to this lkml:
https://lkml.org/lkml/2019/5/23/1547. 

In tegra_wm9712_driver_probe(), 'machine->codec' is allocated by
platform_device_alloc(). When it is NULL, function returns ENOMEM.
However, 'machine' is allocated by devm_kzalloc() before this site.
Thus we should free 'machine' before function ends to prevent memory
leaking.

And as mentioned above, e.g. hisi_sas_debugfs_init() in 
drivers/scsi/hisi_sas/hisi_sas_main.c, devm_kfree() is used to free 
devm_kmalloc().

So, thanks for looking into this, Greg.

Thanks
Gen

Re: [PATCH] mpt3sas_ctl: fix double-fetch bug in _ctl_ioctl_main()

[Gen Zhang]

On Tue, May 28, 2019 at 11:44:35AM +0530, Suganath Prabu Subramani wrote:
> Please consider this patch as Ack-by: Suganath Prabu S
> 
> 
> Thanks,
> Suganath.
> 
> 
> On Mon, May 27, 2019 at 6:27 AM Gen Zhang  wrote:
> >
> > In _ctl_ioctl_main(), 'ioctl_header' is fetched the first time from
> > userspace. 'ioctl_header.ioc_number' is then checked. The legal result
> > is saved to 'ioc'. Then, in condition MPT3COMMAND, the whole struct is
> > fetched again from the userspace. Then _ctl_do_mpt_command() is called,
> > 'ioc' and 'karg' as inputs.
> >
> > However, a malicious user can change the 'ioc_number' between the two
> > fetches, which will cause a potential security issues.  Moreover, a
> > malicious user can provide a valid 'ioc_number' to pass the check in
> > first fetch, and then modify it in the second fetch.
> >
> > To fix this, we need to recheck the 'ioc_number' in the second fetch.
> >
> > Signed-off-by: Gen Zhang 
> > ---
> > diff --git a/drivers/scsi/mpt3sas/mpt3sas_ctl.c 
> > b/drivers/scsi/mpt3sas/mpt3sas_ctl.c
> > index b2bb47c..5181c03 100644
> > --- a/drivers/scsi/mpt3sas/mpt3sas_ctl.c
> > +++ b/drivers/scsi/mpt3sas/mpt3sas_ctl.c
> > @@ -2319,6 +2319,10 @@ _ctl_ioctl_main(struct file *file, unsigned int cmd, 
> > void __user *arg,
> > break;
> > }
> >
> > +   if (karg.hdr.ioc_number != ioctl_header.ioc_number) {
> > +   ret = -EINVAL;
> > +   break;
> > +   }
> > if (_IOC_SIZE(cmd) == sizeof(struct mpt3_ioctl_command)) {
> > uarg = arg;
> > ret = _ctl_do_mpt_command(ioc, karg, >mf);
Thanks for your reply, Suganath.

Thanks
Gen

[PATCH] clk-sunxi: fix a missing-check bug in sunxi_divs_clk_setup()

[Gen Zhang]

In sunxi_divs_clk_setup(), 'derived_name' is allocated by kstrndup().
It returns NULL when fails. 'derived_name' should be checked.

Signed-off-by: Gen Zhang 
---
diff --git a/drivers/clk/sunxi/clk-sunxi.c b/drivers/clk/sunxi/clk-sunxi.c
index f5b1c00..830bfb7 100644
--- a/drivers/clk/sunxi/clk-sunxi.c
+++ b/drivers/clk/sunxi/clk-sunxi.c
@@ -989,6 +989,8 @@ static struct clk ** __init sunxi_divs_clk_setup(struct 
device_node *node,
if (endp) {
derived_name = kstrndup(clk_name, endp - clk_name,
GFP_KERNEL);
+   if (!derived_name)
+   return NULL;
factors.name = derived_name;
} else {
factors.name = clk_name;
---

[PATCH] dfs_cache: fix a wrong use of kfree in flush_cache_ent()

[Gen Zhang]

In flush_cache_ent(), 'ce->ce_path' is allocated by kstrdup_const().
It should be freed by kfree_const(), rather than kfree().

Signed-off-by: Gen Zhang 
---
diff --git a/fs/cifs/dfs_cache.c b/fs/cifs/dfs_cache.c
index 85dc89d..e3e1c13 100644
--- a/fs/cifs/dfs_cache.c
+++ b/fs/cifs/dfs_cache.c
@@ -132,7 +132,7 @@ static inline void flush_cache_ent(struct dfs_cache_entry 
*ce)
return;
 
hlist_del_init_rcu(>ce_hlist);
-   kfree(ce->ce_path);
+   kfree_const(ce->ce_path);
free_tgts(ce);
dfs_cache_count--;
call_rcu(>ce_rcu, free_cache_entry);
@@ -422,7 +422,7 @@ alloc_cache_entry(const char *path, const struct 
dfs_info3_param *refs,
 
rc = copy_ref_data(refs, numrefs, ce, NULL);
if (rc) {
-   kfree(ce->ce_path);
+   kfree_const(ce->ce_path);
kmem_cache_free(dfs_cache_slab, ce);
ce = ERR_PTR(rc);
}
---

[PATCH v3] vt: Fix a missing-check bug in con_init()

[Gen Zhang]

In function con_init(), the pointer variable vc_cons[currcons].d, vc and
vc->vc_screenbuf is allocated by kzalloc(). And they are used in the 
following codes. However, kzalloc() returns NULL when fails, and null 
pointer dereference may happen. And it will cause the kernel to crash. 
Therefore, we should check the return value and handle the error.

Further, since the allcoation is in a loop, we should free all the 
allocated memory in a loop.

Signed-off-by: Gen Zhang 
Reviewed-by: Nicolas Pitre 
---
diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
index fdd12f8..d50f68f 100644
--- a/drivers/tty/vt/vt.c
+++ b/drivers/tty/vt/vt.c
@@ -3350,10 +3350,14 @@ static int __init con_init(void)
 
for (currcons = 0; currcons < MIN_NR_CONSOLES; currcons++) {
vc_cons[currcons].d = vc = kzalloc(sizeof(struct vc_data), 
GFP_NOWAIT);
+   if (!vc)
+   goto fail1;
INIT_WORK(_cons[currcons].SAK_work, vc_SAK);
tty_port_init(>port);
visual_init(vc, currcons, 1);
vc->vc_screenbuf = kzalloc(vc->vc_screenbuf_size, GFP_NOWAIT);
+   if (!vc->vc_screenbuf)
+   goto fail2;
vc_init(vc, vc->vc_rows, vc->vc_cols,
currcons || !vc->vc_sw->con_save_screen);
}
@@ -3375,6 +3379,16 @@ static int __init con_init(void)
register_console(_console_driver);
 #endif
return 0;
+fail1:
+   while (currcons > 0) {
+   currcons--;
+   kfree(vc_cons[currcons].d->vc_screenbuf);
+fail2:
+   kfree(vc_cons[currcons].d);
+   vc_cons[currcons].d = NULL;
+   }
+   console_unlock();
+   return -ENOMEM;
 }
 console_initcall(con_init);
 
---

[Question: devm_kfree] When should devm_kfree() be used?

[Gen Zhang]

devm_kmalloc() is used to allocate memory for a driver dev. Comments
above the definition and doc 
(https://www.kernel.org/doc/Documentation/driver-model/devres.txt) all
imply that allocated the memory is automatically freed on driver attach,
no matter allocation fail or not. However, I examined the code, and
there are many sites that devm_kfree() is used to free devm_kmalloc().
e.g. hisi_sas_debugfs_init() in drivers/scsi/hisi_sas/hisi_sas_main.c.
So I am totally confused about this issue. Can anybody give me some
guidance? When should we use devm_kfree()?

Thanks

Re: [PATCH] tegra-hsp: fix a missing-check bug in tegra_hsp_doorbell_create()

[Gen Zhang]

On Mon, May 27, 2019 at 04:05:49PM +0200, Thierry Reding wrote:
> On Mon, May 27, 2019 at 09:04:30PM +0800, Gen Zhang wrote:
> > In tegra_hsp_doorbell_create(), 'db->name' is allocated by 
> > devm_kstrdup_const(). It returns NULL when fails. So 'db->name' should
> > be checked.
> > 
> > Signed-off-by: Gen Zhang 
> > ---
> > diff --git a/drivers/mailbox/tegra-hsp.c b/drivers/mailbox/tegra-hsp.c
> > index 11fc9fd..b613c46 100644
> > --- a/drivers/mailbox/tegra-hsp.c
> > +++ b/drivers/mailbox/tegra-hsp.c
> > @@ -292,6 +292,8 @@ tegra_hsp_doorbell_create(struct tegra_hsp *hsp, const 
> > char *name,
> > db->channel.hsp = hsp;
> >  
> > db->name = devm_kstrdup_const(hsp->dev, name, GFP_KERNEL);
> > +   if (!db->name)
> > +   return ERR_PTR(-ENOMEM);
> 
> I don't think this could ever happen, since name is always from .rodata
> and hence devm_kstrdup_const() never returns NULL. But formally this is
> correct, so:
> 
> Acked-by: Thierry Reding 
Thanks for your reply, Thierry.

Thanks
Gen

[PATCH] clk: fix a missing-free bug in clk_cpy_name()

[Gen Zhang]

In clk_cpy_name(), '*dst_p'('parent->name'and 'parent->fw_name') and 
'dst' are allcoted by kstrdup_const(). According to doc: "Strings 
allocated by kstrdup_const should be freed by kfree_const". So 
'parent->name', 'parent->fw_name' and 'dst' should be freed.

Signed-off-by: Gen Zhang 
---
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index aa51756..85c4d3f 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -3435,6 +3435,7 @@ static int clk_cpy_name(const char **dst_p, const char 
*src, bool must_exist)
if (!dst)
return -ENOMEM;
 
+   kfree_const(dst);
return 0;
 }
 
@@ -3491,6 +3492,8 @@ static int clk_core_populate_parent_map(struct clk_core 
*core)
kfree_const(parents[i].name);
kfree_const(parents[i].fw_name);
} while (--i >= 0);
+   kfree_const(parent->name);
+   kfree_const(parent->fw_name);
kfree(parents);
 
return ret;
---

[PATCH] tegra-hsp: fix a missing-check bug in tegra_hsp_doorbell_create()

[Gen Zhang]

In tegra_hsp_doorbell_create(), 'db->name' is allocated by 
devm_kstrdup_const(). It returns NULL when fails. So 'db->name' should
be checked.

Signed-off-by: Gen Zhang 
---
diff --git a/drivers/mailbox/tegra-hsp.c b/drivers/mailbox/tegra-hsp.c
index 11fc9fd..b613c46 100644
--- a/drivers/mailbox/tegra-hsp.c
+++ b/drivers/mailbox/tegra-hsp.c
@@ -292,6 +292,8 @@ tegra_hsp_doorbell_create(struct tegra_hsp *hsp, const char 
*name,
db->channel.hsp = hsp;
 
db->name = devm_kstrdup_const(hsp->dev, name, GFP_KERNEL);
+   if (!db->name)
+   return ERR_PTR(-ENOMEM);
db->master = master;
db->index = index;
 
---

[PATCH] mdesc: fix a missing-check bug in get_vdev_port_node_info()

[Gen Zhang]

In get_vdev_port_node_info(), 'node_info->vdev_port.name' is allcoated
by kstrdup_const(), and it returns NULL when fails. So 
'node_info->vdev_port.name' should be checked.

Signed-off-by: Gen Zhang 
---
diff --git a/arch/sparc/kernel/mdesc.c b/arch/sparc/kernel/mdesc.c
index 9a26b44..8e645dd 100644
--- a/arch/sparc/kernel/mdesc.c
+++ b/arch/sparc/kernel/mdesc.c
@@ -356,6 +356,8 @@ static int get_vdev_port_node_info(struct mdesc_handle *md, 
u64 node,
 
node_info->vdev_port.id = *idp;
node_info->vdev_port.name = kstrdup_const(name, GFP_KERNEL);
+   if (!node_info->vdev_port.name)
+   return -1;
node_info->vdev_port.parent_cfg_hdl = *parent_cfg_hdlp;
 
return 0;
---

[PATCH] sg: fix a double-fetch bug in sg_write()

[Gen Zhang]

In sg_write(), the opcode of the command is fetched the first time from 
the userspace by __get_user(). Then the whole command, the opcode 
included, is fetched again from userspace by __copy_from_user(). 
However, a malicious user can change the opcode between the two fetches.
This can cause inconsistent data and potential errors as cmnd is used in
the following codes.

Thus we should check opcode between the two fetches to prevent this.

Signed-off-by: Gen Zhang 
---
diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index d3f1531..a2971b8 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -694,6 +694,8 @@ sg_write(struct file *filp, const char __user *buf, size_t 
count, loff_t * ppos)
hp->flags = input_size; /* structure abuse ... */
hp->pack_id = old_hdr.pack_id;
hp->usr_ptr = NULL;
+   if (opcode != cmnd[0])
+   return -EINVAL;
if (__copy_from_user(cmnd, buf, cmd_size))
return -EFAULT;
/*
---

[PATCH] mpt3sas_ctl: fix double-fetch bug in _ctl_ioctl_main()

[Gen Zhang]

In _ctl_ioctl_main(), 'ioctl_header' is fetched the first time from 
userspace. 'ioctl_header.ioc_number' is then checked. The legal result 
is saved to 'ioc'. Then, in condition MPT3COMMAND, the whole struct is
fetched again from the userspace. Then _ctl_do_mpt_command() is called,
'ioc' and 'karg' as inputs.

However, a malicious user can change the 'ioc_number' between the two 
fetches, which will cause a potential security issues.  Moreover, a 
malicious user can provide a valid 'ioc_number' to pass the check in 
first fetch, and then modify it in the second fetch.

To fix this, we need to recheck the 'ioc_number' in the second fetch.

Signed-off-by: Gen Zhang 
---
diff --git a/drivers/scsi/mpt3sas/mpt3sas_ctl.c 
b/drivers/scsi/mpt3sas/mpt3sas_ctl.c
index b2bb47c..5181c03 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_ctl.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_ctl.c
@@ -2319,6 +2319,10 @@ _ctl_ioctl_main(struct file *file, unsigned int cmd, 
void __user *arg,
break;
}
 
+   if (karg.hdr.ioc_number != ioctl_header.ioc_number) {
+   ret = -EINVAL;
+   break;
+   }
if (_IOC_SIZE(cmd) == sizeof(struct mpt3_ioctl_command)) {
uarg = arg;
ret = _ctl_do_mpt_command(ioc, karg, >mf);

[PATCH] dlpar: Fix a missing-check bug in dlpar_parse_cc_property()

[Gen Zhang]

In dlpar_parse_cc_property(), 'prop->name' is allocated by kstrdup().
kstrdup() may return NULL, so it should be checked and handle error.
And prop should be freed if 'prop->name' is NULL.

Signed-off-by: Gen Zhang 
---
diff --git a/arch/powerpc/platforms/pseries/dlpar.c 
b/arch/powerpc/platforms/pseries/dlpar.c
index 1795804..c852024 100644
--- a/arch/powerpc/platforms/pseries/dlpar.c
+++ b/arch/powerpc/platforms/pseries/dlpar.c
@@ -61,6 +61,10 @@ static struct property *dlpar_parse_cc_property(struct 
cc_workarea *ccwa)
 
name = (char *)ccwa + be32_to_cpu(ccwa->name_offset);
prop->name = kstrdup(name, GFP_KERNEL);
+   if (!prop->name) {
+   dlpar_free_cc_property(prop);
+   return NULL;
+   }
 
prop->length = be32_to_cpu(ccwa->prop_length);
value = (char *)ccwa + be32_to_cpu(ccwa->prop_offset);
---

Re: [A General Question] What should I do after getting Reviewed-by from a maintainer?

[Gen Zhang]

On Sat, May 25, 2019 at 07:06:48AM +0200, Willy Tarreau wrote:
> On Sat, May 25, 2019 at 10:12:41AM +0800, Gen Zhang wrote:
> > On Fri, May 24, 2019 at 04:21:36PM -0700, Randy Dunlap wrote:
> > > On 5/22/19 6:17 PM, Gen Zhang wrote:
> > > > Hi Andrew,
> > > > I am starting submitting patches these days and got some patches 
> > > > "Reviewed-by" from maintainers. After checking the 
> > > > submitting-patches.html, I figured out what "Reviewed-by" means. But I
> > > > didn't get the guidance on what to do after getting "Reviewed-by".
> > > > Am I supposed to send this patch to more maintainers? Or something else?
> > > > Thanks
> > > > Gen
> > > > 
> > > 
> > > [Yes, I am not Andrew. ;]
> > > 
> > > Patches should be sent to a maintainer who is responsible for merging
> > > changes for the driver or $arch or subsystem.
> > > And they should also be Cc-ed to the appropriate mailing list(s) and
> > > source code author(s), usually [unless they are no longer active].
> > > 
> > > Some source files have author email addresses in them.
> > > Or in a kernel git tree, you can use "git log path/to/source/file.c" to 
> > > see
> > > who has been making & merging patches to that file.c.
> > > Probably the easiest thing to do is run ./scripts/get_maintainer.pl and
> > > it will try to tell you who to send the patch to.
> > > 
> > > HTH.
> > > -- 
> > > ~Randy
> > Thanks for your patient instructions, Randy! I alrady figured it out.
> 
> Then if your question is what to do with these "Reviewed-by", you should
> edit your patches and place these fields next to your Signed-off-by line
> to indicate that these persons have reviewed this code (and didn't have
> anything particular to say about it). From this point you should not
> modify the patches with this tag.
> 
> When you'll resend your final series to the maintainer, it will include
> all these reviewed-by tags and will generally save the maintainer some
> review time by skipping some of them.
> 
> Willy
Thanks for your instructions, Willy! I already figured out what to do
now.

Thanks
Gen

[tip:efi/urgent] efi/x86/Add missing error handling to old_memmap 1:1 mapping code

[tip-bot for Gen Zhang]

Commit-ID:  4e78921ba4dd0aca1cc89168f45039add4183f8e
Gitweb: https://git.kernel.org/tip/4e78921ba4dd0aca1cc89168f45039add4183f8e
Author: Gen Zhang 
AuthorDate: Sat, 25 May 2019 13:25:58 +0200
Committer:  Ingo Molnar 
CommitDate: Sat, 25 May 2019 13:48:17 +0200

efi/x86/Add missing error handling to old_memmap 1:1 mapping code

The old_memmap flow in efi_call_phys_prolog() performs numerous memory
allocations, and either does not check for failure at all, or it does
but fails to propagate it back to the caller, which may end up calling
into the firmware with an incomplete 1:1 mapping.

So let's fix this by returning NULL from efi_call_phys_prolog() on
memory allocation failures only, and by handling this condition in the
caller. Also, clean up any half baked sets of page tables that we may
have created before returning with a NULL return value.

Note that any failure at this level will trigger a panic() two levels
up, so none of this makes a huge difference, but it is a nice cleanup
nonetheless.

[ardb: update commit log, add efi_call_phys_epilog() call on error path]

Signed-off-by: Gen Zhang 
Signed-off-by: Ard Biesheuvel 
Cc: Linus Torvalds 
Cc: Peter Zijlstra 
Cc: Rob Bradford 
Cc: Thomas Gleixner 
Cc: linux-...@vger.kernel.org
Link: http://lkml.kernel.org/r/20190525112559.7917-2-ard.biesheu...@linaro.org
Signed-off-by: Ingo Molnar 
---
 arch/x86/platform/efi/efi.c| 2 ++
 arch/x86/platform/efi/efi_64.c | 9 ++---
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index e1cb01a22fa8..a7189a3b4d70 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -85,6 +85,8 @@ static efi_status_t __init phys_efi_set_virtual_address_map(
pgd_t *save_pgd;
 
save_pgd = efi_call_phys_prolog();
+   if (!save_pgd)
+   return EFI_ABORTED;
 
/* Disable interrupts around EFI calls: */
local_irq_save(flags);
diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
index cf0347f61b21..08ce8177c3af 100644
--- a/arch/x86/platform/efi/efi_64.c
+++ b/arch/x86/platform/efi/efi_64.c
@@ -84,13 +84,15 @@ pgd_t * __init efi_call_phys_prolog(void)
 
if (!efi_enabled(EFI_OLD_MEMMAP)) {
efi_switch_mm(_mm);
-   return NULL;
+   return efi_mm.pgd;
}
 
early_code_mapping_set_exec(1);
 
n_pgds = DIV_ROUND_UP((max_pfn << PAGE_SHIFT), PGDIR_SIZE);
save_pgd = kmalloc_array(n_pgds, sizeof(*save_pgd), GFP_KERNEL);
+   if (!save_pgd)
+   return NULL;
 
/*
 * Build 1:1 identity mapping for efi=old_map usage. Note that
@@ -138,10 +140,11 @@ pgd_t * __init efi_call_phys_prolog(void)
pgd_offset_k(pgd * PGDIR_SIZE)->pgd &= ~_PAGE_NX;
}
 
-out:
__flush_tlb_all();
-
return save_pgd;
+out:
+   efi_call_phys_epilog(save_pgd);
+   return NULL;
 }
 
 void __init efi_call_phys_epilog(pgd_t *save_pgd)

Re: [A General Question] What should I do after getting Reviewed-by from a maintainer?

[Gen Zhang]

On Fri, May 24, 2019 at 04:21:36PM -0700, Randy Dunlap wrote:
> On 5/22/19 6:17 PM, Gen Zhang wrote:
> > Hi Andrew,
> > I am starting submitting patches these days and got some patches 
> > "Reviewed-by" from maintainers. After checking the 
> > submitting-patches.html, I figured out what "Reviewed-by" means. But I
> > didn't get the guidance on what to do after getting "Reviewed-by".
> > Am I supposed to send this patch to more maintainers? Or something else?
> > Thanks
> > Gen
> > 
> 
> [Yes, I am not Andrew. ;]
> 
> Patches should be sent to a maintainer who is responsible for merging
> changes for the driver or $arch or subsystem.
> And they should also be Cc-ed to the appropriate mailing list(s) and
> source code author(s), usually [unless they are no longer active].
> 
> Some source files have author email addresses in them.
> Or in a kernel git tree, you can use "git log path/to/source/file.c" to see
> who has been making & merging patches to that file.c.
> Probably the easiest thing to do is run ./scripts/get_maintainer.pl and
> it will try to tell you who to send the patch to.
> 
> HTH.
> -- 
> ~Randy
Thanks for your patient instructions, Randy! I alrady figured it out.

Thanks
Gen

[devm_kfree() usage] When should devm_kfree() be used?

[Gen Zhang]

devm_kmalloc() is used to allocate memory for a driver dev. Comments
above the definition and doc 
(https://www.kernel.org/doc/Documentation/driver-model/devres.txt) all
imply that allocated the memory is automatically freed on driver attach,
no matter allocation fail or not. However, I examined the code, and
there are many sites that devm_kfree() is used to free devm_kmalloc().
e.g. hisi_sas_debugfs_init() in drivers/scsi/hisi_sas/hisi_sas_main.c.
So I am totally confused about this issue. Can anybody give me some
guidance? When should we use devm_kfree()?

Thanks
Gen

Re: [PATCH] tegra_wm9712: Fix a memory leaking bug in tegra_wm9712_driver_probe()

[Gen Zhang]

On Fri, May 24, 2019 at 04:36:54PM +0100, Jon Hunter wrote:
> There could well be cases where you need to explicitly call
> devm_kfree(), but having a quick glance at the example above, I don't
> see why you would call devm_kfree() here and yes looks like that code
> could be simplified significantly. Notice that hisi_sas_debugfs_exit()
> does not free any memory as it is not necessary to explicitly do so.
> 
> Cheers
> Jon
> 
> -- 
> nvpublic
Thanks for your suggestions, Jon! I think I need to e-mail to those
maintainers about this issue.

Thanks
Gen

Re: [PATCH] tegra_wm9712: Fix a memory leaking bug in tegra_wm9712_driver_probe()

[Gen Zhang]

On Fri, May 24, 2019 at 03:47:34PM +0100, Jon Hunter wrote:
> 
> On 24/05/2019 15:33, Gen Zhang wrote:
> > On Fri, May 24, 2019 at 09:33:13AM +0100, Jon Hunter wrote:
> >>
> >> On 24/05/2019 01:50, Gen Zhang wrote:
> >>> In tegra_wm9712_driver_probe(), 'machine->codec' is allocated by
> >>> platform_device_alloc(). When it is NULL, function returns ENOMEM.
> >>> However, 'machine' is allocated by devm_kzalloc() before this site.
> >>> Thus we should free 'machine' before function ends to prevent memory
> >>> leaking.
> >>
> >> Memory allocated by devm_xxx() is automatically freed on failure so this
> >> is not correct.
> > Thanks for your comments, Jon. But after I examined the code, I am still
> > confused about the usage of devm_kmalloc(). You can kindly refer to 
> > hisi_sas_debugfs_init() in drivers/scsi/hisi_sas/hisi_sas_main.c. And
> > devm_kfree() is used to free a memory allocated by devm_kmalloc(). And
> > I found other situations similar to this in other files.
> > 
> > So, I hope you can give me some guidance on this. Thanks!
> 
> Please refer to the devres documentation [0].
> 
> Cheers,
> Jon
> 
> [0] https://www.kernel.org/doc/Documentation/driver-model/devres.txt
> 
> -- 
> nvpublic
Thanks for your reply. I figured out that devm_kmalloc will free the 
memory no matter fail or not. But I still want to ask why other codes
as I above mentioned use devm_kfree() to free memory allocated by 
devm_kmalloc(). If the memory is automatically freed, is this 
devm_kfee() redundant codes that should be removed? Am I 
misunderstanding this again or it is something else?

Thanks
Gen

Re: [PATCH] tegra_wm9712: Fix a memory leaking bug in tegra_wm9712_driver_probe()

[Gen Zhang]

On Fri, May 24, 2019 at 09:33:13AM +0100, Jon Hunter wrote:
> 
> On 24/05/2019 01:50, Gen Zhang wrote:
> > In tegra_wm9712_driver_probe(), 'machine->codec' is allocated by
> > platform_device_alloc(). When it is NULL, function returns ENOMEM.
> > However, 'machine' is allocated by devm_kzalloc() before this site.
> > Thus we should free 'machine' before function ends to prevent memory
> > leaking.
> 
> Memory allocated by devm_xxx() is automatically freed on failure so this
> is not correct.
Thanks for your comments, Jon. But after I examined the code, I am still
confused about the usage of devm_kmalloc(). You can kindly refer to 
hisi_sas_debugfs_init() in drivers/scsi/hisi_sas/hisi_sas_main.c. And
devm_kfree() is used to free a memory allocated by devm_kmalloc(). And
I found other situations similar to this in other files.

So, I hope you can give me some guidance on this. Thanks!
> 
> > Further, we should free 'machine->util_data', 'machine->codec' and
> > 'machine' before this function normally ends to prevent memory leaking.
> 
> This is also incorrect. Why would we free all resources after
> successfully initialising the driver?
I re-checked this part, and it is totally incorrect. It should be deleted.

Thanks
Gen

Re: [PATCH v2] initramfs: Fix a missing-chek bug in dir_add()teven.pr...@arm.com、

[Gen Zhang]

On Thu, May 23, 2019 at 08:35:23PM -0700, Andrew Morton wrote:
> On Fri, 24 May 2019 11:30:45 +0800 Gen Zhang  wrote:
> 
> > In dir_add() and do_name(), de->name and vcollected are allocated by
> > kstrdup(). And de->name and vcollected are dereferenced in the following
> > codes. However, memory allocation functions such as kstrdup() may fail. 
> > Dereferencing this null pointer may cause the kernel go wrong. Thus we
> > should check these two kstrdup() operations.
> > Further, if kstrdup() returns NULL, we should free de in dir_add().
> 
> We generally assume that memory allocations within __init code cannot
> fail.  If one does fail, something quite horrid has happened.  The
> resulting oops will provide the same information as the proposed panic()
> anyway.
Thanks for your reply, Andrew.
You mean that it is not necessary to check memory allcoation in __init,
right?
Thanks
Gen

[PATCH v2] initramfs: Fix a missing-chek bug in dir_add()

[Gen Zhang]

In dir_add() and do_name(), de->name and vcollected are allocated by
kstrdup(). And de->name and vcollected are dereferenced in the following
codes. However, memory allocation functions such as kstrdup() may fail. 
Dereferencing this null pointer may cause the kernel go wrong. Thus we
should check these two kstrdup() operations.
Further, if kstrdup() returns NULL, we should free de in dir_add().

Signed-off-by: Gen Zhang 
---
diff --git a/init/initramfs.c b/init/initramfs.c
index 178130f..1421488 100644
--- a/init/initramfs.c
+++ b/init/initramfs.c
@@ -125,6 +125,10 @@ static void __init dir_add(const char *name, time64_t 
mtime)
panic("can't allocate dir_entry buffer");
INIT_LIST_HEAD(>list);
de->name = kstrdup(name, GFP_KERNEL);
+   if (!de->name) {
+   kfree(de);
+   panic("can't allocate dir_entry name buffer");
+   }
de->mtime = mtime;
list_add(>list, _list);
 }
@@ -340,6 +344,10 @@ static int __init do_name(void)
if (body_len)
ksys_ftruncate(wfd, body_len);
vcollected = kstrdup(collected, GFP_KERNEL);
+   if (!vcollected) {
+   panic("can't allocate vcollected 
buffer");
+   return 0;
+   }
state = CopyFile;
}
}

[PATCH] ip_sockglue: Fix missing-check bug in ip_ra_control()

[Gen Zhang]

In function ip_ra_control(), the pointer new_ra is allocated a memory 
space via kmalloc(). And it is used in the following codes. However, 
when  there is a memory allocation error, kmalloc() fails. Thus null 
pointer dereference may happen. And it will cause the kernel to crash. 
Therefore, we should check the return value and handle the error.

Signed-off-by: Gen Zhang 

---
diff --git a/net/ipv4/ip_sockglue.c b/net/ipv4/ip_sockglue.c
index 82f341e..aa3fd61 100644
--- a/net/ipv4/ip_sockglue.c
+++ b/net/ipv4/ip_sockglue.c
@@ -343,6 +343,8 @@ int ip_ra_control(struct sock *sk, unsigned char on,
return -EINVAL;
 
new_ra = on ? kmalloc(sizeof(*new_ra), GFP_KERNEL) : NULL;
+   if (on && !new_ra)
+   return -ENOMEM;
 
mutex_lock(>ipv4.ra_mutex);
for (rap = >ipv4.ra_chain;

[PATCH] ipv6_sockglue: Fix a missing-check bug in ip6_ra_control()

[Gen Zhang]

In function ip6_ra_control(), the pointer new_ra is allocated a memory 
space via kmalloc(). And it is used in the following codes. However, 
when there is a memory allocation error, kmalloc() fails. Thus null 
pointer dereference may happen. And it will cause the kernel to crash. 
Therefore, we should check the return value and handle the error.

Signed-off-by: Gen Zhang 

---
diff --git a/net/ipv6/ipv6_sockglue.c b/net/ipv6/ipv6_sockglue.c
index 40f21fe..0a3d035 100644
--- a/net/ipv6/ipv6_sockglue.c
+++ b/net/ipv6/ipv6_sockglue.c
@@ -68,6 +68,8 @@ int ip6_ra_control(struct sock *sk, int sel)
return -ENOPROTOOPT;
 
new_ra = (sel >= 0) ? kmalloc(sizeof(*new_ra), GFP_KERNEL) : NULL;
+   if (sel >= 0 && !new_ra)
+   return -ENOMEM;
 
write_lock_bh(_ra_lock);
for (rap = _ra_chain; (ra = *rap) != NULL; rap = >next) {

[PATCH] wlcore: spi: Fix a memory leaking bug in wl1271_probe()

[Gen Zhang]

In wl1271_probe(), 'glue->core' is allocated by platform_device_alloc(),
when this allocation fails, ENOMEM is returned. However, 'pdev_data'
and 'glue' are allocated by devm_kzalloc() before 'glue->core'. When
platform_device_alloc() returns NULL, we should also free 'pdev_data'
and 'glue' before wl1271_probe() ends to prevent leaking memory.

Similarly, we shoulf free 'pdev_data' when 'glue' is NULL. And we should
free 'pdev_data' and 'glue' when 'glue->reg' is error and when 'ret' is
error.

Further, we should free 'glue->core', 'pdev_data' and 'glue' when this 
function normally ends to prevent leaking memory.

Signed-off-by: Gen Zhang 
---
diff --git a/drivers/net/wireless/ti/wlcore/spi.c 
b/drivers/net/wireless/ti/wlcore/spi.c
index 62ce54a..ea0ec26 100644
--- a/drivers/net/wireless/ti/wlcore/spi.c
+++ b/drivers/net/wireless/ti/wlcore/spi.c
@@ -480,7 +480,7 @@ static int wl1271_probe(struct spi_device *spi)
struct wl12xx_spi_glue *glue;
struct wlcore_platdev_data *pdev_data;
struct resource res[1];
-   int ret;
+   int ret = -ENOMEM;
 
pdev_data = devm_kzalloc(>dev, sizeof(*pdev_data), GFP_KERNEL);
if (!pdev_data)
@@ -491,7 +491,8 @@ static int wl1271_probe(struct spi_device *spi)
glue = devm_kzalloc(>dev, sizeof(*glue), GFP_KERNEL);
if (!glue) {
dev_err(>dev, "can't allocate glue\n");
-   return -ENOMEM;
+   ret = -ENOMEM;
+   goto out_free1;
}
 
glue->dev = >dev;
@@ -503,31 +504,35 @@ static int wl1271_probe(struct spi_device *spi)
spi->bits_per_word = 32;
 
glue->reg = devm_regulator_get(>dev, "vwlan");
-   if (PTR_ERR(glue->reg) == -EPROBE_DEFER)
-   return -EPROBE_DEFER;
+   if (PTR_ERR(glue->reg) == -EPROBE_DEFER) {
+   ret = -EPROBE_DEFER;
+   goto out_free2;
+   }
if (IS_ERR(glue->reg)) {
dev_err(glue->dev, "can't get regulator\n");
-   return PTR_ERR(glue->reg);
+   ret = PTR_ERR(glue->reg);
+   goto out_free2;
}
 
ret = wlcore_probe_of(spi, glue, pdev_data);
if (ret) {
dev_err(glue->dev,
"can't get device tree parameters (%d)\n", ret);
-   return ret;
+   goto out_free2;
}
 
ret = spi_setup(spi);
if (ret < 0) {
dev_err(glue->dev, "spi_setup failed\n");
-   return ret;
+   goto out_free2;
}
 
glue->core = platform_device_alloc(pdev_data->family->name,
   PLATFORM_DEVID_AUTO);
if (!glue->core) {
dev_err(glue->dev, "can't allocate platform_device\n");
-   return -ENOMEM;
+   ret = -ENOMEM;
+   goto out_free2;
}
 
glue->core->dev.parent = >dev;
@@ -557,10 +562,14 @@ static int wl1271_probe(struct spi_device *spi)
goto out_dev_put;
}
 
-   return 0;
+   ret =  0;
 
 out_dev_put:
platform_device_put(glue->core);
+out_free2:
+   devm_kfree(>dev, glue);
+out_free1:
+   devm_kfree(>dev, pdev_data);
return ret;
 }
 
---

[PATCH v2] wlcore: sdio: Fix a memory leaking bug in wl1271_probe()

[Gen Zhang]

In wl1271_probe(), 'glue->core' is allocated by platform_device_alloc(),
when this allocation fails, ENOMEM is returned. However, 'pdev_data'
and 'glue' are allocated by devm_kzalloc() before 'glue->core'. When
platform_device_alloc() returns NULL, we should also free 'pdev_data'
and 'glue' before wl1271_probe() ends to prevent leaking memory.

Similarly, we should free 'pdev_data' when 'glue' is NULL. And we
should free 'pdev_data' and 'glue' when 'ret' is error.

Further, we shoulf free 'glue->dev', 'pdev_data' and 'glue' when this
function normally ends to prevent memory leaking.

Signed-off-by: Gen Zhang 
---
diff --git a/drivers/net/wireless/ti/wlcore/sdio.c 
b/drivers/net/wireless/ti/wlcore/sdio.c
index 4d4b0770..9110891 100644
--- a/drivers/net/wireless/ti/wlcore/sdio.c
+++ b/drivers/net/wireless/ti/wlcore/sdio.c
@@ -298,8 +298,10 @@ static int wl1271_probe(struct sdio_func *func,
pdev_data->if_ops = _ops;
 
glue = devm_kzalloc(>dev, sizeof(*glue), GFP_KERNEL);
-   if (!glue)
-   return -ENOMEM;
+   if (!glue) {
+   ret = -ENOMEM;
+   goto out_free1;
+   }
 
glue->dev = >dev;
 
@@ -311,7 +313,7 @@ static int wl1271_probe(struct sdio_func *func,
 
ret = wlcore_probe_of(>dev, , , pdev_data);
if (ret)
-   goto out;
+   goto out_free2;
 
/* if sdio can keep power while host is suspended, enable wow */
mmcflags = sdio_get_host_pm_caps(func);
@@ -340,7 +342,7 @@ static int wl1271_probe(struct sdio_func *func,
if (!glue->core) {
dev_err(glue->dev, "can't allocate platform_device");
ret = -ENOMEM;
-   goto out;
+   goto out_free2;
}
 
glue->core->dev.parent = >dev;
@@ -380,12 +382,17 @@ static int wl1271_probe(struct sdio_func *func,
dev_err(glue->dev, "can't add platform device\n");
goto out_dev_put;
}
-   return 0;
+   ret = 0;
 
 out_dev_put:
platform_device_put(glue->core);
 
-out:
+out_free2:
+   devm_kfree(>dev, glue);
+
+out_free1:
+   devm_kfree(>dev, pdev_data);
+
return ret;
 }
 
---

[PATCH v3] vt: Fix a missing-check bug in con_init()

[Gen Zhang]

In function con_init(), the pointer variable vc_cons[currcons].d, vc and
vc->vc_screenbuf is allocated a memory space via kzalloc(). And they are
used in the following codes.
However, when there is a memory allocation error, kzalloc() can fail.
Thus null pointer (vc_cons[currcons].d, vc and vc->vc_screenbuf)
dereference may happen. And it will cause the kernel to crash. Therefore,
we should check return value and handle the error.
Further, since the allcoation is in a loop, we should free all the 
allocated memory in a loop.

Signed-off-by: Gen Zhang 
Reviewed-by: Nicolas Pitre 
---
diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
index fdd12f8..d50f68f 100644
--- a/drivers/tty/vt/vt.c
+++ b/drivers/tty/vt/vt.c
@@ -3350,10 +3350,14 @@ static int __init con_init(void)
 
for (currcons = 0; currcons < MIN_NR_CONSOLES; currcons++) {
vc_cons[currcons].d = vc = kzalloc(sizeof(struct vc_data), 
GFP_NOWAIT);
+   if (!vc)
+   goto fail1;
INIT_WORK(_cons[currcons].SAK_work, vc_SAK);
tty_port_init(>port);
visual_init(vc, currcons, 1);
vc->vc_screenbuf = kzalloc(vc->vc_screenbuf_size, GFP_NOWAIT);
+   if (!vc->vc_screenbuf)
+   goto fail2;
vc_init(vc, vc->vc_rows, vc->vc_cols,
currcons || !vc->vc_sw->con_save_screen);
}
@@ -3375,6 +3379,16 @@ static int __init con_init(void)
register_console(_console_driver);
 #endif
return 0;
+fail1:
+   while (currcons > 0) {
+   currcons--;
+   kfree(vc_cons[currcons].d->vc_screenbuf);
+fail2:
+   kfree(vc_cons[currcons].d);
+   vc_cons[currcons].d = NULL;
+   }
+   console_unlock();
+   return -ENOMEM;
 }
 console_initcall(con_init);
 
---

[PATCH v2] consolemap: Fix a memory leaking bug in con_insert_unipair()

[Gen Zhang]

In function con_insert_unipair(), when allocation for p2 and p1[n]
fails, ENOMEM is returned, but previously allocated p1 is not freed, 
remains as leaking memory. Thus we should free p1 as well when this
allocation fails.

Signed-off-by: Gen Zhang 
Reviewed-by: Kees Cook 
---
diff --git a/drivers/tty/vt/consolemap.c b/drivers/tty/vt/consolemap.c
index b28aa0d..79fcc96 100644
--- a/drivers/tty/vt/consolemap.c
+++ b/drivers/tty/vt/consolemap.c
@@ -489,7 +489,11 @@ con_insert_unipair(struct uni_pagedir *p, u_short unicode, 
u_short fontpos)
p2 = p1[n = (unicode >> 6) & 0x1f];
if (!p2) {
p2 = p1[n] = kmalloc_array(64, sizeof(u16), GFP_KERNEL);
-   if (!p2) return -ENOMEM;
+   if (!p2) {
+   kfree(p1);
+   p->uni_pgdir[n] = NULL;
+   return -ENOMEM;
+   }
memset(p2, 0xff, 64*sizeof(u16)); /* No glyphs for the 
characters (yet) */
}
 
---

Re: [PATCH v2] consolemap: Fix a memory leaking bug in drivers/tty/vt/consolemap.c

[Gen Zhang]

On Thu, May 23, 2019 at 09:54:18AM -0700, Kees Cook wrote:
> On Thu, May 23, 2019 at 08:34:52AM +0800, Gen Zhang wrote:
> > In function con_insert_unipair(), when allocation for p2 and p1[n]
> > fails, ENOMEM is returned, but previously allocated p1 is not freed, 
> > remains as leaking memory. Thus we should free p1 as well when this
> > allocation fails.
> > 
> > Signed-off-by: Gen Zhang 
> 
> As far as I can see this is correct, as it's just restoring the prior
> state before the p1 allocation.
> 
> Reviewed-by: Kees Cook 
> 
Thanks for your review, Kees!

Thanks
Gen
> > ---
> > diff --git a/drivers/tty/vt/consolemap.c b/drivers/tty/vt/consolemap.c
> > index b28aa0d..79fcc96 100644
> > --- a/drivers/tty/vt/consolemap.c
> > +++ b/drivers/tty/vt/consolemap.c
> > @@ -489,7 +489,11 @@ con_insert_unipair(struct uni_pagedir *p, u_short 
> > unicode, u_short fontpos)
> > p2 = p1[n = (unicode >> 6) & 0x1f];
> > if (!p2) {
> > p2 = p1[n] = kmalloc_array(64, sizeof(u16), GFP_KERNEL);
> > -   if (!p2) return -ENOMEM;
> > +   if (!p2) {
> > +   kfree(p1);
> > +   p->uni_pgdir[n] = NULL;
> > +   return -ENOMEM;
> > +   }
> > memset(p2, 0xff, 64*sizeof(u16)); /* No glyphs for the 
> > characters (yet) */
> > }
> >  
> > ---
> 
> -- 
> Kees Cook

[PATCH] pcm030-audio-fabric: Fix a memory leaking bug in pcm030_fabric_probe()

[Gen Zhang]

In pcm030_fabric_probe(), 'pdata->codec_device' is allocated by
platform_device_alloc(). When this allocation fails, ENOMEM is returned.
However, 'pdata' is allocated by devm_kzalloc() before this site. We
should free 'pdata' before function ends to prevent memory leaking.

Similarly, we should free 'pdata' when 'pdata->codec_device' is NULL.
And we should free 'pdata->codec_device' and 'pdata' when 'ret' is error
to prevent memory leaking.

Signed-off-by: Gen Zhang 
---
diff --git a/sound/soc/fsl/pcm030-audio-fabric.c 
b/sound/soc/fsl/pcm030-audio-fabric.c
index a7fe4ad..d2e6eae 100644
--- a/sound/soc/fsl/pcm030-audio-fabric.c
+++ b/sound/soc/fsl/pcm030-audio-fabric.c
@@ -72,29 +72,43 @@ static int pcm030_fabric_probe(struct platform_device *op)
platform_np = of_parse_phandle(np, "asoc-platform", 0);
if (!platform_np) {
dev_err(>dev, "ac97 not registered\n");
-   return -ENODEV;
+   ret = -ENODEV;
+   goto out_free1;
}
 
for_each_card_prelinks(card, i, dai_link)
dai_link->platform_of_node = platform_np;
 
ret = request_module("snd-soc-wm9712");
-   if (ret)
+   if (ret) {
dev_err(>dev, "request_module returned: %d\n", ret);
+   goto out_free1;
+   }
 
pdata->codec_device = platform_device_alloc("wm9712-codec", -1);
-   if (!pdata->codec_device)
+   if (!pdata->codec_device) {
dev_err(>dev, "platform_device_alloc() failed\n");
+   ret = -ENOMEM;
+   goto out_free1;
+   }
 
ret = platform_device_add(pdata->codec_device);
-   if (ret)
+   if (ret) {
dev_err(>dev, "platform_device_add() failed: %d\n", ret);
+   goto out_free2;
+   }
 
ret = snd_soc_register_card(card);
-   if (ret)
+   if (ret) {
dev_err(>dev, "snd_soc_register_card() failed: %d\n", ret);
+   goto out_free2;
+   }
 
platform_set_drvdata(op, pdata);
+out_free2:
+   platform_device_put(pdata->codec_device);
+out_free1:
+   devm_kfree(>dev, pdata);
 
return ret;
 }
---

[PATCH] tegra_wm9712: Fix a memory leaking bug in tegra_wm9712_driver_probe()

[Gen Zhang]

In tegra_wm9712_driver_probe(), 'machine->codec' is allocated by
platform_device_alloc(). When it is NULL, function returns ENOMEM.
However, 'machine' is allocated by devm_kzalloc() before this site.
Thus we should free 'machine' before function ends to prevent memory
leaking.

Further, we should free 'machine->util_data', 'machine->codec' and
'machine' before this function normally ends to prevent memory leaking.

Signed-off-by: Gen Zhang 
---
diff --git a/sound/soc/tegra/tegra_wm9712.c b/sound/soc/tegra/tegra_wm9712.c
index 864a334..295c41d 100644
--- a/sound/soc/tegra/tegra_wm9712.c
+++ b/sound/soc/tegra/tegra_wm9712.c
@@ -86,7 +86,8 @@ static int tegra_wm9712_driver_probe(struct platform_device 
*pdev)
machine->codec = platform_device_alloc("wm9712-codec", -1);
if (!machine->codec) {
dev_err(>dev, "Can't allocate wm9712 platform device\n");
-   return -ENOMEM;
+   ret = -ENOMEM;
+   goto codec_free;
}
 
ret = platform_device_add(machine->codec);
@@ -127,6 +128,10 @@ static int tegra_wm9712_driver_probe(struct 
platform_device *pdev)
goto asoc_utils_fini;
}
 
+   tegra_asoc_utils_fini(>util_data);
+   platform_device_del(machine->codec);
+   platform_device_put(machine->codec);
+   devm_kfree(>dev, machine);
return 0;
 
 asoc_utils_fini:
@@ -135,6 +140,8 @@ static int tegra_wm9712_driver_probe(struct platform_device 
*pdev)
platform_device_del(machine->codec);
 codec_put:
platform_device_put(machine->codec);
+codec_free:
+   devm_kfree(>dev, machine);
return ret;
 }
 
---

[PATCH v2] sdio: Fix a memory leaking bug in wl1271_probe()

[Gen Zhang]

In wl1271_probe(), 'glue->core' is allocated by platform_device_alloc(),
when this allocation fails, ENOMEM is returned. However, 'pdev_data'
and 'glue' are allocated by devm_kzalloc() before 'glue->core'. When
platform_device_alloc() returns NULL, we should also free 'pdev_data'
and 'glue' before wl1271_probe() ends to prevent leaking memory.

Similarly, we should free 'pdev_data' when 'glue' is NULL. And we
should free 'pdev_data' and 'glue' when 'ret' is error.

Further, we shoulf free 'glue->dev', 'pdev_data' and 'glue' when this
function normally ends to prevent memory leaking.

Signed-off-by: Gen Zhang 
---
diff --git a/drivers/net/wireless/ti/wlcore/sdio.c 
b/drivers/net/wireless/ti/wlcore/sdio.c
index 4d4b0770..232ce5f 100644
--- a/drivers/net/wireless/ti/wlcore/sdio.c
+++ b/drivers/net/wireless/ti/wlcore/sdio.c
@@ -298,8 +298,10 @@ static int wl1271_probe(struct sdio_func *func,
pdev_data->if_ops = _ops;
 
glue = devm_kzalloc(>dev, sizeof(*glue), GFP_KERNEL);
-   if (!glue)
-   return -ENOMEM;
+   if (!glue) {
+   ret = -ENOMEM;
+   goto out_free1;
+   }
 
glue->dev = >dev;
 
@@ -311,7 +313,7 @@ static int wl1271_probe(struct sdio_func *func,
 
ret = wlcore_probe_of(>dev, , , pdev_data);
if (ret)
-   goto out;
+   goto out_free2;
 
/* if sdio can keep power while host is suspended, enable wow */
mmcflags = sdio_get_host_pm_caps(func);
@@ -340,7 +342,7 @@ static int wl1271_probe(struct sdio_func *func,
if (!glue->core) {
dev_err(glue->dev, "can't allocate platform_device");
ret = -ENOMEM;
-   goto out;
+   goto out_free2;
}
 
glue->core->dev.parent = >dev;
@@ -380,11 +382,20 @@ static int wl1271_probe(struct sdio_func *func,
dev_err(glue->dev, "can't add platform device\n");
goto out_dev_put;
}
+   platform_device_put(glue->core);
+   devm_kfree(>dev, glue);
+   devm_kfree(>dev, pdev_data);
return 0;
 
 out_dev_put:
platform_device_put(glue->core);
 
+out_free2:
+   devm_kfree(>dev, glue);
+
+out_free1:
+   devm_kfree(>dev, pdev_data);
+
 out:
return ret;
 }
---

[PATCH] spi: Fix a memory leaking bug in wl1271_probe()

[Gen Zhang]

In wl1271_probe(), 'glue->core' is allocated by platform_device_alloc(),
when this allocation fails, ENOMEM is returned. However, 'pdev_data'
and 'glue' are allocated by devm_kzalloc() before 'glue->core'. When
platform_device_alloc() returns NULL, we should also free 'pdev_data'
and 'glue' before wl1271_probe() ends to prevent leaking memory.

Similarly, we shoulf free 'pdev_data' when 'glue' is NULL. And we should
free 'pdev_data' and 'glue' when 'glue->reg' is error and when 'ret' is
error.

Further, we should free 'glue->core', 'pdev_data' and 'glue' when this 
function normally ends to prevent leaking memory.

Signed-off-by: Gen Zhang 
---
diff --git a/drivers/net/wireless/ti/wlcore/spi.c 
b/drivers/net/wireless/ti/wlcore/spi.c
index 62ce54a..3a020bd 100644
--- a/drivers/net/wireless/ti/wlcore/spi.c
+++ b/drivers/net/wireless/ti/wlcore/spi.c
@@ -480,7 +480,7 @@ static int wl1271_probe(struct spi_device *spi)
struct wl12xx_spi_glue *glue;
struct wlcore_platdev_data *pdev_data;
struct resource res[1];
-   int ret;
+   int ret = -ENOMEM;
 
pdev_data = devm_kzalloc(>dev, sizeof(*pdev_data), GFP_KERNEL);
if (!pdev_data)
@@ -491,7 +491,8 @@ static int wl1271_probe(struct spi_device *spi)
glue = devm_kzalloc(>dev, sizeof(*glue), GFP_KERNEL);
if (!glue) {
dev_err(>dev, "can't allocate glue\n");
-   return -ENOMEM;
+   ret = -ENOMEM;
+   goto out_free1;
}
 
glue->dev = >dev;
@@ -503,31 +504,35 @@ static int wl1271_probe(struct spi_device *spi)
spi->bits_per_word = 32;
 
glue->reg = devm_regulator_get(>dev, "vwlan");
-   if (PTR_ERR(glue->reg) == -EPROBE_DEFER)
-   return -EPROBE_DEFER;
+   if (PTR_ERR(glue->reg) == -EPROBE_DEFER) {
+   ret = -EPROBE_DEFER;
+   goto out_free2;
+   }
if (IS_ERR(glue->reg)) {
dev_err(glue->dev, "can't get regulator\n");
-   return PTR_ERR(glue->reg);
+   ret = PTR_ERR(glue->reg);
+   goto out_free2;
}
 
ret = wlcore_probe_of(spi, glue, pdev_data);
if (ret) {
dev_err(glue->dev,
"can't get device tree parameters (%d)\n", ret);
-   return ret;
+   goto out_free2;
}
 
ret = spi_setup(spi);
if (ret < 0) {
dev_err(glue->dev, "spi_setup failed\n");
-   return ret;
+   goto out_free2;
}
 
glue->core = platform_device_alloc(pdev_data->family->name,
   PLATFORM_DEVID_AUTO);
if (!glue->core) {
dev_err(glue->dev, "can't allocate platform_device\n");
-   return -ENOMEM;
+   ret = -ENOMEM;
+   goto out_free2;
}
 
glue->core->dev.parent = >dev;
@@ -557,10 +562,18 @@ static int wl1271_probe(struct spi_device *spi)
goto out_dev_put;
}
 
+   platform_device_put(glue->core);
+   devm_kfree(>dev, glue);
+   devm_kfree(>dev, pdev_data);
return 0;
 
 out_dev_put:
platform_device_put(glue->core);
+out_free2:
+   devm_kfree(>dev, glue);
+out_free1:
+   devm_kfree(>dev, pdev_data);
+out:
return ret;
 }
 
---

[PATCH] sdio: Fix a memory leaking bug in wl1271_probe()

[Gen Zhang]

In wl1271_probe(), 'glue->core' is allocated by platform_device_alloc(),
when this allocation fails, ENOMEM is returned. However, 'pdev_data'
and 'glue' are allocated by devm_kzalloc() before 'glue->core'. When
platform_device_alloc() returns NULL, we should also free 'pdev_data'
and 'glue' before wl1271_probe() ends to prevent leaking memory.

Signed-off-by: Gen Zhang 
---
diff --git a/drivers/net/wireless/ti/wlcore/sdio.c 
b/drivers/net/wireless/ti/wlcore/sdio.c
index 4d4b0770..e134f30 100644
--- a/drivers/net/wireless/ti/wlcore/sdio.c
+++ b/drivers/net/wireless/ti/wlcore/sdio.c
@@ -340,7 +340,7 @@ static int wl1271_probe(struct sdio_func *func,
if (!glue->core) {
dev_err(glue->dev, "can't allocate platform_device");
ret = -ENOMEM;
-   goto out;
+   goto out_free;
}
 
glue->core->dev.parent = >dev;
@@ -385,6 +385,10 @@ static int wl1271_probe(struct sdio_func *func,
 out_dev_put:
platform_device_put(glue->core);
 
+out_free:
+   devm_kfree(>dev, pdev_data);
+   devm_kfree(>dev, glue);
+
 out:
return ret;
 }
---

[PATCH] sg: Fix a double-fetch bug in drivers/scsi/sg.c

[Gen Zhang]

In sg_write(), the opcode of the command is fetched the first time from 
the userspace by __get_user(). Then the whole command, the opcode 
included, is fetched again from userspace by __copy_from_user(). 
However, a malicious user can change the opcode between the two fetches.
This can cause inconsistent data and potential errors as cmnd is used in
the following codes.

Thus we should check opcode between the two fetches to prevent this.

Signed-off-by: Gen Zhang 
---
diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index d3f1531..a2971b8 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -694,6 +694,8 @@ sg_write(struct file *filp, const char __user *buf, size_t 
count, loff_t * ppos)
hp->flags = input_size; /* structure abuse ... */
hp->pack_id = old_hdr.pack_id;
hp->usr_ptr = NULL;
+   if (opcode != cmnd[0])
+   return -EINVAL;
if (__copy_from_user(cmnd, buf, cmd_size))
return -EFAULT;
/*
---

[A General Question] What should I do after getting Reviewed-by from a maintainer?

[Gen Zhang]

Hi Andrew,
I am starting submitting patches these days and got some patches 
"Reviewed-by" from maintainers. After checking the 
submitting-patches.html, I figured out what "Reviewed-by" means. But I
didn't get the guidance on what to do after getting "Reviewed-by".
Am I supposed to send this patch to more maintainers? Or something else?
Thanks
Gen

[PATCH v2] consolemap: Fix a memory leaking bug in drivers/tty/vt/consolemap.c

[Gen Zhang]

In function con_insert_unipair(), when allocation for p2 and p1[n]
fails, ENOMEM is returned, but previously allocated p1 is not freed, 
remains as leaking memory. Thus we should free p1 as well when this
allocation fails.

Signed-off-by: Gen Zhang 
---
diff --git a/drivers/tty/vt/consolemap.c b/drivers/tty/vt/consolemap.c
index b28aa0d..79fcc96 100644
--- a/drivers/tty/vt/consolemap.c
+++ b/drivers/tty/vt/consolemap.c
@@ -489,7 +489,11 @@ con_insert_unipair(struct uni_pagedir *p, u_short unicode, 
u_short fontpos)
p2 = p1[n = (unicode >> 6) & 0x1f];
if (!p2) {
p2 = p1[n] = kmalloc_array(64, sizeof(u16), GFP_KERNEL);
-   if (!p2) return -ENOMEM;
+   if (!p2) {
+   kfree(p1);
+   p->uni_pgdir[n] = NULL;
+   return -ENOMEM;
+   }
memset(p2, 0xff, 64*sizeof(u16)); /* No glyphs for the 
characters (yet) */
}
 
---

Re: [PATCH] consolemap: Fix a memory leaking bug in drivers/tty/vt/consolemap.c

[Gen Zhang]

On Wed, May 22, 2019 at 01:54:47PM -0700, Kees Cook wrote:
> On Wed, May 22, 2019 at 09:50:55AM +0800, Gen Zhang wrote:
> > On Tue, May 21, 2019 at 01:44:33PM -0700, Kees Cook wrote:
> > > This doesn't look safe to me: p->uni_pgdir[n] will still have a handle
> > > to the freed memory, won't it?
> > > 
> > Thanks for your reply, Kees!
> > I think you are right. Maybe we should do this:
> > kfree(p1);
> > p->uni_pgdir[n] = NULL;
> > Is this correct?
> 
> That's what I'm not sure about. I *think* so, from reading the code, but
> I'd love to have Greg (or someone more familiar with the code) to
> double-check this.
> 
> Otherwise, yeah, this looks right. Please send a v2 and we can debate
> the correctness there, if it turns out to be wrong. :)
> 
> -- 
> Kees Cook
Thanks for your suggestions, Kees.
I follow your guidance and work on resubmitting a patch.
Thanks
Gen

[PATCH] mpt3sas_ctl: Fix a double-fetch bug in drivers/scsi/mpt3sas/mpt3sas_ctl.c

[Gen Zhang]

In _ctl_ioctl_main(), 'ioctl_header' is fetched the first time from 
userspace. 'ioctl_header.ioc_number' is then checked. The legal result 
is saved to 'ioc'. Then, in condition MPT3COMMAND, the whole struct is
fetched again from the userspace. Then _ctl_do_mpt_command() is called,
'ioc' and 'karg' as inputs.

However, a malicious user can change the 'ioc_number' between the two 
fetches, which will cause a potential security issues.  Moreover, a 
malicious user can provide a valid 'ioc_number' to pass the check in 
first fetch, and then modify it in the second fetch.

To fix this, we need to recheck the 'ioc_number' in the second fetch.

Signed-off-by: Gen Zhang 
---
diff --git a/drivers/scsi/mpt3sas/mpt3sas_ctl.c 
b/drivers/scsi/mpt3sas/mpt3sas_ctl.c
index b2bb47c..5181c03 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_ctl.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_ctl.c
@@ -2319,6 +2319,10 @@ _ctl_ioctl_main(struct file *file, unsigned int cmd, 
void __user *arg,
break;
}
 
+   if (karg.hdr.ioc_number != ioctl_header.ioc_number) {
+   ret = -EINVAL;
+   break;
+   }
if (_IOC_SIZE(cmd) == sizeof(struct mpt3_ioctl_command)) {
uarg = arg;
ret = _ctl_do_mpt_command(ioc, karg, >mf);

[PATCH v3] vt: Fix a missing-check bug in drivers/tty/vt/vt.c

[Gen Zhang]

In function con_init(), the pointer variable vc_cons[currcons].d, vc and
vc->vc_screenbuf is allocated a memory space via kzalloc(). And they are
used in the following codes.
However, when there is a memory allocation error, kzalloc() can fail.
Thus null pointer (vc_cons[currcons].d, vc and vc->vc_screenbuf)
dereference may happen. And it will cause the kernel to crash. Therefore,
we should check return value and handle the error.
Further, since the allcoation is in a loop, we should free all the 
allocated memory in a loop.

Signed-off-by: Gen Zhang 

---
diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
index fdd12f8..d50f68f 100644
--- a/drivers/tty/vt/vt.c
+++ b/drivers/tty/vt/vt.c
@@ -3350,10 +3350,14 @@ static int __init con_init(void)
 
for (currcons = 0; currcons < MIN_NR_CONSOLES; currcons++) {
vc_cons[currcons].d = vc = kzalloc(sizeof(struct vc_data), 
GFP_NOWAIT);
+   if (!vc)
+   goto fail1;
INIT_WORK(_cons[currcons].SAK_work, vc_SAK);
tty_port_init(>port);
visual_init(vc, currcons, 1);
vc->vc_screenbuf = kzalloc(vc->vc_screenbuf_size, GFP_NOWAIT);
+   if (!vc->vc_screenbuf)
+   goto fail2;
vc_init(vc, vc->vc_rows, vc->vc_cols,
currcons || !vc->vc_sw->con_save_screen);
}
@@ -3375,6 +3379,16 @@ static int __init con_init(void)
register_console(_console_driver);
 #endif
return 0;
+fail1:
+   while (currcons > 0) {
+   currcons--;
+   kfree(vc_cons[currcons].d->vc_screenbuf);
+fail2:
+   kfree(vc_cons[currcons].d);
+   vc_cons[currcons].d = NULL;
+   }
+   console_unlock();
+   return -ENOMEM;
 }
 console_initcall(con_init);
 
---

Re: [PATCH] tty_io: Fix a missing-check bug in drivers/tty/tty_io.c

[Gen Zhang]

On Wed, May 22, 2019 at 01:19:49PM +0200, Johan Hovold wrote:
> On Wed, May 22, 2019 at 07:13:54PM +0800, Gen Zhang wrote:
> > On Wed, May 22, 2019 at 12:29:00PM +0200, Johan Hovold wrote:
> > > Where do you see that the kernel is dereferencing tty->dev without
> > > checking for NULL first? If you can find that, then that would indeed be
> > > a bug that needs fixing.
> > Thanks for your reply, Johan!
> > I examined the code but failed to find this situation.
> 
> Ok, so your claim in the commit message was incorrect:
> 
>   And tty->dev is dereferenced in the following codes.
> 
> > Anyway, checking return value of tty_get_device() is theoritically
> > right. But tty->dev is never dereferenced, so checking is not needed.
> 
> No, sorry, it's not even theoretically correct. Our current code depends
> on tty->dev sometimes being NULL. Your patch would specifically break
> pseudo terminals.
Thanks for your comments. I have to be very proficient in this module
to know this. Of course I am not.
Anyway, appreciate your replies, Johan.
Thanks
Gen
> 
> > However, what if in later kernels tty->dev is dereferenced by some
> > codes? Is it better to apply this check for this reason?
> 
> So for the above reason, no.
> 
> Johan

Re: [PATCH] tty_io: Fix a missing-check bug in drivers/tty/tty_io.c

[Gen Zhang]

On Wed, May 22, 2019 at 10:15:56AM +0200, Jiri Slaby wrote:
> Look at the top of alloc_tty_struct: there is tty_ldisc_init. If
> tty_get_device fails here, you have to call tty_ldisc_deinit. Better,
> you should add a failure-handling tail to this function and "goto" there.
Thanks for your explaination, Jiri.
I will work on it.
Thanks
Gen

Re: [PATCH] tty_io: Fix a missing-check bug in drivers/tty/tty_io.c

[Gen Zhang]

On Wed, May 22, 2019 at 12:29:00PM +0200, Johan Hovold wrote:
> Where do you see that the kernel is dereferencing tty->dev without
> checking for NULL first? If you can find that, then that would indeed be
> a bug that needs fixing.
Thanks for your reply, Johan!
I examined the code but failed to find this situation.
Anyway, checking return value of tty_get_device() is theoritically
right. But tty->dev is never dereferenced, so checking is not needed.
However, what if in later kernels tty->dev is dereferenced by some
codes? Is it better to apply this check for this reason?
Thanks
Gen

Re: [PATCH v2] vt: Fix a missing-check bug in drivers/tty/vt/vt.c

[Gen Zhang]

On Tue, May 21, 2019 at 10:43:11PM -0400, Nicolas Pitre wrote:
> On Tue, 21 May 2019, Gen Zhang wrote:
> 
> > On Tue, May 21, 2019 at 12:30:38AM -0400, Nicolas Pitre wrote:
> > > Now imagine that MIN_NR_CONSOLES is defined to 10 instead of 1.
> > > 
> > > What happens with allocated memory if the err_vc condition is met on the 
> > > 5th loop?
> > Yes, vc->vc_screenbuf from the last loop is still not freed, right? I
> > don't have idea to solve this one. Could please give some advice? Since
> > we have to consider the err_vc condition.
> > 
> > > If err_vc_screenbuf condition is encountered on the 5th loop (curcons = 
> > > 4), what is the value of vc_cons[4].d? Isn't it the same as vc that you 
> > > just freed?
> > > 
> > > 
> > > Nicolas
> > Thanks for your explaination! You mean a double free situation may 
> > happen, right? But in vc_allocate() there is also such a kfree(vc) and 
> > vc_cons[currcons].d = NULL operation. This situation is really confusing
> > me.
> 
> What you could do is something that looks like:
> 
>   for (currcons = 0; currcons < MIN_NR_CONSOLES; currcons++) {
>   vc_cons[currcons].d = vc = kzalloc(...);
>   if (!vc)
>   goto fail1;
>   ...
>   vc->vc_screenbuf = kzalloc(...);
>   if (!vc->vc_screenbuf)
>   goto fail2;
>   ...
> 
>   return 0;
> 
>   /* free already allocated memory on error */
> fail1:
>   while (curcons > 0) {
>   curcons--;
>   kfree(vc_cons[currcons].d->vc_screenbuf);
> fail2:
>   kfree(vc_cons[currcons].d);
>   vc_cons[currcons].d = NULL;
>   }
>   console_unlock();
>   return -ENOMEM;
> 
> 
> Nicolas
Thanks for your patient explaination, Nicolas!
I will work on it and resubmit it.
Thanks
Gen

Re: [PATCH] tty_io: Fix a missing-check bug in drivers/tty/tty_io.c

[Gen Zhang]

On Wed, May 22, 2019 at 06:25:36AM +0200, Jiri Slaby wrote:
> On 22. 05. 19, 3:40, Gen Zhang wrote:
> > In alloc_tty_struct(), tty->dev is assigned by tty_get_device(). And it
> > calls class_find_device(). And class_find_device() may return NULL.
> > And tty->dev is dereferenced in the following codes. When 
> > tty_get_device() returns NULL, dereferencing this tty->dev null pointer
> > may cause the kernel go wrong. Thus we should check tty->dev.
> > Further, if tty_get_device() returns NULL, we should free tty and 
> > return NULL.
> > 
> > Signed-off-by: Gen Zhang 
> > 
> > ---
> > diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
> > index 033ac7e..1444b59 100644
> > --- a/drivers/tty/tty_io.c
> > +++ b/drivers/tty/tty_io.c
> > @@ -3008,6 +3008,10 @@ struct tty_struct *alloc_tty_struct(struct 
> > tty_driver *driver, int idx)
> > tty->index = idx;
> > tty_line_name(driver, idx, tty->name);
> > tty->dev = tty_get_device(tty);
> > +   if (!tty->dev) {
> > +   kfree(tty);
> > +   return NULL;
> > +   }
> 
> This is incorrect, you introduced an ldisc reference leak.
Thanks for your reply, Jiri!
And what do you mean by an ldisc reference leak? I did't get the reason
of introducing it.
> 
> And can this happen at all?
I think tty_get_device() may happen to return NULL. Because it calls 
class_find_device() and there's a chance class_find_device() returns
NULL.
Thanks
Gen
> 
> thanks,
> -- 
> js
> suse labs