linux-kernel - Re: [PATCH] arm: sys_oabi-compat: Use kmalloc

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:   Fri, 26 Aug 2016 15:22:29 +0200
From:   SF Markus Elfring <elfring@...rs.sourceforge.net>
To:     Russell King <linux@...linux.org.uk>
Cc:     linux-arm-kernel@...ts.infradead.org,
        Al Viro <viro@...iv.linux.org.uk>,
        Dave Weinstein <olorin@...gle.com>,
        Jeff Layton <jeff.layton@...marydata.com>,
        Kees Cook <keescook@...omium.org>,
        Nicolas Pitre <nico@...aro.org>,
        LKML <linux-kernel@...r.kernel.org>,
        kernel-janitors@...r.kernel.org,
        Julia Lawall <julia.lawall@...6.fr>,
        Paolo Bonzini <pbonzini@...hat.com>
Subject: Re: [PATCH] arm: sys_oabi-compat: Use kmalloc_array() in two
 functions

>> Multiplications for the size determination of memory allocations
>> indicated that array data structures should be processed.
> 
> I'm afraid the above comment doesn't mean much to me, can you rephrase?

Yes, of course.

How verbose should the explanation for this update suggestion become?


> Maybe:
> 
> "Multiplications for kmalloc size arguments are liable to overflow,
> potentially causing a potential security issue.  Using kmalloc_array()
> allows the overflow to be caught and the allocation failed.  Switch
> these callsites to kmalloc_array()."

Thanks for your wording variant.


>> @@ -285,7 +285,7 @@ asmlinkage long sys_oabi_epoll_wait(int epfd,
>>  		return -EINVAL;
>>  	if (!access_ok(VERIFY_WRITE, events, sizeof(*events) * maxevents))
>>  		return -EFAULT;
>> -	kbuf = kmalloc(sizeof(*kbuf) * maxevents, GFP_KERNEL);
>> +	kbuf = kmalloc_array(maxevents, sizeof(*kbuf), GFP_KERNEL);
> 
> kmalloc_array() here actually buys us no additional safety at either
> of the callsites in your patch

Can this inline function apply a few sanity checks in a consistent way?
http://lxr.free-electrons.com/source/include/linux/slab.h#L564


> - we need to have carefully checked the values to ensure
> they don't overflow prior to the kmalloc for other reasons.

Are there any more constraints to consider?


> That's probably something that should be noted in the commit message too,
> so reviewers have the confidence that you're not blindly changing everything...

I imagine that a few contributors can get mixed feelings from a bunch
of my recent patches. There is a significant patch number in the works
for various Linux software modules.

Regards,
Markus