From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751377AbeECXAw (ORCPT ); Thu, 3 May 2018 19:00:52 -0400 Received: from mail-wm0-f66.google.com ([74.125.82.66]:37372 "EHLO mail-wm0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751111AbeECXAv (ORCPT ); Thu, 3 May 2018 19:00:51 -0400 X-Google-Smtp-Source: AB8JxZqbe2PKCt++FmK3W8pSJwFLow+x5Z9mIj1wwPLz0jm0Mic2M1QZAVZFarUW523WtQL1uthVGg== Subject: Re: [PATCH 2/2] mm: Add kvmalloc_ab_c and kvzalloc_struct To: Kees Cook , Daniel Vetter Cc: Matthew Wilcox , Julia Lawall , Andrew Morton , Matthew Wilcox , Linux-MM , LKML , Kernel Hardening , cocci@systeme.lip6.fr, Himanshu Jha References: <20180308025812.GA9082@bombadil.infradead.org> <20180308230512.GD29073@bombadil.infradead.org> <20180313183220.GA21538@bombadil.infradead.org> <20180429203023.GA11891@bombadil.infradead.org> <20180430201607.GA7041@bombadil.infradead.org> <4ad99a55-9c93-5ea1-5954-3cb6e5ba7df9@rasmusvillemoes.dk> From: Rasmus Villemoes Message-ID: <4e25ff5b-f8fc-7012-83c2-b56e6928e8bc@rasmusvillemoes.dk> Date: Fri, 4 May 2018 01:00:43 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 2018-05-01 19:00, Kees Cook wrote: > On Mon, Apr 30, 2018 at 2:29 PM, Rasmus Villemoes > wrote: >> >> gcc 5.1+ (I think) have the __builtin_OP_overflow checks that should >> generate reasonable code. Too bad there's no completely generic >> check_all_ops_in_this_expression(a+b*c+d/e, or_jump_here). Though it's >> hard to define what they should be checked against - probably would >> require all subexpressions (including the variables themselves) to have >> the same type. >> >> plug: https://lkml.org/lkml/2015/7/19/358 > > That's a very nice series. Why did it never get taken? Well, nobody seemed particularly interested, and then https://lkml.org/lkml/2015/10/28/215 happened... but he did later seem to admit that it could be useful for the multiplication checking, and that "the gcc interface for multiplication overflow is fine". I still think even for unsigned types overflow checking can be subtle. E.g. u32 somevar; if (somevar + sizeof(foo) < somevar) return -EOVERFLOW; somevar += sizeof(this); is broken, because the LHS is promoted to unsigned long/size_t, then so is the RHS for the comparison, and the comparison is thus always false (on 64bit). It gets worse if the two types are more "opaque", and in any case it's not always easy to verify at a glance that the types are the same, or at least that the expression of the widest type is on the RHS. > It seems to do the right things quite correctly. Yes, I wouldn't suggest it without the test module verifying corner cases, and checking it has the same semantics whether used with old or new gcc. Would you shepherd it through if I updated the patches and resent? Rasmus From mboxrd@z Thu Jan 1 00:00:00 1970 From: linux@rasmusvillemoes.dk (Rasmus Villemoes) Date: Fri, 4 May 2018 01:00:43 +0200 Subject: [Cocci] [PATCH 2/2] mm: Add kvmalloc_ab_c and kvzalloc_struct In-Reply-To: References: <20180308025812.GA9082@bombadil.infradead.org> <20180308230512.GD29073@bombadil.infradead.org> <20180313183220.GA21538@bombadil.infradead.org> <20180429203023.GA11891@bombadil.infradead.org> <20180430201607.GA7041@bombadil.infradead.org> <4ad99a55-9c93-5ea1-5954-3cb6e5ba7df9@rasmusvillemoes.dk> Message-ID: <4e25ff5b-f8fc-7012-83c2-b56e6928e8bc@rasmusvillemoes.dk> To: cocci@systeme.lip6.fr List-Id: cocci@systeme.lip6.fr On 2018-05-01 19:00, Kees Cook wrote: > On Mon, Apr 30, 2018 at 2:29 PM, Rasmus Villemoes > wrote: >> >> gcc 5.1+ (I think) have the __builtin_OP_overflow checks that should >> generate reasonable code. Too bad there's no completely generic >> check_all_ops_in_this_expression(a+b*c+d/e, or_jump_here). Though it's >> hard to define what they should be checked against - probably would >> require all subexpressions (including the variables themselves) to have >> the same type. >> >> plug: https://lkml.org/lkml/2015/7/19/358 > > That's a very nice series. Why did it never get taken? Well, nobody seemed particularly interested, and then https://lkml.org/lkml/2015/10/28/215 happened... but he did later seem to admit that it could be useful for the multiplication checking, and that "the gcc interface for multiplication overflow is fine". I still think even for unsigned types overflow checking can be subtle. E.g. u32 somevar; if (somevar + sizeof(foo) < somevar) return -EOVERFLOW; somevar += sizeof(this); is broken, because the LHS is promoted to unsigned long/size_t, then so is the RHS for the comparison, and the comparison is thus always false (on 64bit). It gets worse if the two types are more "opaque", and in any case it's not always easy to verify at a glance that the types are the same, or@least that the expression of the widest type is on the RHS. > It seems to do the right things quite correctly. Yes, I wouldn't suggest it without the test module verifying corner cases, and checking it has the same semantics whether used with old or new gcc. Would you shepherd it through if I updated the patches and resent? Rasmus