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 AAE1CC433FE for ; Tue, 18 Oct 2022 19:29:03 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 31CC56B0075; Tue, 18 Oct 2022 15:29:03 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 2CCD46B0078; Tue, 18 Oct 2022 15:29:03 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 1BB658E0001; Tue, 18 Oct 2022 15:29:03 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0010.hostedemail.com [216.40.44.10]) by kanga.kvack.org (Postfix) with ESMTP id 0D64B6B0075 for ; Tue, 18 Oct 2022 15:29:03 -0400 (EDT) Received: from smtpin20.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay10.hostedemail.com (Postfix) with ESMTP id CC824C0936 for ; Tue, 18 Oct 2022 19:29:02 +0000 (UTC) X-FDA: 80035058124.20.8E8A429 Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) by imf19.hostedemail.com (Postfix) with ESMTP id 07C5D1A0005 for ; Tue, 18 Oct 2022 19:28:59 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=infradead.org; s=bombadil.20210309; h=Content-Transfer-Encoding: Content-Type:In-Reply-To:From:References:Cc:To:Subject:MIME-Version:Date: Message-ID:Sender:Reply-To:Content-ID:Content-Description; bh=lcM7PDU+W78CbEKHN9YfRJV5gYHnLEK9SFOQzcXXDYA=; b=jgE+5SYp10uf2d7LZp4fW/9e1n H0y8oFzq2013vvIW8n9xOwKbx2P1fmnQP65mPNUGrL9rppmd+3Wp2NOS1erj1v20zjVY4ZL8YFDdr VkcknO+/MuQwKQssMGpERUh6HycM3mtWZ15fG/EMWrebY1/jqKe5KhlaKY2ZEVe8qXSe88wxjWWau /smUoWOlAsmSqBYMvDWAVrF06ISrFJbvnkKat6re3j3M72V03gG3/ICwNCnnM3aHtZdYBwsPK1BpZ VmHWZImzI1IU6fCJsNSmx8V3z0QKKtRRTVZxIhILwZcoZo96wq0EqqDawf/VlgPz70yVk6dTjhLud Gfr+e5eA==; Received: from [2601:1c2:d80:3110::a2e7] by bombadil.infradead.org with esmtpsa (Exim 4.94.2 #2 (Red Hat Linux)) id 1oksGd-00AExV-8e; Tue, 18 Oct 2022 19:28:50 +0000 Message-ID: <46b4bf12-f501-0447-8c3c-8dd0cb91ba92@infradead.org> Date: Tue, 18 Oct 2022 12:28:46 -0700 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.3.1 Subject: Re: [PATCH] vsprintf: protect kernel from panic due to non-canonical pointer dereference Content-Language: en-US To: Jane Chu , Petr Mladek Cc: Andy Shevchenko , "rostedt@goodmis.org" , "senozhatsky@chromium.org" , "linux@rasmusvillemoes.dk" , "linux-kernel@vger.kernel.org" , "linux-mm@kvack.org" References: <20221017191611.2577466-1-jane.chu@oracle.com> <5d987403-a7bf-8996-d639-c99edeaabcdf@oracle.com> <799e5390-2ff5-02b7-2df7-61198d5451e2@oracle.com> From: Randy Dunlap In-Reply-To: <799e5390-2ff5-02b7-2df7-61198d5451e2@oracle.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1666121342; 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:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=lcM7PDU+W78CbEKHN9YfRJV5gYHnLEK9SFOQzcXXDYA=; b=SP34omSJViFCbfN1ik1aQGsHNOqmvb+GZ7eVwNfmgf3Lm+9a3o2rRT8Ghr6j6OguDUTnnF LJqCgnv4y1towuwkZsjhqrpTOI/bY+EduRCVTduAuoM+RjEfSrr1D1S9bE1w2oW62cvzpX 5FV2k0pWcXxB9ntNl7TbT1UZ1j4N2Wc= ARC-Authentication-Results: i=1; imf19.hostedemail.com; dkim=pass header.d=infradead.org header.s=bombadil.20210309 header.b=jgE+5SYp; dmarc=none; spf=none (imf19.hostedemail.com: domain of rdunlap@infradead.org has no SPF policy when checking 198.137.202.133) smtp.mailfrom=rdunlap@infradead.org ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1666121342; a=rsa-sha256; cv=none; b=Tom7DNo8G6d41CaiYzHZEE8bjX/duxTgoDyQUEVJUHkKbio72nHI4AiitjnlQj2Gb0S3Mr Ppof0j+xuehN1sTOWoHjFqDTIlHaAQ6XuaAsYv1EGBcs8QT9esMQXtq3xBh8CwPIRm8kQZ ywlzWtjTPZbkbM+ch1Kpky/aQzzcNyo= Authentication-Results: imf19.hostedemail.com; dkim=pass header.d=infradead.org header.s=bombadil.20210309 header.b=jgE+5SYp; dmarc=none; spf=none (imf19.hostedemail.com: domain of rdunlap@infradead.org has no SPF policy when checking 198.137.202.133) smtp.mailfrom=rdunlap@infradead.org X-Rspamd-Server: rspam02 X-Rspamd-Queue-Id: 07C5D1A0005 X-Rspam-User: X-Stat-Signature: njnwzud9j4skpfub1qpyu8yx44r8b84u X-HE-Tag: 1666121339-21766 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: Hi-- On 10/18/22 11:56, 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. > > Here is a snippet of the crash backtrace from an instrumented kernel, > scratched one line for sensitive reason - > > crash> bt > PID: 3250 TASK: ffff9cb50fe23d80 CPU: 18 COMMAND: "cat" > #0 [ffffc0bacf377998] machine_kexec at ffffffff9b06c7c1 > #1 [ffffc0bacf3779f8] __crash_kexec at ffffffff9b13bb52 > #2 [ffffc0bacf377ac8] crash_kexec at ffffffff9b13cdac > #3 [ffffc0bacf377ae8] oops_end at ffffffff9b03357a > #4 [ffffc0bacf377b10] die at ffffffff9b033c32 > #5 [ffffc0bacf377b40] do_general_protection at ffffffff9b030c52 > #6 [ffffc0bacf377b70] general_protection at ffffffff9ba03db4 > [exception RIP: string_nocheck+19] > RIP: ffffffff9b87cc73 RSP: ffffc0bacf377c20 RFLAGS: 00010286 > RAX: 0000000000000000 RBX: ffff9da13fc17fff RCX: ffff0a00ffffff04 > RDX: 726f635f63616465 RSI: ffff9da13fc17fff RDI: ffffffffffffffff > RBP: ffffc0bacf377c20 R8: ffff9da0bfd2f010 R9: ffff9da0bfc18000 > R10: 0000000000001000 R11: 0000000000000000 R12: 726f635f63616465 > R13: ffff0a00ffffff04 R14: ffffffff9c1a6a4f R15: ffffffff9c1a6a4f > ORIG_RAX: ffffffffffffffff CS: 0010 SS: 0018 > #7 [ffffc0bacf377c28] string at ffffffff9b87ce98 > #8 [ffffc0bacf377c58] vsnprintf at ffffffff9b87efe3 > #9 [ffffc0bacf377cb8] sprintf at ffffffff9b87f506 > #10 [ffffc0bacf377d18] <------------------------------> > #11 [ffffc0bacf377d28] dev_attr_show at ffffffff9b56d183 > #12 [ffffc0bacf377d48] sysfs_kf_seq_show at ffffffff9b3272dc > #13 [ffffc0bacf377d68] kernfs_seq_show at ffffffff9b32576c > #14 [ffffc0bacf377d78] seq_read at ffffffff9b2be407 > #15 [ffffc0bacf377de8] kernfs_fop_read at ffffffff9b325ffe > #16 [ffffc0bacf377e28] __vfs_read at ffffffff9b2940ea > #17 [ffffc0bacf377eb0] vfs_read at ffffffff9b2942ac > #18 [ffffc0bacf377ee0] sys_read at ffffffff9b29485c > #19 [ffffc0bacf377f28] do_syscall_64 at ffffffff9b003ca9 > #20 [ffffc0bacf377f50] entry_SYSCALL_64_after_hwframe at ffffffff9ba001b1 > > crash> dis ffffffff9b87cc73 > 0xffffffff9b87cc73 : movzbl (%rdx),%r8d > > and RDX: 726f635f63616465 was a non-canonical address. > > After applying this patch to the instrumented kernel, instead of panic, > the "cat" command produced "(efault)" > >> >> I wonder if this patch would prevent the crash or if the broken >> kernel would crash later anyway. > > A broken kernel has a different issue to be fixed, the upstream kernel > isn't broken, it could just offer better protect in case a bug was > introduced in future. > >> >>> That said, I realized that not all >>> architecture implement meaningful kern_addr_valid(), so this line >>> if ((unsigned long)ptr < PAGE_SIZE || IS_ERR_VALUE(ptr)) >>> is still need. I'll send v2. >> >> Please, add linux-mm@kvack.org into CC. > > Will do. > >> I wonder if kern_addr_valid() >> is safe to use anywhere, especially during early boot. I wonder if >> it would make sense to implement it on all architectures. > > On x86 architecture, kern_addr_valid() looks safe to me though, on > several other architectures, it's defined (1). You might want to compare this patch, which seems to have some support: https://lore.kernel.org/lkml/20221018074014.185687-1-wangkefeng.wang@huawei.com/ -- ~Randy