Received-SPF: pass (google.com: domain of linux-kernel+bounces-71230-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <7a1c2bf7-df85-431a-ae21-82eeb171f1c4@gmail.com>
Date: Mon, 19 Feb 2024 19:41:13 +0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [RESEND RFC] kernel/ksysfs.c: restrict /sys/kernel/notes to root
 access
To: Kees Cook <keescook@chromium.org>, Greg KH <gregkh@linuxfoundation.org>
Cc: linux-hardening@vger.kernel.org, Juergen Gross <jgross@suse.com>,
 Boris Ostrovsky <boris.ostrovsky@oracle.com>,
 Stefano Stabellini <sstabellini@kernel.org>,
 Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>,
 Guixiong Wei <weiguixiong@bytedance.com>, linux-kernel@vger.kernel.org,
 xen-devel@lists.xenproject.org
References: <20240218073501.54555-1-guixiongwei@gmail.com>
 <2024021825-skiing-trustee-a56a@gregkh> <202402180028.6DB512C50@keescook>
From: Guixiong Wei <guixiongwei@gmail.com>
In-Reply-To: <202402180028.6DB512C50@keescook>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit


On 2024/2/18 17:04, Kees Cook wrote:
> On Sun, Feb 18, 2024 at 08:47:03AM +0100, Greg KH wrote:
>> On Sun, Feb 18, 2024 at 03:35:01PM +0800, Guixiong Wei wrote:
>>> From: Guixiong Wei <weiguixiong@bytedance.com>
>>>
>>> Restrict non-privileged user access to /sys/kernel/notes to
>>> avoid security attack.
>>>
>>> The non-privileged users have read access to notes. The notes
>>> expose the load address of startup_xen. This address could be
>>> used to bypass KASLR.
>> How can it be used to bypass it?
>>
>> KASLR is, for local users, pretty much not an issue, as that's not what
>> it protects from, only remote ones.
>>
>>> For example, the startup_xen is built at 0xffffffff82465180 and
>>> commit_creds is built at 0xffffffff810ad570 which could read from
>>> the /boot/System.map. And the loaded address of startup_xen is
>>> 0xffffffffbc265180 which read from /sys/kernel/notes. So the loaded
>>> address of commit_creds is 0xffffffffbc265180 - (0xffffffff82465180
>>>   - 0xffffffff810ad570) = 0xffffffffbaead570.
>> I've cc: the hardening list on this, I'm sure the developers there have
>> opinions about this.
> Oh eww, why is Xen spewing addresses into the notes section? (This must
> be how it finds its entry point? But that would be before relocations
> happen...)
>
> But yes, I can confirm that relocations are done against the .notes
> section at boot, so the addresses exposed in .notes is an immediate
> KASLR offset exposure.
>
> In /sys/kernel/notes (are there any tools to read this? I wrote my own...)
>
> 	type: 1
> 	name: Xen
> 	desc: 0xb4a711c0 0xffffffff
>
> which matches a privileged read of /proc/kallsysms:
>
> 	ffffffffb4a711c0 T startup_xen
>
> (and the hypercall_page too)
>
> There are all coming from arch/x86/xen/xen-head.S:
>
>          ELFNOTE(Xen, XEN_ELFNOTE_GUEST_OS,       .asciz "linux")
>          ELFNOTE(Xen, XEN_ELFNOTE_GUEST_VERSION,  .asciz "2.6")
>          ELFNOTE(Xen, XEN_ELFNOTE_XEN_VERSION,    .asciz "xen-3.0")
> #ifdef CONFIG_XEN_PV
>          ELFNOTE(Xen, XEN_ELFNOTE_VIRT_BASE,      _ASM_PTR __START_KERNEL_map)
>          /* Map the p2m table to a 512GB-aligned user address. */
>          ELFNOTE(Xen, XEN_ELFNOTE_INIT_P2M,       .quad (PUD_SIZE * PTRS_PER_PUD))
>          ELFNOTE(Xen, XEN_ELFNOTE_ENTRY,          _ASM_PTR startup_xen)
> ...
>
> Introduced in commit 5ead97c84fa7 ("xen: Core Xen implementation")
>
> Exposed in commit da1a679cde9b ("Add /sys/kernel/notes")
>
> Amazingly these both went in on the same release (v2.6.23, 2007). This
> has been exposed for longer than KASLR has been upstream. :P
>
>>> Signed-off-by: Guixiong Wei <weiguixiong@bytedance.com>
>>> ---
>>>   kernel/ksysfs.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/kernel/ksysfs.c b/kernel/ksysfs.c
>>> index b1292a57c2a5..09bc0730239b 100644
>>> --- a/kernel/ksysfs.c
>>> +++ b/kernel/ksysfs.c
>>> @@ -199,7 +199,7 @@ static ssize_t notes_read(struct file *filp, struct kobject *kobj,
>>>   static struct bin_attribute notes_attr __ro_after_init  = {
>>>   	.attr = {
>>>   		.name = "notes",
>>> -		.mode = S_IRUGO,
>>> +		.mode = S_IRUSR,
>>>   	},
>>>   	.read = &notes_read,
>>>   };
> Yes please.
>
> Reviewed-by: Kees Cook <keescook@chromium.org>
>
> I wonder if we should also remove relocations that are aimed at the
> .notes section for good measure? If that had already been true, this
> would have just given the same info as System.map.
That's a good idea, but it depends on whether the user space tool can 
accept the remove relocation address.
>> No objection from me, but what userspace tool requires access to this
>> file today?  Will it break if permissions are changed on it?
 From the exposed content, it seems that the main users are Xen-related 
tools. I add Xen list, developers should be able to provide some 
information.
>> And what about the module notes files?  If you change one, shouldn't you
>> change all?
 From what I currently know, the module note files do not expose any 
kernel symbol address, so there is no need for modification.
> Luckily all of _those_ contain what I'd expect: the Linux and
> GNU.build-id notes, which are harmless. But if we're going to suddenly
> have things appearing in here, let's make those root-only too.
Yes, but I also not sure whether the user space tools using this file 
can accept this permission modification.