linux-kernel - Re: [PATCH v2 2/2] hugetlb: Provide safer dummy values for HPAGE_MASK and HPAGE

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4EC59B02.7060804@gmail.com>
Date:	Thu, 17 Nov 2011 15:38:42 -0800
From:	David Daney <ddaney.cavm@...il.com>
To:	Andrew Morton <akpm@...ux-foundation.org>
CC:	linux-mips@...ux-mips.org, ralf@...ux-mips.org,
	linux-kernel@...r.kernel.org, David Daney <david.daney@...ium.com>,
	David Rientjes <rientjes@...gle.com>
Subject: Re: [PATCH v2 2/2] hugetlb: Provide safer dummy values for HPAGE_MASK
 and HPAGE_SIZE

On 11/17/2011 03:28 PM, Andrew Morton wrote:
> On Thu, 17 Nov 2011 13:57:30 -0800
> David Daney<ddaney.cavm@...il.com>  wrote:
>
>> From: David Daney<david.daney@...ium.com>
>>
>> It was pointed out by David Rientjes that the dummy values for
>> HPAGE_MASK and HPAGE_SIZE are quite unsafe.  It they are inadvertently
>> used with !CONFIG_HUGETLB_PAGE, compilation would succeed, but the
>> resulting code would surly not do anything sensible.
>>
>> Place BUG() in the these dummy definitions, as we do in similar
>> circumstances in other places, so any abuse can be easily detected.
>>
>> Since the only sane place to use these symbols when
>> !CONFIG_HUGETLB_PAGE is on dead code paths, the BUG() cause any actual
>> code to be emitted by the compiler.
>
> I assume you meant "omitted" here.

I jumbled it up.  It should read:

... the BUG() will not cause any actual code to be emitted by the 
compiler.  In fact I have verified this on both MIPS64 and x86_64 kernels.

I could re-spin the patch with a corrected changelog if desired.

>
> But I don't think it's true.  Any such code would occur after testing
> is_vm_hugetlb_page() or similar, and would have been omitted anyway.
>

The point being that we are doing:

if (is_vm_hugetlb_page(vma)) {
	/* Do something with HPAGE_MASK*/
} else {
	/* Do something with PAGE_MASK */
}

In the !CONFIG_HUGETLB_PAGE case we have:
static inline int is_vm_hugetlb_page(struct vm_area_struct *vma)
{
	return 0;
}

The compiler sees that the usage of the dummy definitions is in a dead 
code path and nothing is emitted.

>> --- a/include/linux/hugetlb.h
>> +++ b/include/linux/hugetlb.h
>> @@ -111,8 +111,9 @@ static inline void copy_huge_page(struct page *dst, struct page *src)
>>   #define hugetlb_change_protection(vma, address, end, newprot)
>>
>>   #ifndef HPAGE_MASK
>> -#define HPAGE_MASK	PAGE_MASK		/* Keep the compiler happy */
>> -#define HPAGE_SIZE	PAGE_SIZE
>> +/* Keep the compiler happy with some dummy (but BUGgy) values */
>
> That's a quite poor comment.  This?

I was trying to communicate the presence of the BUG() in the definition. 
  Perhaps it is more confusing than it was before.

>
> --- a/include/linux/hugetlb.h~hugetlb-provide-safer-dummy-values-for-hpage_mask-and-hpage_size-fix
> +++ a/include/linux/hugetlb.h
> @@ -111,7 +111,11 @@ static inline void copy_huge_page(struct
>   #define hugetlb_change_protection(vma, address, end, newprot)
>
>   #ifndef HPAGE_MASK
> -/* Keep the compiler happy with some dummy (but BUGgy) values */
> +/*
> + * HPAGE_MASK and friends are defined if !CONFIG_HUGETLB_PAGE as an
> + * ifdef-avoiding convenience.  However they should never be evaluated at
> + * runtime if !CONFIG_HUGETLB_PAGE.
> + */
>   #define HPAGE_MASK	({BUG(); 0; })
>   #define HPAGE_SIZE	({BUG(); 0; })
>   #define HPAGE_SHIFT	({BUG(); 0; })
> _
>
>> +#define HPAGE_MASK	({BUG(); 0; })
>> +#define HPAGE_SIZE	({BUG(); 0; })
>>   #define HPAGE_SHIFT	({BUG(); 0; })
>
> This change means that HPAGE_* cannot be evaluated at compile time.  So
>
> int foo = HPAGE_SIZE;
>
> outside functions will explode.  I guess that's OK - actually desirable
> - as such code shouldn't have been compiled anyway.
>

The exact point of the patch.

Thanks,
David Daney


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/