Re: [PATCH] vmcore: prevent PT_NOTE p_memsz overflow during header update

["Pearson, Greg" <greg.pearson@hp.com>]

On 01/31/2014 04:16 PM, Andrew Morton wrote:
> On Fri, 31 Jan 2014 16:06:06 -0700 Greg Pearson <greg.pearson@hp.com> wrote:
>
>> Currently, update_note_header_size_elf64() and
>> update_note_header_size_elf32() will add the size
>> of a PT_NOTE entry to real_sz even if that causes real_sz
>> to exceeds max_sz. This patch corrects the while loop logic
>> in those routines to ensure that does not happen.
>>
>> ...
>>
>> Occasionally, a second entry is encountered with very
>> large n_namesz and n_descsz sizes:
>>
>>    n_namesz = 0x80000008
>>    n_descsz = 0x510ae163
>>    n_type   = 0x80000008
> Hang on.
>
>> --- a/fs/proc/vmcore.c
>> +++ b/fs/proc/vmcore.c
>> @@ -468,12 +468,13 @@ static int __init update_note_header_size_elf64(const Elf64_Ehdr *ehdr_ptr)
>>   			return rc;
>>   		}
>>   		nhdr_ptr = notes_section;
>> -		while (real_sz < max_sz) {
>> -			if (nhdr_ptr->n_namesz == 0)
>> -				break;
>> +		while (nhdr_ptr->n_namesz != 0) {
>>   			sz = sizeof(Elf64_Nhdr) +
>>   				((nhdr_ptr->n_namesz + 3) & ~3) +
>>   				((nhdr_ptr->n_descsz + 3) & ~3);
>> +			/* Silently drop further PT_NOTE entries */
>> +			if ((real_sz + sz) > max_sz)
>> +				break;
> If we are encountering notes with these crazy sizes then what is
> preventing (real_sx + sz) from wrapping through zero, which would
> defeat this check?
>
>

As far as I know the only consequence of dropping a PT_NOTE entry is 
that it would not be available in the crash dump for use in debugging. 
I'm not sure how important this data might be for triage. I'm guessing 
that in cases where one of these strange PT_NOTE entries shows up with a 
size that causes an overflow it probably isn't even a real PT_NOTE entry 
so dropping it won't matter, but that's a guess at this point since I'm 
still trying to figure out how the bogus entries were created.

I think the wrap case is handled ok by the current code since "real_sz" 
and "sz" are both declared as u64, while the "n_namesz" and "n_descsz" 
fields are declared as u32. This is true for both the elf32 and elf64 case.

Adding a pr_warn() is probably a good idea, then I can remove the comment.

--
Greg