From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756043AbcA2MFd (ORCPT ); Fri, 29 Jan 2016 07:05:33 -0500 Received: from mail.skyhub.de ([78.46.96.112]:55936 "EHLO mail.skyhub.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755129AbcA2MFa (ORCPT ); Fri, 29 Jan 2016 07:05:30 -0500 Date: Fri, 29 Jan 2016 13:05:21 +0100 From: Borislav Petkov To: Matt Fleming Cc: Ingo Molnar , Thomas Gleixner , "H . Peter Anvin" , linux-efi@vger.kernel.org, linux-kernel@vger.kernel.org, Sai Praneeth Prakhya , stable@vger.kernel.org, =?utf-8?B?VmlvcmVsLUPEg3TEg2xpbiBSxINwacibZWFudQ==?= Subject: Re: [PATCH] x86/mm/pageattr: Avoid truncation when converting cpa->numpages to address Message-ID: <20160129120521.GE10187@pd.tnic> References: <1454067370-10374-1-git-send-email-matt@codeblueprint.co.uk> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <1454067370-10374-1-git-send-email-matt@codeblueprint.co.uk> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Jan 29, 2016 at 11:36:10AM +0000, Matt Fleming wrote: > There are a couple of nasty truncation bugs lurking in the pageattr > code that can be triggered when mapping EFI regions, e.g. when we pass > a cpa->pgd pointer. Because cpa->numpages is a 32-bit value, shifting > left by PAGE_SHIFT will truncate the resultant address to 32-bits. > > Viorel-Cătălin managed to trigger this bug on his Dell machine that > provides a ~5GB EFI region which requires 1236992 pages to be mapped. They're going to need all that?! Of course they do! > When calling populate_pud() the end of the region gets calculated > incorrectly in the following buggy expression, > > end = start + (cpa->numpages << PAGE_SHIFT); > > And only 188416 pages are mapped. Next, populate_pud() gets invoked > for a second time because of the loop in __change_page_attr_set_clr(), > only this time no pages get mapped because shifting the remaining > number of pages (1048576) by PAGE_SHIFT is zero. At which point the > loop in __change_page_attr_set_clr() spins forever because we fail to > map progress. > > Hitting this bug depends very much on the virtual address we pick to > map the large region at and how many pages we map on the initial run > through the loop. This explains why this issue was only recently hit > with the introduction of commit > > a5caa209ba9c ("x86/efi: Fix boot crash by mapping EFI memmap entries bottom-up at runtime, instead of top-down") > > It's interesting to note that safe uses of cpa->numpages do exist in > the pageattr code. If instead of shifting ->numpages we multiply by > PAGE_SIZE, no truncation occurs because PAGE_SIZE is a UL value, and > so the result is unsigned long. > > To avoid surprises when users try to convert very large cpa->numpages > values to addresses, change the data type from 'int' to 'unsigned > long', thereby making it suitable for shifting by PAGE_SHIFT without > any type casting. > > The alternative would be to make liberal use of casting, but that is > far more likely to cause problems in the future when someone adds more > code and fails to cast properly; this bug was difficult enough to > track down in the first place. > > Reported-by: Viorel-Cătălin Răpițeanu > Tested-by: Viorel-Cătălin Răpițeanu > Link: https://bugzilla.kernel.org/show_bug.cgi?id=110131 > Cc: Borislav Petkov > Cc: Sai Praneeth Prakhya > Cc: > Signed-off-by: Matt Fleming Acked-by: Borislav Petkov -- Regards/Gruss, Boris. ECO tip #101: Trim your mails when you reply.