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 DD912C4332F for ; Tue, 18 Oct 2022 20:49:34 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 6B0EA6B0072; Tue, 18 Oct 2022 16:49:34 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 63A806B0075; Tue, 18 Oct 2022 16:49:34 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 5023D6B0078; Tue, 18 Oct 2022 16:49:34 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0011.hostedemail.com [216.40.44.11]) by kanga.kvack.org (Postfix) with ESMTP id 3A9A46B0072 for ; Tue, 18 Oct 2022 16:49:34 -0400 (EDT) Received: from smtpin18.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay03.hostedemail.com (Postfix) with ESMTP id 17A27A1102 for ; Tue, 18 Oct 2022 20:49:34 +0000 (UTC) X-FDA: 80035261068.18.275573F Received: from mga17.intel.com (mga17.intel.com [192.55.52.151]) by imf15.hostedemail.com (Postfix) with ESMTP id 5CA48A0038 for ; Tue, 18 Oct 2022 20:49:33 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1666126173; x=1697662173; h=date:from:to:cc:subject:message-id:references: mime-version:in-reply-to; bh=PhtP4L4IziWH+iVHX/zbpE3/ubmrbUo2PAVr5BTmktY=; b=GD/O4HC4R6k6bPugUeqQ6EJAZdWt0K1uuEeOeMmAE90ahi6vLfbyCrLk OBRA1CWLBwKs1sKR9zIhCBxHmOjN11o+MWaHKiBMHj23mLf4lxdTiascW /wBE5MSliDJ66+KmSI9BDZY5SI4XHHmvqlHpKF+7EA5YjWx+qte3uV6l4 TD0n6dgznB5umZOVXjwAULlpD74JeCi50G/e1dEubCHG2p9DHwGIGwPEW Ode8qxreAG4hIYQQdsIJQS7X52fckXbBKhR5cHBooMJuqUFVNjVsw6HPH 9qHrfnqTkx7Lt1RP2VWS2kQcGWJta06Tkw3HJezJEcqf9Vtge9j7SE68x w==; X-IronPort-AV: E=McAfee;i="6500,9779,10504"; a="286622933" X-IronPort-AV: E=Sophos;i="5.95,194,1661842800"; d="scan'208";a="286622933" Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by fmsmga107.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 18 Oct 2022 13:49:32 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6500,9779,10504"; a="771412657" X-IronPort-AV: E=Sophos;i="5.95,194,1661842800"; d="scan'208";a="771412657" Received: from smile.fi.intel.com ([10.237.72.54]) by fmsmga001.fm.intel.com with ESMTP; 18 Oct 2022 13:49:29 -0700 Received: from andy by smile.fi.intel.com with local (Exim 4.96) (envelope-from ) id 1oktWi-009YCi-03; Tue, 18 Oct 2022 23:49:28 +0300 Date: Tue, 18 Oct 2022 23:49:27 +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-Authentication-Results: i=1; imf15.hostedemail.com; dkim=none ("invalid DKIM record") header.d=intel.com header.s=Intel header.b="GD/O4HC4"; spf=none (imf15.hostedemail.com: domain of andriy.shevchenko@linux.intel.com has no SPF policy when checking 192.55.52.151) 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=1666126173; a=rsa-sha256; cv=none; b=NpP746mVCPBUWbvo/+yktAzQozHRexoFahhGpLBsnanC0OY4srXJrtwHXtLsBo5O6Hj2pQ 5o9Qzg6BSOvkm0gEfSI/W/bMN7ZCDOr1n2eYI7dx5Wm81m7utFsTAmj8+oX0akIdakhCrQ ArMsFKFdLzpmsisin/UJqGztx+pt7BY= ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1666126173; 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=6ZOyEsdpV3Y55+HF2JTJAD3Q7/jov8xSPNnGlBAVCNQ=; b=vxwyyCAqT1WLcb2BG+dQOufe9+PfZuh/1b77mqe7HXz9csQvF1HeCFk7k9O+inbJaCrGv6 aOPdvOrPobaHb/LtYXHp3BE0Ru5aIVbXaRErBCeTVMmFbW2x2e3NK0ymisoJjlnQvI0y8J tx4gZQowRz9svNUckXARle+SnzpSOIE= X-Stat-Signature: cueuh87ois9hi4a5bp1xywmr5n6kuf9q X-Rspamd-Queue-Id: 5CA48A0038 Authentication-Results: imf15.hostedemail.com; dkim=none ("invalid DKIM record") header.d=intel.com header.s=Intel header.b="GD/O4HC4"; spf=none (imf15.hostedemail.com: domain of andriy.shevchenko@linux.intel.com has no SPF policy when checking 192.55.52.151) smtp.mailfrom=andriy.shevchenko@linux.intel.com; dmarc=fail reason="No valid SPF" header.from=intel.com (policy=none) X-Rspam-User: X-Rspamd-Server: rspam01 X-HE-Tag: 1666126173-196043 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 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? -- With Best Regards, Andy Shevchenko