From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751933AbcEYVAf (ORCPT ); Wed, 25 May 2016 17:00:35 -0400 Received: from mx1.redhat.com ([209.132.183.28]:34034 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751107AbcEYVAP (ORCPT ); Wed, 25 May 2016 17:00:15 -0400 Message-ID: <1464210010.15044.15.camel@redhat.com> Subject: Re: [PATCH] efi: fix for_each_efi_memory_desc_in_map() for empty memmaps From: Mark Salter To: Matt Fleming , Vitaly Kuznetsov Cc: linux-efi@vger.kernel.org, linux-kernel@vger.kernel.org, "K. Y. Srinivasan" Date: Wed, 25 May 2016 17:00:10 -0400 In-Reply-To: <20160525204805.GE2984@codeblueprint.co.uk> References: <1464187015-5640-1-git-send-email-vkuznets@redhat.com> <20160525204805.GE2984@codeblueprint.co.uk> Organization: Red Hat, Inc Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Content-Transfer-Encoding: 8bit X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.25]); Wed, 25 May 2016 21:00:14 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, 2016-05-25 at 21:48 +0100, Matt Fleming wrote: > (Cc'ing Mark, the original author) > > On Wed, 25 May, at 04:36:55PM, Vitaly Kuznetsov wrote: > > > > Commit 78ce248faa3c ("efi: Iterate over efi.memmap in > > for_each_efi_memory_desc()") introduced a regression for systems booted > > with 'noefi' kernel option. In particular, I observe early kernel hang in > > efi_find_mirror() on for_each_efi_memory_desc() call. As we don't have > > efi memmap we enter this iterator with the following parameters: > > > > efi.memmap.map = 0, efi.memmap.map_end = 0, efi.memmap.desc_size = 28 > > > > for_each_efi_memory_desc_in_map() does the following comparison: > > > > (md) <= (efi_memory_desc_t *)((m)->map_end - (m)->desc_size); > > > > where md = 0, (m)->map_end = 0 and (m)->desc_size = 28 but when we subtract > > something from a NULL pointer wrap around happens and we end up returning > > invalid pointer. > > > > Fixes: 78ce248faa3c ("efi: Iterate over efi.memmap in for_each_efi_memory_desc()") > > Signed-off-by: Vitaly Kuznetsov > > --- > >  include/linux/efi.h | 3 ++- > >  1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/include/linux/efi.h b/include/linux/efi.h > > index c2db3ca..229ccb5 100644 > > --- a/include/linux/efi.h > > +++ b/include/linux/efi.h > > @@ -1005,7 +1005,8 @@ extern int efi_memattr_apply_permissions(struct mm_struct *mm, > >  /* Iterate through an efi_memory_map */ > >  #define for_each_efi_memory_desc_in_map(m, md)    \ > >   for ((md) = (m)->map;    \ > > -      (md) <= (efi_memory_desc_t *)((m)->map_end - (m)->desc_size); \ > > +      (efi_memory_desc_t *)((md) + (m)->desc_size) <=    \ > > +      (efi_memory_desc_t *)(m)->map_end;    \ > >        (md) = (void *)(md) + (m)->desc_size) > Curse C type casting! > > You're adding m->desc_size to a (efi_memory_desc_t *) which is 40 > bytes. You need the below to avoid breaking regular EFI boot. > > But yeah, this looks like a fine fix. Mark, any concerns? No concerns. It looks like the right thing to do. > > --- > > diff --git a/include/linux/efi.h b/include/linux/efi.h > index d8ab480b1089..3a3f8d8bd9be 100644 > --- a/include/linux/efi.h > +++ b/include/linux/efi.h > @@ -1026,7 +1026,7 @@ extern int efi_memattr_apply_permissions(struct mm_struct *mm, >  /* Iterate through an efi_memory_map */ >  #define for_each_efi_memory_desc_in_map(m, md)    \ >   for ((md) = (m)->map;    \ > -      (md) <= (efi_memory_desc_t *)((m)->map_end - (m)->desc_size); \ > +      ((void *)(md) + (m)->desc_size) <= (m)->map_end;    \ >        (md) = (void *)(md) + (m)->desc_size) >   >  /**