Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1425329AbdDUXTO (ORCPT ); Fri, 21 Apr 2017 19:19:14 -0400 Received: from mail-io0-f180.google.com ([209.85.223.180]:35117 "EHLO mail-io0-f180.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1425136AbdDUXTH (ORCPT ); Fri, 21 Apr 2017 19:19:07 -0400 MIME-Version: 1.0 In-Reply-To: References: <1492640420-27345-1-git-send-email-tixxdz@gmail.com> <1492640420-27345-3-git-send-email-tixxdz@gmail.com> From: Kees Cook Date: Fri, 21 Apr 2017 16:19:05 -0700 X-Google-Sender-Auth: bYC9ZS8wDA0dWvYkPcw1oTtVGVA Message-ID: Subject: Re: [PATCH v3 2/2] modules:capabilities: add a per-task modules autoload restriction To: Andy Lutomirski Cc: Djalal Harouni , Linux Kernel Mailing List , Andrew Morton , "Serge E. Hallyn" , "kernel-hardening@lists.openwall.com" , LSM List , Linux API , Dongsu Park , Casey Schaufler , James Morris , Paul Moore , Tetsuo Handa , Greg Kroah-Hartman , Jonathan Corbet , Jessica Yu , Rusty Russell , Arnaldo Carvalho de Melo , Mauro Carvalho Chehab , Ingo Molnar , belakhdar abdeldjalil , Peter Zijlstra Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2332 Lines: 60 On Wed, Apr 19, 2017 at 7:41 PM, Andy Lutomirski wrote: > On Wed, Apr 19, 2017 at 4:43 PM, Kees Cook wrote: >> On Wed, Apr 19, 2017 at 4:15 PM, Andy Lutomirski wrote: >>> On Wed, Apr 19, 2017 at 3:20 PM, Djalal Harouni wrote: >>>> +/* Sets task's modules_autoload */ >>>> +static inline int task_set_modules_autoload(struct task_struct *task, >>>> + unsigned long value) >>>> +{ >>>> + if (value > MODULES_AUTOLOAD_DISABLED) >>>> + return -EINVAL; >>>> + else if (task->modules_autoload > value) >>>> + return -EPERM; >>>> + else if (task->modules_autoload < value) >>>> + task->modules_autoload = value; >>>> + >>>> + return 0; >>>> +} >>> >>> This needs to be more locked down. Otherwise someone could set this >>> and then run a setuid program. Admittedly, it would be quite odd if >>> this particular thing causes a problem, but the issue exists >>> nonetheless. >> >> Eeeh, I don't agree this needs to be changed. APIs provided by modules >> are different than the existing privilege-manipulation syscalls this >> concern stems from. Applications are already forced to deal with >> things being missing like this in the face of it simply not being >> built into the kernel. >> >> Having to hide this behind nnp seems like it'd reduce its utility... >> > > I think that adding an inherited boolean to task_struct that can be > set by unprivileged tasks and passed to privileged tasks is a terrible > precedent. Ideally someone would try to find all the existing things > like this and kill them off. (Tristate, not boolean, but yeah.) I see two others besides seccomp and nnp: PR_MCE_KILL PR_SET_THP_DISABLE I really don't think this needs nnp protection. > I agree that I don't see how one would exploit this particular > feature, but I still think I dislike the approach. This is a slippery > slope to adding a boolean for perf_event_open(), unshare(), etc, and > we should solve these for real rather than half-arsing them IMO. I disagree (obviously); this would be protecting the entire module autoload attack surface. That's hardly a specific control, and it's a demonstrably needed flag. -Kees -- Kees Cook Pixel Security