Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751100AbdLaPXW (ORCPT ); Sun, 31 Dec 2017 10:23:22 -0500 Received: from mail-pl0-f65.google.com ([209.85.160.65]:34830 "EHLO mail-pl0-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750854AbdLaPXV (ORCPT ); Sun, 31 Dec 2017 10:23:21 -0500 X-Google-Smtp-Source: ACJfBouF0siP1v004NcW6vfwBuAd6nfQM+namumdR6oqu95T5rTquAjwJBNiKTlugRThUJONhzPA1A== Content-Type: text/plain; charset=us-ascii Mime-Version: 1.0 (1.0) Subject: Re: [patch V2 1/3] x86/ldt: Plug memory leak in error path From: Andy Lutomirski X-Mailer: iPhone Mail (15C153) In-Reply-To: Date: Sun, 31 Dec 2017 07:23:18 -0800 Cc: LKML , Linus Torvalds , x86@kernel.org, Andy Lutomirski , Dave Hansen , Peter Zijlstra , Borislav Petkov , Dominik Brodowski , Mathieu Desnoyers Message-Id: <72F8C4C3-B83A-4F4E-A900-644EC359E383@amacapital.net> References: <20171230211351.980176980@linutronix.de> <20171230211829.508293470@linutronix.de> To: Thomas Gleixner Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from quoted-printable to 8bit by mail.home.local id vBVFNSfo004254 Content-Length: 1659 Lines: 41 > On Dec 31, 2017, at 2:24 AM, Thomas Gleixner wrote: > > The error path in write_ldt() tries to free old_ldt instead of the newly > allocated new_ldt resulting in a memory leak. It also misses to clean up a > half populated LDT pagetable, which is not a leak as it gets cleaned up > when the process exits. > > Free both the potentially half populated LDT pagetable and the newly > allocated LDT struct. This can be done unconditionally because once a LDT > is mapped subsequent maps will succeed because the PTE page is already > populated and the two LDTs fit into that single page. > > Fixes: f55f0501cbf6 ("x86/pti: Put the LDT in its own PGD if PTI is on") > Reported-by: Mathieu Desnoyers > Signed-off-by: Thomas Gleixner > --- > arch/x86/kernel/ldt.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > --- a/arch/x86/kernel/ldt.c > +++ b/arch/x86/kernel/ldt.c > @@ -421,7 +421,13 @@ static int write_ldt(void __user *ptr, u > */ > error = map_ldt_struct(mm, new_ldt, old_ldt ? !old_ldt->slot : 0); > if (error) { > - free_ldt_struct(old_ldt); > + /* > + * This only can fail for the first LDT setup. If a LDT is > + * already installed then the PTE page is already > + * populated. Mop up a half populated page table. > + */ I liked it better with the conditional. If this ever fails due to fault injection, some silly accounting issue, a paravirt glitch, or whatever, then we'll oops. > + free_ldt_pgtables(mm); > + free_ldt_struct(new_ldt); > goto out_unlock; > } >