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 B5D9BC4332F for ; Wed, 19 Oct 2022 19:26:51 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 097706B0072; Wed, 19 Oct 2022 15:26:51 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 045D56B0073; Wed, 19 Oct 2022 15:26:50 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id E4F916B0074; Wed, 19 Oct 2022 15:26:50 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0012.hostedemail.com [216.40.44.12]) by kanga.kvack.org (Postfix) with ESMTP id D60A36B0072 for ; Wed, 19 Oct 2022 15:26:50 -0400 (EDT) Received: from smtpin03.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay09.hostedemail.com (Postfix) with ESMTP id A426081064 for ; Wed, 19 Oct 2022 19:26:50 +0000 (UTC) X-FDA: 80038681380.03.9F787FA Received: from mga07.intel.com (mga07.intel.com [134.134.136.100]) by imf10.hostedemail.com (Postfix) with ESMTP id AB4F7C0033 for ; Wed, 19 Oct 2022 19:26:49 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1666207610; x=1697743610; h=date:from:to:cc:subject:message-id:references: mime-version:in-reply-to; bh=WBfO4PEEZH2HCMK0CvVGY7P7dLy55zER35u6rUrGgjo=; b=KWsfGIEpU0xaeNIBMCelUrnJ7JEYZtjuwaRc0IpujdKWsD6b+a5m17eL dfkfMGlEjz/RR0jEbo+tOuzsFNnvVum6xtn1+fOsAOAUKJVfx86TiKGt6 AcBwbUE+cZvP1KOjBlO6xH81B0UL8BVyZ7MA0o06iG/1tQvo/X8Z/6Qyz N2CAg2WU0zZCl3hrQEviF6iDT+w+7zbPmWJFktKTI2bwTH7ivumqf/V3E Nfbm6uVXtNZl/YASPjEneNqFVBkusA7ZKC0VZrgTz81xuXT74DEZxlveF TSX92/zM6kX8IZ0diERO3TIdrwyJRidCf4d+SobxOmlFBgQGwQb4+T1gb Q==; X-IronPort-AV: E=McAfee;i="6500,9779,10505"; a="370725761" X-IronPort-AV: E=Sophos;i="5.95,196,1661842800"; d="scan'208";a="370725761" Received: from fmsmga005.fm.intel.com ([10.253.24.32]) by orsmga105.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 19 Oct 2022 12:26:47 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6500,9779,10505"; a="958496507" X-IronPort-AV: E=Sophos;i="5.95,196,1661842800"; d="scan'208";a="958496507" Received: from smile.fi.intel.com ([10.237.72.54]) by fmsmga005.fm.intel.com with ESMTP; 19 Oct 2022 12:26:45 -0700 Received: from andy by smile.fi.intel.com with local (Exim 4.96) (envelope-from ) id 1olEiB-00A4m0-1a; Wed, 19 Oct 2022 22:26:43 +0300 Date: Wed, 19 Oct 2022 22:26:43 +0300 From: Andy Shevchenko To: Jane Chu Cc: Petr Mladek , "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: Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1666207610; 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=xK9z2f31WHWyRGjYtDnbp+FHG9sQ9BGo7ecEdvLMIU8=; b=mQy09Eh6fIZ0R/3fCPGITUHpBCIiDKbFL39OCkkHX3OEtsFB2QIMtzf0sbrs14Kp3NeR5d QEzwIb/tPkNlG3t3ECl0R/eWT8UC9sJBaeWEQsIq9jcrtaaZzq4u0m3yUtg4b6vtSlNzDY GcYaajAM5LZNsJuSVTzoD1y7rXBcKpA= ARC-Authentication-Results: i=1; imf10.hostedemail.com; dkim=none ("invalid DKIM record") header.d=intel.com header.s=Intel header.b=KWsfGIEp; spf=none (imf10.hostedemail.com: domain of andriy.shevchenko@linux.intel.com has no SPF policy when checking 134.134.136.100) smtp.mailfrom=andriy.shevchenko@linux.intel.com; dmarc=fail reason="No valid SPF" header.from=intel.com (policy=none) ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1666207610; a=rsa-sha256; cv=none; b=k81ZIJjShmrhM1PREFQ/mz0dYrRQubOvOLPG9Maf1ly5oiUSc+d7nR+Mcu0rQPboJevAq5 Iz8dXSmYO6zSUSv1Eg0e/0iTM3Y9GumtzzBN6VwunRmicIHFuEdYKwdeM70HveimTAX36w cZC5gF06iBsvH5j8KVWy9v778jQchR0= Authentication-Results: imf10.hostedemail.com; dkim=none ("invalid DKIM record") header.d=intel.com header.s=Intel header.b=KWsfGIEp; spf=none (imf10.hostedemail.com: domain of andriy.shevchenko@linux.intel.com has no SPF policy when checking 134.134.136.100) smtp.mailfrom=andriy.shevchenko@linux.intel.com; dmarc=fail reason="No valid SPF" header.from=intel.com (policy=none) X-Rspamd-Server: rspam07 X-Rspamd-Queue-Id: AB4F7C0033 X-Stat-Signature: sc4gdfb4cr1uyp9kriziry6ewkc85qbj X-Rspam-User: X-HE-Tag: 1666207609-599671 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 Wed, Oct 19, 2022 at 06:36:07PM +0000, Jane Chu wrote: > On 10/18/2022 1:49 PM, 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. > >>>> > >>>>> > >>>>>> the lingering issue > >>>>>> is that the kernel ought to be able to protect itself, as the pointer > >>>>>> contains a non-canonical address. > >>>>> > >>>>> Was the pointer used only by the vsprintf()? > >>>>> Or was it accessed also by another code, please? > >>>> > >>>> The OOB pointer was used only by vsprintf() for the "cat" sysfs case. > >>>> No other code uses the OOB pointer, verified both by code examination > >>>> and test. > >>> > >>> So, then the vsprintf() is _the_ point to crash and why should we hide that? > >>> Because of the crash you found the culprit, right? The efault will hide very > >>> important details. > >>> > >>> So to me it sounds like I like this change less and less... > >> > >> What about the existing check > >> if ((unsigned long)ptr < PAGE_SIZE || IS_ERR_VALUE(ptr)) > >> return "(efault)"; > >> ? > > > > Because it's _special_. We know that First page is equivalent to a NULL pointer > > and the last one is dedicated for so called error pointers. There are no more > > special exceptions to the addresses in the Linux kernel (I don't talk about > > alignment requirements by the certain architectures). > > > >> In an experiment just to print the raw OOB pointer values, I saw below > >> (the devX attrY stuff are substitutes of the real attributes, other > >> values and strings are verbatim copy from "dmesg"): > >> > >> [ 3002.772329] devX_attrY[26]: (ffffffff84d60ad3) Dev X AttributeY E > >> [ 3002.772346] devX_attrY[27]: (ffffffff84d60ae4) Dev X AttributeY F > >> [ 3002.772347] devX_attrY[28]: (ffffffff84d60aee) Dev X AttributeY G > >> [ 3002.772349] devX_attrY[29]: (0) (null) > >> [ 3002.772350] devX_attrY[30]: (0) (null) > >> [ 3002.772351] devX_attrY[31]: (0) (null) > >> [ 3002.772352] devX_attrY[32]: (7665645f63616465) (einval) > >> [ 3002.772354] devX_attrY[33]: (646e61685f656369) (einval) > >> [ 3002.772355] devX_attrY[34]: (6f635f65755f656c) (einval) > >> [ 3002.772355] devX_attrY[35]: (746e75) (einval) > >> > >> where starting from index 29 are all OOB pointers. > >> > >> 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? > > > > If the purpose is to see the bug as early as possible, then getting > "(efault)" from reading sysfs attribute would serve the purpose, right? > > The fact an OOB pointer has already being turned into either NULL or > non-canonical value implies that *if* kernel code other than > vsprintf() references the pointer, it'll crash else where; No, not the case for error pointers and NULL. > but *if* no > other code referencing the pointer, why crash? Because how else you can see the bug?! The trace will give you essential information about registers, etc that gives you a hint what the _cause_ of the crash. And we need that cause. The "(efault)" has not even a bit close to what crash gives us. So, this is my last message in the discussion. Here is a formal NAK. Up to maintainers to decide what to do with this. -- With Best Regards, Andy Shevchenko