From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1030219AbbCLNng (ORCPT ); Thu, 12 Mar 2015 09:43:36 -0400 Received: from mail.savoirfairelinux.com ([209.172.62.77]:61886 "EHLO mail.savoirfairelinux.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932457AbbCLNne (ORCPT ); Thu, 12 Mar 2015 09:43:34 -0400 Date: Thu, 12 Mar 2015 09:43:32 -0400 (EDT) From: Vivien Didelot To: Greg Kroah-Hartman Cc: Guenter Roeck , linux-kernel@vger.kernel.org, kernel@savoirfairelinux.com Message-ID: <1126697358.202300.1426167812093.JavaMail.zimbra@savoirfairelinux.com> In-Reply-To: <55016CC8.3070304@roeck-us.net> References: <1426098131-20106-1-git-send-email-vivien.didelot@savoirfairelinux.com> <1426098131-20106-3-git-send-email-vivien.didelot@savoirfairelinux.com> <20150312100151.GD3682@kroah.com> <55016CC8.3070304@roeck-us.net> Subject: Re: [PATCH v2 2/3] sysfs: Only accept read/write permissions for file attributes MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-Mailer: Zimbra 8.0.9_GA_6191 (ZimbraWebClient - FF36 (Linux)/8.0.9_GA_6191) Thread-Topic: sysfs: Only accept read/write permissions for file attributes Thread-Index: 2ssHG3JNEo4XiWc7yTqSpojfKzLZjw== Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Greg, Guenter, > On 03/12/2015 03:01 AM, Greg Kroah-Hartman wrote: > > On Wed, Mar 11, 2015 at 02:22:10PM -0400, Vivien Didelot wrote: > >> For sysfs file attributes, only read and write permissions make sense. > >> Mask provided attribute permissions accordingly and send a warning > >> to the console if invalid permission bits are set. > >> > >> This patch is originally from Guenter [1] and includes the fixup > >> explained in the thread, that is printing permissions in octal format > >> and limiting the scope of attributes to SYSFS_PREALLOC | 0664. > >> > >> [1] https://lkml.org/lkml/2015/1/19/599 > >> > >> Cc: Guenter Roeck > >> Signed-off-by: Vivien Didelot > >> --- > >> fs/sysfs/group.c | 6 ++++++ > >> 1 file changed, 6 insertions(+) > >> > >> diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c > >> index 3fdccd9..b400c04 100644 > >> --- a/fs/sysfs/group.c > >> +++ b/fs/sysfs/group.c > >> @@ -55,6 +55,12 @@ static int create_files(struct kernfs_node *parent, > >> struct kobject *kobj, > >> if (!mode) > >> continue; > >> } > >> + > >> + WARN(mode & ~(SYSFS_PREALLOC | 0664), > >> + "Attribute %s: Invalid permissions 0%o\n", > >> + (*attr)->name, mode); > >> + > >> + mode &= SYSFS_PREALLOC | 0664; > > > > How does a "normal" boot look with this warning in place? There still > > seem to be a number of files in sysfs that might trigger this. > > > I was under the impression that they all were addressed, but I may have > missed some pattern(s). Can you point me to an example, by any chance ? Same here, given https://lkml.org/lkml/2015/1/20/584, it looked like only hid-lg4ff had executable attributes (which has been fixed) and a few attributes in pci-sysfs have write access to group, but this doesn't trigger the warning. > > Also, we have a build-time warning if a sysfs file is this type of > > attribute, shouldn't we just rely on that instead of this run-time > > warning? > > > > The mode value can be returned from an is_visible function, and even if not > there is no guarantee that the build-time warning triggers (attribute lists > can be generated manually, for example). Indeed, as mentioned in the previous thread https://lkml.org/lkml/2015/1/20/877 the build-time macro VERIFY_OCTAL_PERMISSIONS() is not enough since an attribute mode can be changed at run-time (a run-time equivalent of this macro can be added in a future patch, if necessary). Thanks Greg for the Signed-off reminders for 1/3 and 3/3. A v3 with them is ready to be sent, if you are ok with the patchset. Thanks, -v