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 Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id C4B7CC4332F for ; Thu, 20 Oct 2022 09:18:15 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 0BC668E0001; Thu, 20 Oct 2022 05:18:15 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 06C516B0075; Thu, 20 Oct 2022 05:18:15 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id EC4148E0001; Thu, 20 Oct 2022 05:18:14 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0013.hostedemail.com [216.40.44.13]) by kanga.kvack.org (Postfix) with ESMTP id DEE486B0074 for ; Thu, 20 Oct 2022 05:18:14 -0400 (EDT) Received: from smtpin21.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay05.hostedemail.com (Postfix) with ESMTP id B3A0040C0B for ; Thu, 20 Oct 2022 09:18:14 +0000 (UTC) X-FDA: 80040776508.21.1618AFA Received: from smtp-out2.suse.de (smtp-out2.suse.de [195.135.220.29]) by imf19.hostedemail.com (Postfix) with ESMTP id 21F711A0017 for ; Thu, 20 Oct 2022 09:18:13 +0000 (UTC) Received: from relay2.suse.de (relay2.suse.de [149.44.160.134]) by smtp-out2.suse.de (Postfix) with ESMTP id 97E021F8D9; Thu, 20 Oct 2022 09:18:12 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.com; s=susede1; t=1666257492; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=pZ6dBBuhyWpe56chi8ICnKg8FjxgGg4afvNstG19M38=; b=g+RGhloAAWTO+R+MGTGw4IzHeWaDAPlrDYlekKJUnkGp66TD2VzGVr1qTYLuDvnA9mucmr KinYwPPb1xVQBwZ5ySHPvy/ywxc16n6DAF1hFFkVhjcnwoW2hXTowdTPQOKBg81IoEnkyG W+Oar9dMytX9Mmt2aSNfFNsWbxmXrjs= Received: from suse.cz (unknown [10.100.208.146]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by relay2.suse.de (Postfix) with ESMTPS id 26EAC2C180; Thu, 20 Oct 2022 09:18:11 +0000 (UTC) Date: Thu, 20 Oct 2022 11:18:11 +0200 From: Petr Mladek To: Andy Shevchenko Cc: Jane Chu , "rostedt@goodmis.org" , "senozhatsky@chromium.org" , "linux@rasmusvillemoes.dk" , "linux-kernel@vger.kernel.org" , "linux-mm@kvack.org" , Haakon Bugge , John Haxby Subject: Re: [PATCH] vsprintf: protect kernel from panic due to non-canonical pointer dereference Message-ID: References: <20221017191611.2577466-1-jane.chu@oracle.com> <5d987403-a7bf-8996-d639-c99edeaabcdf@oracle.com> <799e5390-2ff5-02b7-2df7-61198d5451e2@oracle.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1666257494; a=rsa-sha256; cv=none; b=Ta3WxytTMkZuYBSRgwToUiP2bCPq48jN9wSlkZ+pX8AFrDU0czTmaEAlackx2SPxqOWXIJ qSVP39djMPkAw227uxRUp6czF0HmGzwhXByWxgzfRvsGLHqRmO+8shHBsfLz3SPgKcvU4d o4TS4T6nO2j6rYNAlqXoVrALCB/1TEw= ARC-Authentication-Results: i=1; imf19.hostedemail.com; dkim=pass header.d=suse.com header.s=susede1 header.b=g+RGhloA; spf=pass (imf19.hostedemail.com: domain of pmladek@suse.com designates 195.135.220.29 as permitted sender) smtp.mailfrom=pmladek@suse.com; dmarc=pass (policy=quarantine) header.from=suse.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1666257494; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=pZ6dBBuhyWpe56chi8ICnKg8FjxgGg4afvNstG19M38=; b=LpubFItu5MUJaZLeMtklh2s4YBMgrbsnnfUbw/6mbyBJ7nSTfyz+Z4bO2d6YiZzLLBA6YA 1MdoyP7/FrXLhU+c1FA61g8dM9jn8lIBSrfVHUKq+h9e0rg3vXThdVJQNkwOivuK1k6+fd Fg2ANsDR0HJwQ5o/1dGUQRaKspsTpsY= X-Rspam-User: Authentication-Results: imf19.hostedemail.com; dkim=pass header.d=suse.com header.s=susede1 header.b=g+RGhloA; spf=pass (imf19.hostedemail.com: domain of pmladek@suse.com designates 195.135.220.29 as permitted sender) smtp.mailfrom=pmladek@suse.com; dmarc=pass (policy=quarantine) header.from=suse.com X-Stat-Signature: h6bbekymcjw1dn5mbsdss7ghybh4cef5 X-Rspamd-Queue-Id: 21F711A0017 X-Rspamd-Server: rspam10 X-HE-Tag: 1666257493-735175 X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Thu 2022-10-20 09:44:06, Petr Mladek wrote: > On Tue 2022-10-18 23:49:27, Andy Shevchenko wrote: > > On Tue, Oct 18, 2022 at 08:30:01PM +0000, Jane Chu wrote: > > > On 10/18/2022 1:07 PM, Andy Shevchenko wrote: > > > > On Tue, Oct 18, 2022 at 06:56:31PM +0000, Jane Chu wrote: > > > >> On 10/18/2022 5:45 AM, Petr Mladek wrote: > > > >>> On Mon 2022-10-17 19:31:53, Jane Chu wrote: > > > >>>> On 10/17/2022 12:25 PM, Andy Shevchenko wrote: > > > >>>>> On Mon, Oct 17, 2022 at 01:16:11PM -0600, Jane Chu wrote: > > > >>>>>> While debugging a separate issue, it was found that an invalid string > > > >>>>>> pointer could very well contain a non-canical address, such as > > > >>>>>> 0x7665645f63616465. In that case, this line of defense isn't enough > > > >>>>>> to protect the kernel from crashing due to general protection fault > > > >>>>>> > > > >>>>>> if ((unsigned long)ptr < PAGE_SIZE || IS_ERR_VALUE(ptr)) > > > >>>>>> return "(efault)"; > > > >>>>>> > > > >>>>>> So instead, use kern_addr_valid() to validate the string pointer. > > > >>>>> > > > >>>>> How did you check that value of the (invalid string) pointer? > > > >>>>> > > > >>>> > > > >>>> In the bug scenario, the invalid string pointer was an out-of-bound > > > >>>> string pointer. While the OOB referencing is fixed, > > > >>> > > > >>> Could you please provide more details about the fixed OOB? > > > >>> What exact vsprintf()/printk() call was broken and eventually > > > >>> how it was fixed, please? > > > >> > > > >> For sensitive reason, I'd like to avoid mentioning the specific name of > > > >> the sysfs attribute in the bug, instead, just call it "devX_attrY[]", > > > >> and describe the precise nature of the issue. > > > >> > > > >> devX_attrY[] is a string array, declared and filled at compile time, > > > >> like > > > >> const char const devX_attrY[] = { > > > >> [ATTRY_A] = "Dev X AttributeY A", > > > >> [ATTRY_B] = "Dev X AttributeY B", > > > >> ... > > > >> [ATTRY_G] = "Dev X AttributeY G", > > > >> } > > > >> such that, when user "cat /sys/devices/systems/.../attry_1", > > > >> "Dev X AttributeY B" will show up in the terminal. > > > >> That's it, no more reference to the pointer devX_attrY[ATTRY_B] after that. > > > >> > > > >> The bug was that the index to the array was wrongfully produced, > > > >> leading up to OOB, e.g. devX_attrY[11]. The fix was to fix the > > > >> calculation and that is not an upstream fix. > > I see. printk()/vsprintf() is the only code that accesses this pointer. > If vsprintf() survives than the system survives. > > > > As you can see, if the OOBs are NULL, "(null)" was printed due to the > > > existing checking, but when the OOBs are turned to non-canonical which > > > is detectable, the fact the pointer value deviates from > > > (ffffffff84d60aee + 4 * sizeof(void *)) > > > evidently shown that the OOBs are detectable. > > > > > > The question then is why should the non-canonical OOBs be treated > > > differently from NULL and ERR_VALUE? > > > > Obviously, to see the crash. And let kernel _to crash_. Isn't it what we need > > to see a bug as early as possible? > > I do not agree here. Kernel tries to survive many situations when > thighs does not work as expected. It prints a warning so that > users/developers are aware of the problem and could fix it. > > In our case, the crash happened when reading a sysfs file. > IMHO, it is much better to show (-EINVAL) than crash. The bug > when accessing devX_attrY[] does not affect the stability of > the system at all. > > And the broken string might be passed in a very rare case, > e.g. in an error path. So that it might be hard to catch > when testing. That said, there is definitely a difference between NULL or error code code and a random pointer address. The pointers in ERR_RANGE are likely to stay in this range. It means that this pointer is hardly usable as a security attack. On the other hand, "random" pointer has a bigger chance to be used for a security attack. From this POV, it is more important to catch and fix random pointer issues. And shoving just -EINVAL might not be enough to catch attention. I guess that this was what Andy wanted to explain. And kernel crash would definitely catch attention. Showing some warning with KERN_WARNING or even WARN() might be an alternative. Anyway, I think that this patch is not worth it: + kern_addr_valid() always succeeds on all architectures except on x86_64. It means that the check would help only on x86_64. + kern_addr_valid() always fails on x86 when build with SPARSEMEM. This is not acceptable for vsprintf(). + the situation when only vsprintf() would access the wrong pointer are rare. In most cases, the pointer is later used and the kernel crashes anyway. Best Regards, Petr