From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752302AbdLFX01 (ORCPT ); Wed, 6 Dec 2017 18:26:27 -0500 Received: from mail-ua0-f196.google.com ([209.85.217.196]:46454 "EHLO mail-ua0-f196.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751598AbdLFX0Y (ORCPT ); Wed, 6 Dec 2017 18:26:24 -0500 X-Google-Smtp-Source: AGs4zMZ9b83kcEZF0Ytz+g/ByNUpwjim9YJ9aPjzZjgr/ivyEwjN5GsA10YYz1l4GXvd1oV56Ci2+WfYFfWBvRwuA0k= MIME-Version: 1.0 In-Reply-To: References: <20171204142553.GA3344@pjb1027-Latitude-E5410> From: Kees Cook Date: Wed, 6 Dec 2017 15:26:22 -0800 X-Google-Sender-Auth: YVWojxv3MhZID1oaGuxHa4eXkSg Message-ID: Subject: Re: [kernel-hardening][PATCH v3 1/3] arm: mm: dump: make page table dumping reusable To: Jinbum Park Cc: linux-arm-kernel@lists.infradead.org, LKML , kernel-hardening@lists.openwall.com, Afzal Mohammed , Mark Rutland , Laura Abbott , Russell King , Greg KH , Vladimir Murzin , Arnd Bergmann Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Dec 6, 2017 at 1:45 AM, Jinbum Park wrote: >>> +#ifndef __ASM_PTDUMP_H >>> +#define __ASM_PTDUMP_H >>> + >>> +#ifdef CONFIG_ARM_PTDUMP_CORE >> >> Is this #ifdef needed? I think this file is only included in dump.c >> and ptdump_debugfs.c, both of which are only built when >> CONFIG_ARM_PTDUMP_CORE is defined. > > Looking at next patch in this patch-set series ([PATCH v3 3/3] arm: > mm: dump: add checking for writable and executable pages), > Not only dump.c and ptdump_debugfs.c but also arch/arm/mm/init.c > include this file (ptdump.h) to call debug_checkwx(). > mm/init.c is not built only when CONFIG_ARM_PTDUMP_CORE is defined. > So, This #ifdef seems not be needed for this patch, but is needed for > this patch-set series. > > >>> +static int ptdump_init(void) >>> +{ >>> + ptdump_initialize(); >>> + return ptdump_debugfs_register(&kernel_ptdump_info, >>> + "kernel_page_tables"); >> >> This changes the return value of ptdump_init. This should do similar >> to what was done before: >> >> return ptdump_debugfs_register(&kernel_ptdump_info, >> "kernel_page_tables") ? 0 : -ENOMEM; > > > ptdump_debugfs_register() already returns what you think. > >>> +int ptdump_debugfs_register(struct ptdump_info *info, const char *name) >>> +{ >>> + struct dentry *pe; >>> + >>> + pe = debugfs_create_file(name, 0400, NULL, info, &ptdump_fops); >>> + return pe ? 0 : -ENOMEM; >>> + >>> +} > > So "return ptdump_debugfs_register(~~)" is fine. Ah! Yes, I totally missed the change from create_file to debugfs_register. Sorry for the noise! -Kees -- Kees Cook Pixel Security