LKML Archive on lore.kernel.org
 help / color / Atom feed
From: Kees Cook <keescook@chromium.org>
To: Jon Hunter <jonathanh@nvidia.com>, Greg KH <gregkh@linuxfoundation.org>
Cc: Gen Zhang <blackgod016574@gmail.com>,
	jslaby@suse.com, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] consolemap: Fix a memory leaking bug in drivers/tty/vt/consolemap.c
Date: Fri, 24 May 2019 23:04:25 -0700
Message-ID: <201905242302.139A912@keescook> (raw)
In-Reply-To: <b99d0da6-a1d6-1c04-66ff-b2937d21d346@nvidia.com>

On Fri, May 24, 2019 at 10:37:59AM +0100, Jon Hunter wrote:
> Kees,
> 
> On 23/05/2019 17:54, 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 <blackgod016574@gmail.com>
> > 
> > As far as I can see this is correct, as it's just restoring the prior
> > state before the p1 allocation.
> 
> Are you sure this is correct? It looks like p1 is only allocated if
> p->uni_pgdir[n = unicode >> 11] == NULL.

Thanks, yes, I looked more closely, and this is wrong. It's probably
fine to leave it as it was (since it appears to just be allocating
on demand). If we really want to restore the state on failure, we
also can't re-use "n", which is no longer valid. Here, I think,
would be a complete patch to check for allocation and use a separate
index for the other array:


diff --git a/drivers/tty/vt/consolemap.c b/drivers/tty/vt/consolemap.c
index b28aa0d289f8..5f77cffc53b8 100644
--- a/drivers/tty/vt/consolemap.c
+++ b/drivers/tty/vt/consolemap.c
@@ -474,7 +474,8 @@ static int con_unify_unimap(struct vc_data *conp, struct uni_pagedir *p)
 static int
 con_insert_unipair(struct uni_pagedir *p, u_short unicode, u_short fontpos)
 {
-	int i, n;
+	int i, n, high;
+	bool p1_alloced = false;
 	u16 **p1, *p2;
 
 	p1 = p->uni_pgdir[n = unicode >> 11];
@@ -482,14 +483,22 @@ con_insert_unipair(struct uni_pagedir *p, u_short unicode, u_short fontpos)
 		p1 = p->uni_pgdir[n] = kmalloc_array(32, sizeof(u16 *),
 						     GFP_KERNEL);
 		if (!p1) return -ENOMEM;
+		p1_alloced = true;
 		for (i = 0; i < 32; i++)
 			p1[i] = NULL;
 	}
 
-	p2 = p1[n = (unicode >> 6) & 0x1f];
+	p2 = p1[high = (unicode >> 6) & 0x1f];
 	if (!p2) {
-		p2 = p1[n] = kmalloc_array(64, sizeof(u16), GFP_KERNEL);
-		if (!p2) return -ENOMEM;
+		p2 = p1[high] = kmalloc_array(64, sizeof(u16), GFP_KERNEL);
+		if (!p2) {
+			if (p1_alloced) {
+				kfree(p1);
+				p->uni_pgdir[n] = NULL;
+			}
+			return -ENOMEM;
+		}
+
 		memset(p2, 0xff, 64*sizeof(u16)); /* No glyphs for the characters (yet) */
 	}
 

But, frankly, probably the patch should just be removed...

-Kees

> 
> I only mention this because I have seen a few patches from Gen today
> regarding memory leaks and devm_kzalloc() that are not correct.
> 
> > Reviewed-by: Kees Cook <keescook@chromium.org>
> > 
> >> ---
> >> 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) */
> >>  	}
> >>  
> >> ---
> 
> Cheers
> Jon
> 
> -- 
> nvpublic

-- 
Kees Cook

      reply index

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-05-21  9:29 [PATCH] " Gen Zhang
2019-05-21 20:44 ` Kees Cook
2019-05-22  1:50   ` Gen Zhang
2019-05-22 20:54     ` Kees Cook
2019-05-23  0:29       ` Gen Zhang
2019-05-23  0:34       ` [PATCH v2] " Gen Zhang
2019-05-23 16:54         ` Kees Cook
2019-05-24  2:14           ` Gen Zhang
2019-05-24  2:19           ` [PATCH v2] consolemap: Fix a memory leaking bug in con_insert_unipair() Gen Zhang
2019-05-24  8:02             ` Greg KH
2019-05-24  9:37           ` [PATCH v2] consolemap: Fix a memory leaking bug in drivers/tty/vt/consolemap.c Jon Hunter
2019-05-25  6:04             ` Kees Cook [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=201905242302.139A912@keescook \
    --to=keescook@chromium.org \
    --cc=blackgod016574@gmail.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=jonathanh@nvidia.com \
    --cc=jslaby@suse.com \
    --cc=linux-kernel@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git
	git clone --mirror https://lore.kernel.org/lkml/8 lkml/git/8.git
	git clone --mirror https://lore.kernel.org/lkml/9 lkml/git/9.git
	git clone --mirror https://lore.kernel.org/lkml/10 lkml/git/10.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org
	public-inbox-index lkml

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git