linux-kernel - Re: [PATCH] sysctl: Fix division by zero in percpu_pagelist

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]

Date:	Mon, 2 Jun 2014 22:12:57 -0400
From:	Oleg Drokin <green@...uxhacker.ru>
To:	David Rientjes <rientjes@...gle.com>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	linux-kernel@...r.kernel.org, devel@...verdev.osuosl.org,
	Rohit Seth <rohitseth@...gle.com>
Subject: Re: [PATCH] sysctl: Fix division by zero in percpu_pagelist_fraction handler

Hello!

On Jun 2, 2014, at 9:40 PM, David Rientjes wrote:
>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>> index 5dba293..91d0265 100644
>> --- a/mm/page_alloc.c
>> +++ b/mm/page_alloc.c
>> @@ -5854,7 +5854,7 @@ int percpu_pagelist_fraction_sysctl_handler(ctl_table *table, int write,
>> 	int ret;
>> 
>> 	ret = proc_dointvec_minmax(table, write, buffer, length, ppos);
>> -	if (!write || (ret < 0))
>> +	if (!write || (ret < 0) || !*length)
>> 		return ret;
>> 
>> 	mutex_lock(&pcp_batch_high_lock);
> This hasn't made it to linux-next yet (probably because you didn't cc 
> Andrew Morton, the mm maintainer), but I'm wondering why it's needed.  
> Shouldn't this value always be >= min_percpu_pagelist_fract?
> 
> If there's something going on in proc_dointvec_minmax() that disregards 
> that minimum then we need to fix it rather than the caller.

It's needed because at the very beginning of proc_dointvec_minmax it checks if the length is 0 and if it is, immediately returns success because there's nothing to be done
from it's perspective.
And since percpu_pagelist_fraction is 0 from the very beginning, next step is division by this value.

If length is not 0 on the other hand, then there's some value proc_dointvec_minmax can interpret and the min and max checks happen and everything works correctly.

This is kind of hard to fix in proc_dointvec_minmax because there's no passed in value to check against, I think. Sure, you can also check the passed in pointer to value
to make sure it's within range, but that goes beyond the scope of the function.
You might as well just assign a sane value to percpu_pagelist_fraction, which has an added benefit of when you cat that /proc file, you'll get real value of what the kernel uses instead of 0.

Bye,
    Oleg--
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/