From mboxrd@z Thu Jan 1 00:00:00 1970 From: Linus Torvalds Subject: Re: [PATCH v6 11/11] arm64: annotate user pointers casts detected by sparse Date: Thu, 6 Sep 2018 14:13:41 -0700 Message-ID: References: <5d54526e5ff2e5ad63d0dfdd9ab17cf359afa4f2.1535629099.git.andreyknvl@google.com> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Return-path: In-Reply-To: <5d54526e5ff2e5ad63d0dfdd9ab17cf359afa4f2.1535629099.git.andreyknvl@google.com> Sender: linux-kernel-owner@vger.kernel.org To: Andrey Konovalov Cc: Catalin Marinas , Will Deacon , Mark Rutland , Robin Murphy , Al Viro , Kees Cook , Kate Stewart , Greg Kroah-Hartman , Andrew Morton , Ingo Molnar , "Kirill A. Shutemov" , Shuah Khan , linux-arm-kernel , "open list:DOCUMENTATION" , linux-mm , linux-arch , "open list:KERNEL SELFTEST FRAMEWORK" , Linux List-Id: linux-arch.vger.kernel.org On Thu, Aug 30, 2018 at 4:41 AM Andrey Konovalov wrote: > > This patch adds __force annotations for __user pointers casts detected by > sparse with the -Wcast-from-as flag enabled (added in [1]). No, several of these are wrong, and just silence a warning that shows a problem. So for example: > static inline compat_uptr_t ptr_to_compat(void __user *uptr) > { > - return (u32)(unsigned long)uptr; > + return (u32)(__force unsigned long)uptr; > } this actually looks correct. But: > --- a/arch/arm64/include/asm/uaccess.h > +++ b/arch/arm64/include/asm/uaccess.h > @@ -76,7 +76,7 @@ static inline unsigned long __range_ok(const void __user *addr, unsigned long si > { > unsigned long ret, limit = current_thread_info()->addr_limit; > > - __chk_user_ptr(addr); > + __chk_user_ptr((void __force *)addr); This looks actively wrong. The whole - and only - point of "__chk_user_ptr()" is that it warns about a lack of a "__user *" type. So the above makes no sense at all. There are other similar "that makes no sense what-so-ever", like this one: > - struct compat_group_req __user *gr32 = (void *)optval; > + struct compat_group_req __user *gr32 = (__force void *)optval; no, the additionl of __force is not the right thing, the problem, is that a __user pointer is cast to a non-user 'void *' only to be assigned to another user type. The fix should have been to use (void __user *) as the cast instead, no __force needed. In general, I think the patch shows all the signs of "mindlessly just add casts", which is exactly the wrong thing to do to sparse warnings. Linus From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-it0-f67.google.com ([209.85.214.67]:52655 "EHLO mail-it0-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726308AbeIGBvL (ORCPT ); Thu, 6 Sep 2018 21:51:11 -0400 MIME-Version: 1.0 References: <5d54526e5ff2e5ad63d0dfdd9ab17cf359afa4f2.1535629099.git.andreyknvl@google.com> In-Reply-To: <5d54526e5ff2e5ad63d0dfdd9ab17cf359afa4f2.1535629099.git.andreyknvl@google.com> From: Linus Torvalds Date: Thu, 6 Sep 2018 14:13:41 -0700 Message-ID: Subject: Re: [PATCH v6 11/11] arm64: annotate user pointers casts detected by sparse Content-Type: text/plain; charset="UTF-8" Sender: linux-arch-owner@vger.kernel.org List-ID: To: Andrey Konovalov Cc: Catalin Marinas , Will Deacon , Mark Rutland , Robin Murphy , Al Viro , Kees Cook , Kate Stewart , Greg Kroah-Hartman , Andrew Morton , Ingo Molnar , "Kirill A. Shutemov" , Shuah Khan , linux-arm-kernel , "open list:DOCUMENTATION" , linux-mm , linux-arch , "open list:KERNEL SELFTEST FRAMEWORK" , Linux Kernel Mailing List , Dmitry Vyukov , Kostya Serebryany , eugenis@google.com, Lee.Smith@arm.com, Ramana Radhakrishnan , Jacob.Bramley@arm.com, Ruben.Ayrapetyan@arm.com, cpandya@codeaurora.org Message-ID: <20180906211341._4HxOCGRL57FHv8N0C9R-PX7KPXLFm3l6f6moUBjoY8@z> On Thu, Aug 30, 2018 at 4:41 AM Andrey Konovalov wrote: > > This patch adds __force annotations for __user pointers casts detected by > sparse with the -Wcast-from-as flag enabled (added in [1]). No, several of these are wrong, and just silence a warning that shows a problem. So for example: > static inline compat_uptr_t ptr_to_compat(void __user *uptr) > { > - return (u32)(unsigned long)uptr; > + return (u32)(__force unsigned long)uptr; > } this actually looks correct. But: > --- a/arch/arm64/include/asm/uaccess.h > +++ b/arch/arm64/include/asm/uaccess.h > @@ -76,7 +76,7 @@ static inline unsigned long __range_ok(const void __user *addr, unsigned long si > { > unsigned long ret, limit = current_thread_info()->addr_limit; > > - __chk_user_ptr(addr); > + __chk_user_ptr((void __force *)addr); This looks actively wrong. The whole - and only - point of "__chk_user_ptr()" is that it warns about a lack of a "__user *" type. So the above makes no sense at all. There are other similar "that makes no sense what-so-ever", like this one: > - struct compat_group_req __user *gr32 = (void *)optval; > + struct compat_group_req __user *gr32 = (__force void *)optval; no, the additionl of __force is not the right thing, the problem, is that a __user pointer is cast to a non-user 'void *' only to be assigned to another user type. The fix should have been to use (void __user *) as the cast instead, no __force needed. In general, I think the patch shows all the signs of "mindlessly just add casts", which is exactly the wrong thing to do to sparse warnings. Linus