From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-0.9 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 48503C33C9E for ; Tue, 14 Jan 2020 21:22:30 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 1C1A624656 for ; Tue, 14 Jan 2020 21:22:30 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1579036950; bh=2NV40hDC/nYVy8Guf3WBpLHbOgoEkir++JBqumbWgVU=; h=References:In-Reply-To:From:Date:Subject:To:Cc:List-ID:From; b=W3ZLPno5+frpHBRP8FdOqk32hWMvVYlqVlrTXl/+r2mCwIIorgwnjHVXbXt8u47p8 10i73TKmpRchNFORRYHadLRbj57C00WhCDR0sahGt5Om436tjLmNcTaY35CbjtPT7N 4b+gA26p6vmlzTLondxR2S0NPhEARLu2thPDaRzE= Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728900AbgANVW3 (ORCPT ); Tue, 14 Jan 2020 16:22:29 -0500 Received: from mail-lj1-f196.google.com ([209.85.208.196]:34391 "EHLO mail-lj1-f196.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1728731AbgANVW2 (ORCPT ); Tue, 14 Jan 2020 16:22:28 -0500 Received: by mail-lj1-f196.google.com with SMTP id z22so16089344ljg.1 for ; Tue, 14 Jan 2020 13:22:27 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux-foundation.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=sA09QfXfHalAafAysRZFEbl/WyLTN07MU6TFhyXENpY=; b=ZuB9kWPAPxoZGiuuomw2iudECT1rt4Q/HncS4ElvHzHC6yUT2oQ0HUnEnUGkTmGfs5 jSF+DX0BEvuJ4HATwr4M5/u+JM7dyP0AtYM8/eZRZWfxVC069BAzSr0Gdwlw9jjgh6al CCB0j1K9REL8bGh8CtUBoa4q17ax6w7GJvMoo= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=sA09QfXfHalAafAysRZFEbl/WyLTN07MU6TFhyXENpY=; b=eDyHQgSclguGfaOiR4Bp20NzrafVENF9sIdhc/dZwo6JcRjPRozdB3Z215QmoaIqiP LCmYG5/XoOAbaZWmON9F3FOMMGZHiO/LjEHKWeGhnui1bmyGHdQnWk3iU+K3M21APAgi eUuyeZFwra9nNSyykMGJi5CcY3AW569fWyscssYj979Hn4pmJEd5ECsPX3Yh+dTMha2+ /l74EWyhR8UzrCOLqADqEM8RpLzFXjG4w4GOqnyDvisb7Eq6qhQdB2+ZP0nHGwBKQyaI XnWOOAr76QAIjfFwm/kHIPorH3l7cA89SExAzNgr3nY13w1+c9uqR3CDat9/sldwXq8V RmRw== X-Gm-Message-State: APjAAAV1SKFL3U/70m2d2vcUdo0neSMmvvITKkUkihQbT9dSJsCC0VeL D/iZWamRl/AucaGvae/6yPI5LZlSj2E= X-Google-Smtp-Source: APXvYqyTFIbyAuX1d+60dXZRYRZJhbdb6pjGY7TypI0cIhoQOV2Ua1KwwW0r3VfOi/F6F2StSk1UcQ== X-Received: by 2002:a05:651c:1214:: with SMTP id i20mr15985224lja.107.1579036945706; Tue, 14 Jan 2020 13:22:25 -0800 (PST) Received: from mail-lj1-f180.google.com (mail-lj1-f180.google.com. [209.85.208.180]) by smtp.gmail.com with ESMTPSA id l5sm8199136lje.1.2020.01.14.13.22.24 for (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 14 Jan 2020 13:22:24 -0800 (PST) Received: by mail-lj1-f180.google.com with SMTP id h23so16053121ljc.8 for ; Tue, 14 Jan 2020 13:22:24 -0800 (PST) X-Received: by 2002:a2e:800b:: with SMTP id j11mr15632652ljg.126.1579036943823; Tue, 14 Jan 2020 13:22:23 -0800 (PST) MIME-Version: 1.0 References: <20200114200846.29434-1-vgupta@synopsys.com> <20200114200846.29434-3-vgupta@synopsys.com> In-Reply-To: <20200114200846.29434-3-vgupta@synopsys.com> From: Linus Torvalds Date: Tue, 14 Jan 2020 13:22:07 -0800 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [RFC 2/4] lib/strncpy_from_user: Remove redundant user space pointer range check To: Vineet Gupta Cc: Arnd Bergmann , Khalid Aziz , Andrey Konovalov , Andrew Morton , Peter Zijlstra , Christian Brauner , Kees Cook , Ingo Molnar , Aleksa Sarai , linux-snps-arc@lists.infradead.org, Linux Kernel Mailing List , linux-arch Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Jan 14, 2020 at 12:09 PM Vineet Gupta wrote: > > This came up when switching ARC to word-at-a-time interface and using > generic/optimized strncpy_from_user > > It seems the existing code checks for user buffer/string range multiple > times and one of tem cn be avoided. NO! DO NOT DO THIS. This is seriously buggy. > long strncpy_from_user(char *dst, const char __user *src, long count) > { > - unsigned long max_addr, src_addr; > - > if (unlikely(count <= 0)) > return 0; > > - max_addr = user_addr_max(); > - src_addr = (unsigned long)untagged_addr(src); > - if (likely(src_addr < max_addr)) { > - unsigned long max = max_addr - src_addr; > + kasan_check_write(dst, count); > + check_object_size(dst, count, false); > + if (user_access_begin(src, count)) { You can't do that "user_access_begin(src, count)", because "count" is the maximum _possible_ length, but it is *NOT* necessarily the actual length of the string we really get from user space! Think of this situation: - user has a 5-byte string at the end of the address space - kernel does a n = strncpy_from_user(uaddr, page, PAGE_SIZE) now your "user_access_begin(src, count)" will _fail_, because "uaddr" is close to the end of the user address space, and there's not room for PAGE_SIZE bytes any more. But "count" isn't actually how many bytes we will access from user space, it's only the maximum limit on the *target*. IOW, it's about a kernel buffer size, not about the user access size. Because we'll only access that 5-byte string, which fits just fine in the user space, and doing that "user_access_begin(src, count)" gives the wrong answer. The fact is, copying a string from user space is *very* different from copying a fixed number of bytes, and that whole dance with max_addr = user_addr_max(); is absolutely required and necessary. You completely broke string copying. It is very possible that string copying was horribly broken on ARC before too - almost nobody ever gets this right, but the generic routine does. So the generic routine is not only faster, it is *correct*, and your change broke it. Don't touch generic code. If you want to use the generic code, please do so. But DO NOT TOUCH IT. It is correct, your patch is wrong. The exact same issue is true in strnlen_user(). Don't break it. Linus