Re: [PATCH security-next v5 00/30] LSM: Explict ordering
On Wed, Oct 24, 2018 at 1:56 AM, Casey Schaufler wrote: > On 10/23/2018 12:05 PM, Casey Schaufler wrote: >> On 10/23/2018 11:50 AM, Kees Cook wrote: >> >>> Did you poke around at my combined series? >>> https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/log/?h=lsm/ordering-v6-blob-sharing >> I hope to do that on the plane later today. > > I had a chance to poke at the combined series and it > all seems to work as advertised. /me stares at John, Paul, and Stephen. Hurry up and get off your planes! ;) -Kees -- Kees Cook
Re: [PATCH v4 19/31] Documentation: kconfig: document a new Kconfig macro language
On Wed, May 16, 2018 at 11:16 PM, Masahiro Yamada
<yamada.masah...@socionext.com> wrote:
> Add a document for the macro language introduced to Kconfig.
>
> The motivation of this work is to move the compiler option tests to
> Kconfig from Makefile. A number of kernel features require the
> compiler support. Enabling such features blindly in Kconfig ends up
> with a lot of nasty build-time testing in Makefiles. If a chosen
> feature turns out unsupported by the compiler, what the build system
> can do is either to disable it (silently!) or to forcibly break the
> build, despite Kconfig has let the user to enable it. By moving the
> compiler capability tests to Kconfig, features unsupported by the
> compiler will be hidden automatically.
>
> This change was strongly prompted by Linus Torvalds. You can find
> his suggestions [1] [2] in ML. The original idea was to add a new
> attribute with 'option shell=...', but I found more generalized text
> expansion would make Kconfig more powerful and lovely. The basic
> ideas are from Make, but there are some differences.
>
> [1]: https://lkml.org/lkml/2016/12/9/577
> [2]: https://lkml.org/lkml/2018/2/7/527
>
> Signed-off-by: Masahiro Yamada <yamada.masah...@socionext.com>
(Added Randy, Jon, and linux-doc to CC for more review)
This should likely be written in .rst and linked to from the developer index...
https://www.kernel.org/doc/html/latest/doc-guide/sphinx.html#writing-documentation
As for the content, though:
Reviewed-by: Kees Cook <keesc...@chromium.org>
-Kees
> ---
>
> Changes in v4:
> - Update according to the syntax change
>
> Changes in v3:
> - Newly added
>
> Changes in v2: None
>
> Documentation/kbuild/kconfig-macro-language.txt | 252
>
> MAINTAINERS | 2 +-
> 2 files changed, 253 insertions(+), 1 deletion(-)
> create mode 100644 Documentation/kbuild/kconfig-macro-language.txt
>
> diff --git a/Documentation/kbuild/kconfig-macro-language.txt
> b/Documentation/kbuild/kconfig-macro-language.txt
> new file mode 100644
> index 000..a8dc792
> --- /dev/null
> +++ b/Documentation/kbuild/kconfig-macro-language.txt
> @@ -0,0 +1,252 @@
> +Concept
> +---
> +
> +The basic idea was inspired by Make. When we look at Make, we notice sort of
> +two languages in one. One language describes dependency graphs consisting of
> +targets and prerequisites. The other is a macro language for performing
> textual
> +substitution.
> +
> +There is clear distinction between the two language stages. For example, you
> +can write a makefile like follows:
> +
> +APP := foo
> +SRC := foo.c
> +CC := gcc
> +
> +$(APP): $(SRC)
> +$(CC) -o $(APP) $(SRC)
> +
> +The macro language replaces the variable references with their expanded form,
> +and handles as if the source file were input like follows:
> +
> +foo: foo.c
> +gcc -o foo foo.c
> +
> +Then, Make analyzes the dependency graph and determines the targets to be
> +updated.
> +
> +The idea is quite similar in Kconfig - it is possible to describe a Kconfig
> +file like this:
> +
> +CC := gcc
> +
> +config CC_HAS_FOO
> +def_bool $(shell, $(srctree)/scripts/gcc-check-foo.sh $(CC))
> +
> +The macro language in Kconfig processes the source file into the following
> +intermediate:
> +
> +config CC_HAS_FOO
> +def_bool y
> +
> +Then, Kconfig moves onto the evaluation stage to resolve inter-symbol
> +dependency as explained in kconfig-language.txt.
> +
> +
> +Variables
> +-
> +
> +Like in Make, a variable in Kconfig works as a macro variable. A macro
> +variable is expanded "in place" to yield a text string that may then be
> +expanded further. To get the value of a variable, enclose the variable name
> in
> +$( ). The parentheses are required even for single-letter variable names; $X
> is
> +a syntax error. The curly brace form as in ${CC} is not supported either.
> +
> +There are two types of variables: simply expanded variables and recursively
> +expanded variables.
> +
> +A simply expanded variable is defined using the := assignment operator. Its
> +righthand side is expanded immediately upon reading the line from the Kconfig
> +file.
> +
> +A recursively expanded variable is defined using the = assignment operator.
> +Its righthand side is simply stored as the value of the variable without
> +expanding it in any way. Instead, the expansion is performed when the
> variable
> +is used.
> +
> +There is another type of assignment operator; += is used to append text to a
> +variable.
Re: [PATCH v3 0/4] Better integrate seccomp logging and auditing
On Sun, May 6, 2018 at 2:31 PM, Paul Moore <p...@paul-moore.com> wrote: > On Thu, May 3, 2018 at 9:08 PM, Tyler Hicks <tyhi...@canonical.com> wrote: >> Seccomp received improved logging controls in v4.14. Applications can opt >> into >> logging of "handled" actions (SECCOMP_RET_TRAP, SECCOMP_RET_TRACE, >> SECCOMP_RET_ERRNO) using the SECCOMP_FILTER_FLAG_LOG bit when loading >> filters. >> They can also debug filter matching with the new SECCOMP_RET_LOG action. >> Administrators can prevent specific actions from being logged using the >> kernel.seccomp.actions_logged sysctl. >> >> However, one corner case intentionally wasn't addressed in those v4.14 >> changes. >> When a process is being inspected by the audit subsystem, seccomp's decision >> making for logging ignores the new controls and unconditionally logs every >> action taken except for SECCOMP_RET_ALLOW. This isn't particularly useful >> since >> many existing applications don't intend to log handled actions due to them >> occurring very frequently. This amount of logging fills the audit logs >> without >> providing many benefits now that application authors have fine grained >> controls >> at their disposal. >> >> This patch set aligns the seccomp logging behavior for both audited and >> non-audited processes. It also emits an audit record, if auditing is enabled, >> when the kernel.seccomp.actions_logged sysctl is written to so that there's a >> paper trail when entire actions are quieted. >> >> Changes in v3: >> * Patch 3 >> - Never drop a field when emitting the audit record >> - Use the value "?" for the actions field when an error occurred while >> writing to the sysctl >> - Use the value "?" for the actions and/or old-actions fields when a >> failure >> to translate actions to names >> - Use the value "(none)" for the actions and/or old-actions fields when no >> actions are specified >> + This is possible when writing an empty string to the sysctl >> - Update the commit message to note the new values and give an example of >> when an empty string is written >> * Patch 4 >> - Adjust the control flow of seccomp_log() to exit early if nothing should >> be >> logged >> >> Changes in v2: >> * Patch 2 >> - New patch, allowing for a configurable separator between action names >> * Patch 3 >> - The value of the actions field in the audit record now uses a comma >> instead >> of a space >> - The value of the actions field in the audit record is no longer enclosed >> in >> quotes >> - audit_log_start() is called with the current processes' audit_context in >> audit_seccomp_actions_logged() >> - audit_seccomp_actions_logged() no longer records the pid, uid, auid, tty, >> ses, task context, comm, or executable path >> - The new and old value of seccomp_actions_logged is recorded in the >> AUDIT_CONFIG_CHANGE record >> - The value of the "res" field in the CONFIG_CHANGE audit record is >> corrected >> (1 indicates success, 0 failure) >> - Updated patch 3's commit message to reflect the updated audit record >> format >> in the examples >> * Patch 4 >> - A function comment for audit_seccomp() was added to explain, among other >> things, that event filtering is performed in seccomp_log() > > Kees, are you still okay with v3? Also, are you okay with these > patches going in via the audit tree, or would you prefer to take them > via seccomp? I've got a slight preference for the audit tree myself, > but as I said before, as long as it hits Linus' tree I'm happy. Yup, it looks good. I have no tree preference, so you win! :) Please consider the whole series: Acked-by: Kees Cook <keesc...@chromium.org> -Kees -- Kees Cook Pixel Security -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Documentation: refcount-vs-atomic: Update reference to LKMM doc.
On Fri, May 4, 2018 at 2:11 PM, Andrea Parri <andrea.pa...@amarulasolutions.com> wrote: > The LKMM project has moved to 'tools/memory-model/'. > > Signed-off-by: Andrea Parri <andrea.pa...@amarulasolutions.com> > --- > Documentation/core-api/refcount-vs-atomic.rst | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/Documentation/core-api/refcount-vs-atomic.rst > b/Documentation/core-api/refcount-vs-atomic.rst > index 83351c258cdb9..322851bada167 100644 > --- a/Documentation/core-api/refcount-vs-atomic.rst > +++ b/Documentation/core-api/refcount-vs-atomic.rst > @@ -17,7 +17,7 @@ in order to help maintainers validate their code against > the change in > these memory ordering guarantees. > > The terms used through this document try to follow the formal LKMM defined in > -github.com/aparri/memory-model/blob/master/Documentation/explanation.txt > +tools/memory-model/Documentation/explanation.txt. > > memory-barriers.txt and atomic_t.txt provide more background to the > memory ordering in general and for atomic operations specifically. Will this get linkified by rst ? -Kees -- Kees Cook Pixel Security -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 4/4] seccomp: Don't special case audited processes when logging
On Wed, May 2, 2018 at 8:53 AM, Tyler Hicks <tyhi...@canonical.com> wrote: > diff --git a/kernel/seccomp.c b/kernel/seccomp.c > index da78835..9029d9d 100644 > --- a/kernel/seccomp.c > +++ b/kernel/seccomp.c > @@ -584,18 +584,13 @@ static inline void seccomp_log(unsigned long syscall, > long signr, u32 action, > } > > /* > -* Force an audit message to be emitted when the action is RET_KILL_*, > -* RET_LOG, or the FILTER_FLAG_LOG bit was set and the action is > -* allowed to be logged by the admin. > +* Emit an audit message when the action is RET_KILL_*, RET_LOG, or > the > +* FILTER_FLAG_LOG bit was set. The admin has the ability to silence > +* any action from being logged by removing the action name from the > +* seccomp_actions_logged sysctl. > */ > if (log) > - return __audit_seccomp(syscall, signr, action); > - > - /* > -* Let the audit subsystem decide if the action should be audited > based > -* on whether the current task itself is being audited. > -*/ > - return audit_seccomp(syscall, signr, action); > + audit_seccomp(syscall, signr, action); > } This whole series looks great to me. If I can get an Ack from Paul for the audit bits, I can take it via the seccomp tree. One minor nit on seccomp_log() above, I'd probably change this to show the "exception" case as "out of line" of normal code flow. i.e. instead of "if (log) audit_seccomp", invert it to return early: ... if (!log) return; audit_seccomp(syscall, signr, action); } But if there isn't some other need for a v3, I can just make this change when I commit. Thanks for fixing this up! -Kees -- Kees Cook Pixel Security -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] coresight: Remove %px for printing pcsr value
On Tue, May 1, 2018 at 10:00 PM, Leo Yan <leo@linaro.org> wrote: > The driver prints pcsr twice: the first time it uses specifier %px to > print hexadecimal pcsr value and the second time uses specifier %pS for > output kernel symbols. > > As suggested by Kees, using %pS should be sufficient and %px isn't > necessary; the reason is if the pcsr is a kernel space address, we can > easily get to know the code line from %pS format, on the other hand, if > the pcsr value doesn't fall into kernel space range (e.g. if the CPU is > stuck in firmware), %pS also gives out pcsr hexadecimal value. > > So this commit removes useless %px and update section "Output format" > in the document for alignment between the code and document. > > Suggested-by: Kees Cook <keesc...@chromium.org> > Cc: Mathieu Poirier <mathieu.poir...@linaro.org> > Signed-off-by: Leo Yan <leo@linaro.org> Thanks! Reviewed-by: Kees Cook <keesc...@chromium.org> -Kees > --- > Documentation/trace/coresight-cpu-debug.txt | 4 ++-- > drivers/hwtracing/coresight/coresight-cpu-debug.c | 2 +- > 2 files changed, 3 insertions(+), 3 deletions(-) > > diff --git a/Documentation/trace/coresight-cpu-debug.txt > b/Documentation/trace/coresight-cpu-debug.txt > index 2b9b51c..89ab09e 100644 > --- a/Documentation/trace/coresight-cpu-debug.txt > +++ b/Documentation/trace/coresight-cpu-debug.txt > @@ -177,11 +177,11 @@ Here is an example of the debugging output format: > ARM external debug module: > coresight-cpu-debug 85.debug: CPU[0]: > coresight-cpu-debug 85.debug: EDPRSR: 0001 (Power:On DLK:Unlock) > -coresight-cpu-debug 85.debug: EDPCSR: [] > handle_IPI+0x174/0x1d8 > +coresight-cpu-debug 85.debug: EDPCSR: handle_IPI+0x174/0x1d8 > coresight-cpu-debug 85.debug: EDCIDSR: > coresight-cpu-debug 85.debug: EDVIDSR: 9000 (State:Non-secure > Mode:EL1/0 Width:64bits VMID:0) > coresight-cpu-debug 852000.debug: CPU[1]: > coresight-cpu-debug 852000.debug: EDPRSR: 0001 (Power:On DLK:Unlock) > -coresight-cpu-debug 852000.debug: EDPCSR: [] > debug_notifier_call+0x23c/0x358 > +coresight-cpu-debug 852000.debug: EDPCSR: debug_notifier_call+0x23c/0x358 > coresight-cpu-debug 852000.debug: EDCIDSR: > coresight-cpu-debug 852000.debug: EDVIDSR: 9000 (State:Non-secure > Mode:EL1/0 Width:64bits VMID:0) > diff --git a/drivers/hwtracing/coresight/coresight-cpu-debug.c > b/drivers/hwtracing/coresight/coresight-cpu-debug.c > index 9cdb3fb..78a054e 100644 > --- a/drivers/hwtracing/coresight/coresight-cpu-debug.c > +++ b/drivers/hwtracing/coresight/coresight-cpu-debug.c > @@ -315,7 +315,7 @@ static void debug_dump_regs(struct debug_drvdata *drvdata) > } > > pc = debug_adjust_pc(drvdata); > - dev_emerg(dev, " EDPCSR: [<%px>] %pS\n", (void *)pc, (void *)pc); > + dev_emerg(dev, " EDPCSR: %pS\n", (void *)pc); > > if (drvdata->edcidsr_present) > dev_emerg(dev, " EDCIDSR: %08x\n", drvdata->edcidsr); > -- > 2.7.4 > -- Kees Cook Pixel Security -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 3/8] sysctl: Warn when a clamped sysctl parameter is set out of range
I like this series overall, thanks! No objections from me. One thing I
noted, though:
On Fri, Apr 27, 2018 at 2:00 PM, Waiman Long <long...@redhat.com> wrote:
> if (param->min && *param->min > val) {
> if (clamp) {
> val = *param->min;
> + clamped = true;
> } else {
> return -EINVAL;
> }
This appears as a common bit of logic in many places in the series. It
seems like it'd make sense to make this a helper of some kind?
-Kees
--
Kees Cook
Pixel Security
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] [v2] docs: clarify security-bugs disclosure policy
On Wed, Mar 7, 2018 at 1:46 PM, Dave Hansen <dave.han...@linux.intel.com> wrote: > > From: Dave Hansen <dave.han...@linux.intel.com> > > I think we need to soften the language a bit. It might scare folks > off, especially the: > > We prefer to fully disclose the bug as soon as possible. > > which is not really the case. Linus says: > > It's not full disclosure, it's not coordinated disclosure, > and it's not "no disclosure". It's more like just "timely > open fixes". > > I changed a bit of the wording in here, but mostly to remove the word > "disclosure" since it seems to mean very specific things to people > that we do not mean here. > > Signed-off-by: Dave Hansen <dave.han...@linux.intel.com> > Reviewed-by: Dan Williams <dan.j.willi...@intel.com> > Cc: Thomas Gleixner <t...@linutronix.de> > Cc: Greg Kroah-Hartman <gre...@linuxfoundation.org> > Cc: Linus Torvalds <torva...@linux-foundation.org> > Cc: Alan Cox <gno...@lxorguk.ukuu.org.uk> > Cc: Andrea Arcangeli <aarca...@redhat.com> > Cc: Andy Lutomirski <l...@kernel.org> > Cc: Kees Cook <keesc...@google.com> > Cc: Tim Chen <tim.c.c...@linux.intel.com> > Cc: Alexander Viro <v...@zeniv.linux.org.uk> > Cc: Andrew Morton <a...@linux-foundation.org> > Cc: linux-doc@vger.kernel.org > Cc: Jonathan Corbet <cor...@lwn.net> > Cc: Mark Rutland <mark.rutl...@arm.com> > --- > > b/Documentation/admin-guide/security-bugs.rst | 24 +--- > 1 file changed, 13 insertions(+), 11 deletions(-) > > diff -puN Documentation/admin-guide/security-bugs.rst~embargo2 > Documentation/admin-guide/security-bugs.rst > --- a/Documentation/admin-guide/security-bugs.rst~embargo2 2018-03-07 > 13:23:49.390228208 -0800 > +++ b/Documentation/admin-guide/security-bugs.rst 2018-03-07 > 13:42:37.618225395 -0800 > @@ -29,18 +29,20 @@ made public. > Disclosure > -- > > -The goal of the Linux kernel security team is to work with the > -bug submitter to bug resolution as well as disclosure. We prefer > -to fully disclose the bug as soon as possible. It is reasonable to > -delay disclosure when the bug or the fix is not yet fully understood, > -the solution is not well-tested or for vendor coordination. However, we > -expect these delays to be short, measurable in days, not weeks or months. > -A disclosure date is negotiated by the security team working with the > -bug submitter as well as vendors. However, the kernel security team > -holds the final say when setting a disclosure date. The timeframe for > -disclosure is from immediate (esp. if it's already publicly known) > +The goal of the Linux kernel security team is to work with the bug > +submitter to understand and fix the bug. We prefer to publish the fix as > +soon as possible, but try to avoid public discussion of the bug itself > +and leave that to others. > + > +Publishing the fix may be delayed when the bug or the fix is not yet > +fully understood, the solution is not well-tested or for vendor > +coordination. However, we expect these delays to be short, measurable in > +days, not weeks or months. A release date is negotiated by the security > +team working with the bug submitter as well as vendors. However, the > +kernel security team holds the final say when setting a timeframe. The > +timeframe varies from immediate (esp. if it's already publicly known bug) Nit: I think "a" is missing. I was expecting: "... already a publicly known ... > to a few weeks. As a basic default policy, we expect report date to > -disclosure date to be on the order of 7 days. > +release date to be on the order of 7 days. Otherwise, yeah, looks good. Acked-by: Kees Cook <keesc...@chromium.org> -Kees -- Kees Cook Pixel Security -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] docs: clarify security-bugs disclosure policy
On Tue, Mar 6, 2018 at 3:31 PM, Dave Hansen <dave.han...@linux.intel.com> wrote: > > From: Dave Hansen <dave.han...@linux.intel.com> > > I think we need to soften the language a bit. It might scare folks > off, especially the: > > We prefer to fully disclose the bug as soon as possible. > > which is not really the case. As Greg mentioned in private mail, we > really do not prefer to disclose things until *after* a fix. The > whole "we have the final say" is also a bit harsh. > [...] > -- > > -The goal of the Linux kernel security team is to work with the > -bug submitter to bug resolution as well as disclosure. We prefer > -to fully disclose the bug as soon as possible. It is reasonable to > -delay disclosure when the bug or the fix is not yet fully understood, > -the solution is not well-tested or for vendor coordination. However, we > -expect these delays to be short, measurable in days, not weeks or months. > +The goal of the Linux kernel security team is to work with the bug > +submitter to bug resolution as well as disclosure. We prefer to fully > +disclose the bug as soon as possible after a fix is available. It is > +customary to delay disclosure when the bug or the fix is not yet fully > +understood, the solution is not well-tested or for vendor coordination. > +However, we expect these delays to typically be short, measurable in > +days, not weeks or months. > + > A disclosure date is negotiated by the security team working with the > -bug submitter as well as vendors. However, the kernel security team > -holds the final say when setting a disclosure date. The timeframe for > -disclosure is from immediate (esp. if it's already publicly known) > -to a few weeks. As a basic default policy, we expect report date to > -disclosure date to be on the order of 7 days. > +bug submitter as well as affected vendors. The security team prefers > +coordinated disclosure and will consider pre-existing, reasonable > +disclosure dates. > + > +The timeframe for disclosure ranges from immediate (esp. if it's > +already publicly known) to a few weeks. As a basic default policy, we > +expect report date to disclosure date to be on the order of 7 days. This seems reasonable. Though I have two thoughts related to "we have final say" and why it was there: Sometimes we get insane embargo requests, and Linus has wanted to go public ASAP. The wording was chosen to let reporters know that it is possible for issues that don't need to wait 7 days to not wait 7 days. For example, letting secur...@kernel.org know about a flaw and then tell us to sit on it for 2 months until some public presentation, that's not going to happen. Additionally, we frequently make all network bugs immediately public, since the net subsystem tends to reject embargoes. So, maybe we could be more explicit about these cases? Or explicitly describe what "reasonable" means (it's only hinted at). -Kees -- Kees Cook Pixel Security -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] doc: process: Add "Root-caused-by" and "Suggested-by"
On Wed, Feb 21, 2018 at 8:43 PM, Randy Dunlap <rdun...@infradead.org> wrote:
> On 02/21/2018 04:37 PM, Kees Cook wrote:
>> As recently pointed out by Linus, "Root-caused-by" is a good tag to include
>> since it can indicate significantly more work than "just" a Reported-by.
>> This adds it and "Suggested-by" (which was also missing) to the documented
>> list of tags. Additionally updates checkpatch.pl to match the process docs.
>>
>> Signed-off-by: Kees Cook <keesc...@chromium.org>
>> ---
>> Documentation/process/5.Posting.rst | 7 +++
>> scripts/checkpatch.pl | 2 ++
>> 2 files changed, 9 insertions(+)
>
> I would still rather see Co-developed-by: in both the docs and in checkpatch.
> :(
Hm? It is in docs. This syncs the process doc to checkpatch...
-Kees
>
>> diff --git a/Documentation/process/5.Posting.rst
>> b/Documentation/process/5.Posting.rst
>> index 645fa9c7388a..2ff01f76f02a 100644
>> --- a/Documentation/process/5.Posting.rst
>> +++ b/Documentation/process/5.Posting.rst
>> @@ -234,6 +234,13 @@ The tags in common use are:
>> people who test our code and let us know when things do not work
>> correctly.
>>
>> + - Suggested-by: names a person who suggested the solution, but may not
>> + have constructed the full patch. A weaker version of `Co-Developed-by`.
>> +
>> + - Root-caused-by: names a person who diagnosed the root cause of a
>> + problem. This usually indicates significantly more work than a simple
>> + `Reported-by`.
>> +
>> - Cc: the named person received a copy of the patch and had the
>> opportunity to comment on it.
>>
>> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
>> index 3d4040322ae1..a1ab82e70b54 100755
>> --- a/scripts/checkpatch.pl
>> +++ b/scripts/checkpatch.pl
>> @@ -464,9 +464,11 @@ our $logFunctions = qr{(?x:
>> our $signature_tags = qr{(?xi:
>> Signed-off-by:|
>> Acked-by:|
>> + Co-Developed-by:|
>> Tested-by:|
>> Reviewed-by:|
>> Reported-by:|
>> + Root-caused-by:|
>> Suggested-by:|
>> To:|
>> Cc:
>>
>
>
> --
> ~Randy
--
Kees Cook
Pixel Security
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] doc: process: Add "Root-caused-by" and "Suggested-by"
On Wed, Feb 21, 2018 at 6:13 PM, Joe Perches <j...@perches.com> wrote:
> On Wed, 2018-02-21 at 16:37 -0800, Kees Cook wrote:
>> As recently pointed out by Linus, "Root-caused-by" is a good tag to include
>> since it can indicate significantly more work than "just" a Reported-by.
>> This adds it and "Suggested-by" (which was also missing) to the documented
>> list of tags. Additionally updates checkpatch.pl to match the process docs.
> []
>> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> []
>> @@ -464,9 +464,11 @@ our $logFunctions = qr{(?x:
>> our $signature_tags = qr{(?xi:
>> Signed-off-by:|
>> Acked-by:|
>> + Co-Developed-by:|
>> Tested-by:|
>> Reviewed-by:|
>> Reported-by:|
>> + Root-caused-by:|
>> Suggested-by:|
>> To:|
>> Cc:
>
> Patch does not match commit description
Hm? Why not? It's updating checkpatch.pl to match the process docs.
checkpatch.pl was missing Co-Developed-by and Root-caused-by, relative
to the process docs.
-Kees
--
Kees Cook
Pixel Security
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] doc: process: Add "Root-caused-by" and "Suggested-by"
As recently pointed out by Linus, "Root-caused-by" is a good tag to include
since it can indicate significantly more work than "just" a Reported-by.
This adds it and "Suggested-by" (which was also missing) to the documented
list of tags. Additionally updates checkpatch.pl to match the process docs.
Signed-off-by: Kees Cook <keesc...@chromium.org>
---
Documentation/process/5.Posting.rst | 7 +++
scripts/checkpatch.pl | 2 ++
2 files changed, 9 insertions(+)
diff --git a/Documentation/process/5.Posting.rst
b/Documentation/process/5.Posting.rst
index 645fa9c7388a..2ff01f76f02a 100644
--- a/Documentation/process/5.Posting.rst
+++ b/Documentation/process/5.Posting.rst
@@ -234,6 +234,13 @@ The tags in common use are:
people who test our code and let us know when things do not work
correctly.
+ - Suggested-by: names a person who suggested the solution, but may not
+ have constructed the full patch. A weaker version of `Co-Developed-by`.
+
+ - Root-caused-by: names a person who diagnosed the root cause of a
+ problem. This usually indicates significantly more work than a simple
+ `Reported-by`.
+
- Cc: the named person received a copy of the patch and had the
opportunity to comment on it.
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 3d4040322ae1..a1ab82e70b54 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -464,9 +464,11 @@ our $logFunctions = qr{(?x:
our $signature_tags = qr{(?xi:
Signed-off-by:|
Acked-by:|
+ Co-Developed-by:|
Tested-by:|
Reviewed-by:|
Reported-by:|
+ Root-caused-by:|
Suggested-by:|
To:|
Cc:
--
2.7.4
--
Kees Cook
Pixel Security
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 0/3] taint: Add taint for randstruct
This cleans up the taint flags and documentation before adding a new one for randstruct. This v2 reverts the #define->enum change as some architectures include TAINT flags in assembly source, which cannot use enums. Patch 3/3 reads: Since the randstruct plugin can intentionally produce extremely unusual kernel structure layouts (even performance pathological ones), some maintainers want to be able to trivially determine if an Oops is coming from a randstruct-built kernel, so as to keep their sanity when debugging. This adds the new flag and initializes taint_mask immediately when built with randstruct. Thanks, -Kees -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 1/3] taint: Convert to indexed initialization
This converts to using indexed initializers instead of comments, adds a
comment on why the taint flags can't be an enum, and make sure that no one
forgets to update the taint_flags when adding new bits.
Signed-off-by: Kees Cook <keesc...@chromium.org>
---
include/linux/kernel.h | 1 +
kernel/panic.c | 36 +++-
2 files changed, 20 insertions(+), 17 deletions(-)
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index ce51455e2adf..62fa060a6e1a 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -533,6 +533,7 @@ extern enum system_states {
SYSTEM_RESTART,
} system_state;
+/* This cannot be an enum because some may be used in assembly source. */
#define TAINT_PROPRIETARY_MODULE 0
#define TAINT_FORCED_MODULE1
#define TAINT_CPU_OUT_OF_SPEC 2
diff --git a/kernel/panic.c b/kernel/panic.c
index 2cfef408fec9..c5e0fd5a188e 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -308,23 +308,23 @@ EXPORT_SYMBOL(panic);
* is being removed anyway.
*/
const struct taint_flag taint_flags[TAINT_FLAGS_COUNT] = {
- { 'P', 'G', true }, /* TAINT_PROPRIETARY_MODULE */
- { 'F', ' ', true }, /* TAINT_FORCED_MODULE */
- { 'S', ' ', false },/* TAINT_CPU_OUT_OF_SPEC */
- { 'R', ' ', false },/* TAINT_FORCED_RMMOD */
- { 'M', ' ', false },/* TAINT_MACHINE_CHECK */
- { 'B', ' ', false },/* TAINT_BAD_PAGE */
- { 'U', ' ', false },/* TAINT_USER */
- { 'D', ' ', false },/* TAINT_DIE */
- { 'A', ' ', false },/* TAINT_OVERRIDDEN_ACPI_TABLE */
- { 'W', ' ', false },/* TAINT_WARN */
- { 'C', ' ', true }, /* TAINT_CRAP */
- { 'I', ' ', false },/* TAINT_FIRMWARE_WORKAROUND */
- { 'O', ' ', true }, /* TAINT_OOT_MODULE */
- { 'E', ' ', true }, /* TAINT_UNSIGNED_MODULE */
- { 'L', ' ', false },/* TAINT_SOFTLOCKUP */
- { 'K', ' ', true }, /* TAINT_LIVEPATCH */
- { 'X', ' ', true }, /* TAINT_AUX */
+ [ TAINT_PROPRIETARY_MODULE ]= { 'P', 'G', true },
+ [ TAINT_FORCED_MODULE ] = { 'F', ' ', true },
+ [ TAINT_CPU_OUT_OF_SPEC ] = { 'S', ' ', false },
+ [ TAINT_FORCED_RMMOD ] = { 'R', ' ', false },
+ [ TAINT_MACHINE_CHECK ] = { 'M', ' ', false },
+ [ TAINT_BAD_PAGE ] = { 'B', ' ', false },
+ [ TAINT_USER ] = { 'U', ' ', false },
+ [ TAINT_DIE ] = { 'D', ' ', false },
+ [ TAINT_OVERRIDDEN_ACPI_TABLE ] = { 'A', ' ', false },
+ [ TAINT_WARN ] = { 'W', ' ', false },
+ [ TAINT_CRAP ] = { 'C', ' ', true },
+ [ TAINT_FIRMWARE_WORKAROUND ] = { 'I', ' ', false },
+ [ TAINT_OOT_MODULE ]= { 'O', ' ', true },
+ [ TAINT_UNSIGNED_MODULE ] = { 'E', ' ', true },
+ [ TAINT_SOFTLOCKUP ]= { 'L', ' ', false },
+ [ TAINT_LIVEPATCH ] = { 'K', ' ', true },
+ [ TAINT_AUX ] = { 'X', ' ', true },
};
/**
@@ -354,6 +354,8 @@ const char *print_tainted(void)
{
static char buf[TAINT_FLAGS_COUNT + sizeof("Tainted: ")];
+ BUILD_BUG_ON(ARRAY_SIZE(taint_flags) != TAINT_FLAGS_COUNT);
+
if (tainted_mask) {
char *s;
int i;
--
2.7.4
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 2/3] taint: Consolidate documentation
This consolidates the taint bit documentation into a single place with
both numeric and letter values. Additionally adds the missing TAINT_AUX
documentation.
Signed-off-by: Kees Cook <keesc...@chromium.org>
---
Documentation/sysctl/kernel.txt | 53 +
kernel/panic.c | 23 --
2 files changed, 31 insertions(+), 45 deletions(-)
diff --git a/Documentation/sysctl/kernel.txt b/Documentation/sysctl/kernel.txt
index 412314eebda6..4a890c7fb6c3 100644
--- a/Documentation/sysctl/kernel.txt
+++ b/Documentation/sysctl/kernel.txt
@@ -964,32 +964,33 @@ detect a hard lockup condition.
tainted:
-Non-zero if the kernel has been tainted. Numeric values, which
-can be ORed together:
-
- 1 - A module with a non-GPL license has been loaded, this
- includes modules with no license.
- Set by modutils >= 2.4.9 and module-init-tools.
- 2 - A module was force loaded by insmod -f.
- Set by modutils >= 2.4.9 and module-init-tools.
- 4 - Unsafe SMP processors: SMP with CPUs not designed for SMP.
- 8 - A module was forcibly unloaded from the system by rmmod -f.
- 16 - A hardware machine check error occurred on the system.
- 32 - A bad page was discovered on the system.
- 64 - The user has asked that the system be marked "tainted". This
- could be because they are running software that directly modifies
- the hardware, or for other reasons.
- 128 - The system has died.
- 256 - The ACPI DSDT has been overridden with one supplied by the user
-instead of using the one provided by the hardware.
- 512 - A kernel warning has occurred.
-1024 - A module from drivers/staging was loaded.
-2048 - The system is working around a severe firmware bug.
-4096 - An out-of-tree module has been loaded.
-8192 - An unsigned module has been loaded in a kernel supporting module
- signature.
-16384 - A soft lockup has previously occurred on the system.
-32768 - The kernel has been live patched.
+Non-zero if the kernel has been tainted. Numeric values, which can be
+ORed together. The letters are seen in "Tainted" line of Oops reports.
+
+ 1 (P): A module with a non-GPL license has been loaded, this
+ includes modules with no license.
+ Set by modutils >= 2.4.9 and module-init-tools.
+ 2 (F): A module was force loaded by insmod -f.
+Set by modutils >= 2.4.9 and module-init-tools.
+ 4 (S): Unsafe SMP processors: SMP with CPUs not designed for SMP.
+ 8 (R): A module was forcibly unloaded from the system by rmmod -f.
+16 (M): A hardware machine check error occurred on the system.
+32 (B): A bad page was discovered on the system.
+64 (U): The user has asked that the system be marked "tainted". This
+could be because they are running software that directly modifies
+the hardware, or for other reasons.
+ 128 (D): The system has died.
+ 256 (A): The ACPI DSDT has been overridden with one supplied by the user
+instead of using the one provided by the hardware.
+ 512 (W): A kernel warning has occurred.
+ 1024 (C): A module from drivers/staging was loaded.
+ 2048 (I): The system is working around a severe firmware bug.
+ 4096 (O): An out-of-tree module has been loaded.
+ 8192 (E): An unsigned module has been loaded in a kernel supporting module
+signature.
+ 16384 (L): A soft lockup has previously occurred on the system.
+ 32768 (K): The kernel has been live patched.
+ 65536 (X): Auxiliary taint, defined and used by for distros.
==
diff --git a/kernel/panic.c b/kernel/panic.c
index c5e0fd5a188e..15d333a54ece 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -328,27 +328,12 @@ const struct taint_flag taint_flags[TAINT_FLAGS_COUNT] = {
};
/**
- * print_tainted - return a string to represent the kernel taint state.
+ * print_tainted - return a string to represent the kernel taint state.
*
- * 'P' - Proprietary module has been loaded.
- * 'F' - Module has been forcibly loaded.
- * 'S' - SMP with CPUs not designed for SMP.
- * 'R' - User forced a module unload.
- * 'M' - System experienced a machine check exception.
- * 'B' - System has hit bad_page.
- * 'U' - Userspace-defined naughtiness.
- * 'D' - Kernel has oopsed before
- * 'A' - ACPI table overridden.
- * 'W' - Taint on warning.
- * 'C' - modules from drivers/staging are loaded.
- * 'I' - Working around severe firmware bug.
- * 'O' - Out-of-tree module has been loaded.
- * 'E' - Unsigned module has been loaded.
- * 'L' - A soft lockup has previously occurred.
- * 'K' - Kernel has been live patched.
- * 'X' - Auxiliary taint, for distros' use.
+ * For individual taint flag meanings, see Documentation/sysctl/kernel.txt
*
- * The string is overwritten by the next call to print_tainted().
+ * The string is overwritten by the
[PATCH v2 3/3] taint: Add taint for randstruct
Since the randstruct plugin can intentionally produce extremely unusual
kernel structure layouts (even performance pathological ones), some
maintainers want to be able to trivially determine if an Oops is coming
from a randstruct-built kernel, so as to keep their sanity when debugging.
This adds the new flag and initializes taint_mask immediately when built
with randstruct.
Signed-off-by: Kees Cook <keesc...@chromium.org>
---
Documentation/sysctl/kernel.txt | 1 +
include/linux/kernel.h | 3 ++-
kernel/panic.c | 4 +++-
3 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/Documentation/sysctl/kernel.txt b/Documentation/sysctl/kernel.txt
index 4a890c7fb6c3..eded671d55eb 100644
--- a/Documentation/sysctl/kernel.txt
+++ b/Documentation/sysctl/kernel.txt
@@ -991,6 +991,7 @@ ORed together. The letters are seen in "Tainted" line of
Oops reports.
16384 (L): A soft lockup has previously occurred on the system.
32768 (K): The kernel has been live patched.
65536 (X): Auxiliary taint, defined and used by for distros.
+131072 (T): The kernel was built with the struct randomization plugin.
==
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 62fa060a6e1a..ce1e43bf5f49 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -551,7 +551,8 @@ extern enum system_states {
#define TAINT_SOFTLOCKUP 14
#define TAINT_LIVEPATCH15
#define TAINT_AUX 16
-#define TAINT_FLAGS_COUNT 17
+#define TAINT_RANDSTRUCT 17
+#define TAINT_FLAGS_COUNT 18
struct taint_flag {
char c_true;/* character printed when tainted */
diff --git a/kernel/panic.c b/kernel/panic.c
index 15d333a54ece..0153cae0d330 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -34,7 +34,8 @@
#define PANIC_BLINK_SPD 18
int panic_on_oops = CONFIG_PANIC_ON_OOPS_VALUE;
-static unsigned long tainted_mask;
+static unsigned long tainted_mask =
+ IS_ENABLED(CONFIG_GCC_PLUGIN_RANDSTRUCT) ? (1 << TAINT_RANDSTRUCT) : 0;
static int pause_on_oops;
static int pause_on_oops_flag;
static DEFINE_SPINLOCK(pause_on_oops_lock);
@@ -325,6 +326,7 @@ const struct taint_flag taint_flags[TAINT_FLAGS_COUNT] = {
[ TAINT_SOFTLOCKUP ]= { 'L', ' ', false },
[ TAINT_LIVEPATCH ] = { 'K', ' ', true },
[ TAINT_AUX ] = { 'X', ' ', true },
+ [ TAINT_RANDSTRUCT ]= { 'T', ' ', true },
};
/**
--
2.7.4
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] taint: Convert to enum and indexed initialization
On Sat, Feb 17, 2018 at 9:03 PM, kbuild test robot <l...@intel.com> wrote: > Hi Kees, > > I love your patch! Yet something to improve: > > [auto build test ERROR on linus/master] > [also build test ERROR on v4.16-rc1 next-20180216] > [if your patch is applied to the wrong git tree, please drop us a note to > help improve the system] > > url: > https://github.com/0day-ci/linux/commits/Kees-Cook/taint-Add-taint-for-randstruct/20180218-100113 > config: arm64-defconfig (attached as .config) > compiler: aarch64-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0 > reproduce: > wget > https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O > ~/bin/make.cross > chmod +x ~/bin/make.cross > # save the attached .config to linux build tree > make.cross ARCH=arm64 > > All errors (new ones prefixed by >>): > >/tmp/ccUwVEny.s: Assembler messages: >>> /tmp/ccUwVEny.s:897: Error: invalid operands (*UND* and *ABS* sections) for >>> `<<' >/tmp/ccUwVEny.s:932: Error: invalid operands (*UND* and *ABS* sections) > for `<<' >/tmp/ccUwVEny.s:1116: Error: invalid operands (*UND* and *ABS* sections) > for `<<' Ugh, it looks like an enum can't be used here because of assembly files? I'll try to figure this out... -Kees > > --- > 0-DAY kernel test infrastructure Open Source Technology Center > https://lists.01.org/pipermail/kbuild-all Intel Corporation -- Kees Cook Pixel Security -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] taint: Add taint for randstruct
On Fri, Feb 16, 2018 at 1:02 PM, Andrew Morton <a...@linux-foundation.org> wrote: > On Thu, 15 Feb 2018 19:37:44 -0800 Kees Cook <keesc...@chromium.org> wrote: > >> --- a/Documentation/sysctl/kernel.txt >> +++ b/Documentation/sysctl/kernel.txt >> @@ -991,6 +991,7 @@ ORed together. The letters are seen in "Tainted" line of >> Oops reports. >> 16384 (L): A soft lockup has previously occurred on the system. >> 32768 (K): The kernel has been live patched. >> 65536 (X): Auxiliary taint, defined and used by for distros. >> +131072 (T): The kernel was built with the struct randomization plugin. > > Uncle. > > > From: Andrew Morton <a...@linux-foundation.org> > Subject: Documentation/sysctl/kernel.txt: show taint codes in hex > > The decimal representation is getting a bit hard to follow. The rationale, AIUI, is that /proc/sys/kernel/tainted prints the values in decimal. If we change the docs to be hex and leave the output decimal, that makes it even harder to examine. If we change the proc output, will we break userspace? And if we change it, maybe avoid numbers at all, and proc should bring the same thing that Oops does (the letter codes)? (But then the sysctl would need to parse the letters...) -Kees -- Kees Cook Pixel Security -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/3] taint: Add taint for randstruct
This cleans up the taint flags and documentation before adding a new one for randstruct. Patch 3/3 reads: Since the randstruct plugin can intentionally produce extremely unusual kernel structure layouts (even performance pathological ones), some maintainers want to be able to trivially determine if an Oops is coming from a randstruct-built kernel, so as to keep their sanity when debugging. This adds the new flag and initializes taint_mask immediately when built with randstruct. Thanks, -Kees -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 3/3] taint: Add taint for randstruct
Since the randstruct plugin can intentionally produce extremely unusual
kernel structure layouts (even performance pathological ones), some
maintainers want to be able to trivially determine if an Oops is coming
from a randstruct-built kernel, so as to keep their sanity when debugging.
This adds the new flag and initializes taint_mask immediately when built
with randstruct.
Signed-off-by: Kees Cook <keesc...@chromium.org>
---
Documentation/sysctl/kernel.txt | 1 +
include/linux/kernel.h | 1 +
kernel/panic.c | 4 +++-
3 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/Documentation/sysctl/kernel.txt b/Documentation/sysctl/kernel.txt
index 4a890c7fb6c3..eded671d55eb 100644
--- a/Documentation/sysctl/kernel.txt
+++ b/Documentation/sysctl/kernel.txt
@@ -991,6 +991,7 @@ ORed together. The letters are seen in "Tainted" line of
Oops reports.
16384 (L): A soft lockup has previously occurred on the system.
32768 (K): The kernel has been live patched.
65536 (X): Auxiliary taint, defined and used by for distros.
+131072 (T): The kernel was built with the struct randomization plugin.
==
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 0d2a2dd507b7..9e93ab8358d0 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -551,6 +551,7 @@ enum taint_enum {
TAINT_SOFTLOCKUP,
TAINT_LIVEPATCH,
TAINT_AUX,
+ TAINT_RANDSTRUCT,
/* End of taint bits */
TAINT_FLAGS_COUNT
diff --git a/kernel/panic.c b/kernel/panic.c
index 15d333a54ece..0153cae0d330 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -34,7 +34,8 @@
#define PANIC_BLINK_SPD 18
int panic_on_oops = CONFIG_PANIC_ON_OOPS_VALUE;
-static unsigned long tainted_mask;
+static unsigned long tainted_mask =
+ IS_ENABLED(CONFIG_GCC_PLUGIN_RANDSTRUCT) ? (1 << TAINT_RANDSTRUCT) : 0;
static int pause_on_oops;
static int pause_on_oops_flag;
static DEFINE_SPINLOCK(pause_on_oops_lock);
@@ -325,6 +326,7 @@ const struct taint_flag taint_flags[TAINT_FLAGS_COUNT] = {
[ TAINT_SOFTLOCKUP ]= { 'L', ' ', false },
[ TAINT_LIVEPATCH ] = { 'K', ' ', true },
[ TAINT_AUX ] = { 'X', ' ', true },
+ [ TAINT_RANDSTRUCT ]= { 'T', ' ', true },
};
/**
--
2.7.4
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/3] taint: Consolidate documentation
This consolidates the taint bit documentation into a single place with
both numeric and letter values. Additionally adds the missing TAINT_AUX
documentation.
Signed-off-by: Kees Cook <keesc...@chromium.org>
---
Documentation/sysctl/kernel.txt | 53 +
kernel/panic.c | 23 --
2 files changed, 31 insertions(+), 45 deletions(-)
diff --git a/Documentation/sysctl/kernel.txt b/Documentation/sysctl/kernel.txt
index 412314eebda6..4a890c7fb6c3 100644
--- a/Documentation/sysctl/kernel.txt
+++ b/Documentation/sysctl/kernel.txt
@@ -964,32 +964,33 @@ detect a hard lockup condition.
tainted:
-Non-zero if the kernel has been tainted. Numeric values, which
-can be ORed together:
-
- 1 - A module with a non-GPL license has been loaded, this
- includes modules with no license.
- Set by modutils >= 2.4.9 and module-init-tools.
- 2 - A module was force loaded by insmod -f.
- Set by modutils >= 2.4.9 and module-init-tools.
- 4 - Unsafe SMP processors: SMP with CPUs not designed for SMP.
- 8 - A module was forcibly unloaded from the system by rmmod -f.
- 16 - A hardware machine check error occurred on the system.
- 32 - A bad page was discovered on the system.
- 64 - The user has asked that the system be marked "tainted". This
- could be because they are running software that directly modifies
- the hardware, or for other reasons.
- 128 - The system has died.
- 256 - The ACPI DSDT has been overridden with one supplied by the user
-instead of using the one provided by the hardware.
- 512 - A kernel warning has occurred.
-1024 - A module from drivers/staging was loaded.
-2048 - The system is working around a severe firmware bug.
-4096 - An out-of-tree module has been loaded.
-8192 - An unsigned module has been loaded in a kernel supporting module
- signature.
-16384 - A soft lockup has previously occurred on the system.
-32768 - The kernel has been live patched.
+Non-zero if the kernel has been tainted. Numeric values, which can be
+ORed together. The letters are seen in "Tainted" line of Oops reports.
+
+ 1 (P): A module with a non-GPL license has been loaded, this
+ includes modules with no license.
+ Set by modutils >= 2.4.9 and module-init-tools.
+ 2 (F): A module was force loaded by insmod -f.
+Set by modutils >= 2.4.9 and module-init-tools.
+ 4 (S): Unsafe SMP processors: SMP with CPUs not designed for SMP.
+ 8 (R): A module was forcibly unloaded from the system by rmmod -f.
+16 (M): A hardware machine check error occurred on the system.
+32 (B): A bad page was discovered on the system.
+64 (U): The user has asked that the system be marked "tainted". This
+could be because they are running software that directly modifies
+the hardware, or for other reasons.
+ 128 (D): The system has died.
+ 256 (A): The ACPI DSDT has been overridden with one supplied by the user
+instead of using the one provided by the hardware.
+ 512 (W): A kernel warning has occurred.
+ 1024 (C): A module from drivers/staging was loaded.
+ 2048 (I): The system is working around a severe firmware bug.
+ 4096 (O): An out-of-tree module has been loaded.
+ 8192 (E): An unsigned module has been loaded in a kernel supporting module
+signature.
+ 16384 (L): A soft lockup has previously occurred on the system.
+ 32768 (K): The kernel has been live patched.
+ 65536 (X): Auxiliary taint, defined and used by for distros.
==
diff --git a/kernel/panic.c b/kernel/panic.c
index c5e0fd5a188e..15d333a54ece 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -328,27 +328,12 @@ const struct taint_flag taint_flags[TAINT_FLAGS_COUNT] = {
};
/**
- * print_tainted - return a string to represent the kernel taint state.
+ * print_tainted - return a string to represent the kernel taint state.
*
- * 'P' - Proprietary module has been loaded.
- * 'F' - Module has been forcibly loaded.
- * 'S' - SMP with CPUs not designed for SMP.
- * 'R' - User forced a module unload.
- * 'M' - System experienced a machine check exception.
- * 'B' - System has hit bad_page.
- * 'U' - Userspace-defined naughtiness.
- * 'D' - Kernel has oopsed before
- * 'A' - ACPI table overridden.
- * 'W' - Taint on warning.
- * 'C' - modules from drivers/staging are loaded.
- * 'I' - Working around severe firmware bug.
- * 'O' - Out-of-tree module has been loaded.
- * 'E' - Unsigned module has been loaded.
- * 'L' - A soft lockup has previously occurred.
- * 'K' - Kernel has been live patched.
- * 'X' - Auxiliary taint, for distros' use.
+ * For individual taint flag meanings, see Documentation/sysctl/kernel.txt
*
- * The string is overwritten by the next call to print_tainted().
+ * The string is overwritten by the
[PATCH 1/3] taint: Convert to enum and indexed initialization
This converts the taint bit defines to an enum, uses indexed initializers
instead of comments, and make sure that no one forgets to update the
taint_flags when adding new bits.
Signed-off-by: Kees Cook <keesc...@chromium.org>
---
include/linux/kernel.h | 40 ++--
kernel/panic.c | 36 +++-
2 files changed, 41 insertions(+), 35 deletions(-)
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index ce51455e2adf..0d2a2dd507b7 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -533,24 +533,28 @@ extern enum system_states {
SYSTEM_RESTART,
} system_state;
-#define TAINT_PROPRIETARY_MODULE 0
-#define TAINT_FORCED_MODULE1
-#define TAINT_CPU_OUT_OF_SPEC 2
-#define TAINT_FORCED_RMMOD 3
-#define TAINT_MACHINE_CHECK4
-#define TAINT_BAD_PAGE 5
-#define TAINT_USER 6
-#define TAINT_DIE 7
-#define TAINT_OVERRIDDEN_ACPI_TABLE8
-#define TAINT_WARN 9
-#define TAINT_CRAP 10
-#define TAINT_FIRMWARE_WORKAROUND 11
-#define TAINT_OOT_MODULE 12
-#define TAINT_UNSIGNED_MODULE 13
-#define TAINT_SOFTLOCKUP 14
-#define TAINT_LIVEPATCH15
-#define TAINT_AUX 16
-#define TAINT_FLAGS_COUNT 17
+enum taint_enum {
+ TAINT_PROPRIETARY_MODULE = 0,
+ TAINT_FORCED_MODULE,
+ TAINT_CPU_OUT_OF_SPEC,
+ TAINT_FORCED_RMMOD,
+ TAINT_MACHINE_CHECK,
+ TAINT_BAD_PAGE,
+ TAINT_USER,
+ TAINT_DIE,
+ TAINT_OVERRIDDEN_ACPI_TABLE,
+ TAINT_WARN,
+ TAINT_CRAP,
+ TAINT_FIRMWARE_WORKAROUND,
+ TAINT_OOT_MODULE,
+ TAINT_UNSIGNED_MODULE,
+ TAINT_SOFTLOCKUP,
+ TAINT_LIVEPATCH,
+ TAINT_AUX,
+
+ /* End of taint bits */
+ TAINT_FLAGS_COUNT
+};
struct taint_flag {
char c_true;/* character printed when tainted */
diff --git a/kernel/panic.c b/kernel/panic.c
index 2cfef408fec9..c5e0fd5a188e 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -308,23 +308,23 @@ EXPORT_SYMBOL(panic);
* is being removed anyway.
*/
const struct taint_flag taint_flags[TAINT_FLAGS_COUNT] = {
- { 'P', 'G', true }, /* TAINT_PROPRIETARY_MODULE */
- { 'F', ' ', true }, /* TAINT_FORCED_MODULE */
- { 'S', ' ', false },/* TAINT_CPU_OUT_OF_SPEC */
- { 'R', ' ', false },/* TAINT_FORCED_RMMOD */
- { 'M', ' ', false },/* TAINT_MACHINE_CHECK */
- { 'B', ' ', false },/* TAINT_BAD_PAGE */
- { 'U', ' ', false },/* TAINT_USER */
- { 'D', ' ', false },/* TAINT_DIE */
- { 'A', ' ', false },/* TAINT_OVERRIDDEN_ACPI_TABLE */
- { 'W', ' ', false },/* TAINT_WARN */
- { 'C', ' ', true }, /* TAINT_CRAP */
- { 'I', ' ', false },/* TAINT_FIRMWARE_WORKAROUND */
- { 'O', ' ', true }, /* TAINT_OOT_MODULE */
- { 'E', ' ', true }, /* TAINT_UNSIGNED_MODULE */
- { 'L', ' ', false },/* TAINT_SOFTLOCKUP */
- { 'K', ' ', true }, /* TAINT_LIVEPATCH */
- { 'X', ' ', true }, /* TAINT_AUX */
+ [ TAINT_PROPRIETARY_MODULE ]= { 'P', 'G', true },
+ [ TAINT_FORCED_MODULE ] = { 'F', ' ', true },
+ [ TAINT_CPU_OUT_OF_SPEC ] = { 'S', ' ', false },
+ [ TAINT_FORCED_RMMOD ] = { 'R', ' ', false },
+ [ TAINT_MACHINE_CHECK ] = { 'M', ' ', false },
+ [ TAINT_BAD_PAGE ] = { 'B', ' ', false },
+ [ TAINT_USER ] = { 'U', ' ', false },
+ [ TAINT_DIE ] = { 'D', ' ', false },
+ [ TAINT_OVERRIDDEN_ACPI_TABLE ] = { 'A', ' ', false },
+ [ TAINT_WARN ] = { 'W', ' ', false },
+ [ TAINT_CRAP ] = { 'C', ' ', true },
+ [ TAINT_FIRMWARE_WORKAROUND ] = { 'I', ' ', false },
+ [ TAINT_OOT_MODULE ]= { 'O', ' ', true },
+ [ TAINT_UNSIGNED_MODULE ] = { 'E', ' ', true },
+ [ TAINT_SOFTLOCKUP ]= { 'L', ' ', false },
+ [ TAINT_LIVEPATCH ] = { 'K', ' ', true },
+ [ TAINT_AUX ] = { 'X', ' ', true },
};
/**
@@ -354,6 +354,8 @@ const char *print_tainted(void)
{
static char buf[TAINT_FLAGS_COUNT + sizeof("Tainted: ")];
+ BUILD_BUG_ON(ARRAY_SIZE(taint_flags) != TAINT_FLAGS_COUNT);
+
if (tainted_mask) {
char *s;
int i;
--
2.7.4
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Documentation/process: add Co-Developed-by: tag for patches with multiple authors
On Thu, Nov 16, 2017 at 5:23 AM, Greg Kroah-Hartman <gre...@linuxfoundation.org> wrote: > Sometimes a single patch is the result of multiple authors. As git only > can have one "author" of a patch, it is still good to properly give > credit to the other developers of a commit. To address this, document > the "Co-Developed-by:" tag which can be used to show other authors of > the patch. > > Note, these other authors must also provide a Signed-off-by: tag as it > is their work that is being submitted here. > > Reported-by: Thomas Gleixner <t...@linutronix.de> > Signed-off-by: Greg Kroah-Hartman <gre...@linuxfoundation.org> I see no uses of this tag yet, and I'd like to get it right. If patch v1 was written by author A, then heavily modified and sent by author B to produce patch v2, what should the resulting states of git-author, and tag order be? I'm assuming it should be: git-author: B ... Signed-off-by: A Co-Developed-by: A Signed-off-by: B It's not clear to me if git-author should instead be A, and/or Co-Developed-by should be B... Thanks! -Kees -- Kees Cook Pixel Security -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] docs: refcount_t documentation
On Mon, Dec 11, 2017 at 1:42 PM, Jonathan Corbet <cor...@lwn.net> wrote: > On Tue, 5 Dec 2017 12:46:35 +0200 > Elena Reshetova <elena.reshet...@intel.com> wrote: > >> Some functions from refcount_t API provide different >> memory ordering guarantees that their atomic counterparts. >> This adds a document outlining these differences ( >> Documentation/core-api/refcount-vs-atomic.rst) as well as >> some other minor improvements. > > I've applied this, thanks, it looks good. > > One thing I noticed, though, is that this landed in the core-api manual, > while the refcount_t documentation is in the driver-api manual. The > cross-references work just fine and such, but this still doesn't seem quite > right. Probably what should be done is to have all the refcount_t > material in core-api; I may do that if I get a moment. I did notice that, yeah. It seemed like a bunch of kernel-doc was living in the driver-api manual, where it should be in core. Since atomics were already there, I put refcount_t there... -Kees -- Kees Cook Pixel Security -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] doc: convert printk-formats.txt to rst
On Thu, Dec 7, 2017 at 4:46 PM, Tobin C. Harding <m...@tobin.cc> wrote: > On Thu, Dec 07, 2017 at 04:19:56PM -0800, Kees Cook wrote: >> On Thu, Dec 7, 2017 at 3:44 PM, Tobin C. Harding <m...@tobin.cc> wrote: >> > Cheers Kees. FTR, changes to implement are: >> > >> > - Fix the capitalization of 'kernel'. >> >> I don't really have an opinion about which way is right. I personally >> don't capitalize it unless I speak about it as a single thing "The >> Linux Kernel". In this case I just noticed you had mixed usage. >> >> > - Add ESCAPE_* flags back into kernel-docs in lib/vsprintf.c >> >> I actually meant each of the sections. Several of the formats have >> per-item breakdowns that went missing in the new kernel-doc (ESCAPE_* >> was just an example). > > Oh dear, you don't like that. This is actually the part of the patch > that I was least sure about doing. I'm happy to revert, can I give you > my thought process for comment? > > When the kernel-docs get included into printk-formats.rst it seems > overly verbose to have all the information given twice. And then it > seems odd to bother having the extra descriptions in printk-formats.rst > if _all_ the required information is already in the kernel-docs? > > So I guessed that it would be nice for devs to get a bit of a hint at > the specifiers when having lib/vsprintf.c open (and they have the code > too) then if they needed more information going to printk-formats.rst. > > Also, since there is more space in printk-fomats.rst the info can be > spaced better and easier to read. > > Your thoughts? Well ... my sense is that lib/vsprintf.c should remain the canonical documentation. Anyone working on the code has the docs all together in one file. If it helps the .rst file to reformat the comments into kernel-doc, that's fine, but it shouldn't reduce the detail that is present, IMO. Now, expanding on it in printk-formats.rst is certainly a great idea, but I don't think it should come at the expense of someone just reading through vsprintf.c. That said, I can certainly see that redundancy is annoying, and it's possible for printk-formats.rst and vsprintf.c get get out of sync, but that doesn't seem to be a new problem. I'd be curious to see what Jon or Joe think about this. (Perhaps the best first step would be to leave vsprintf.c as-is without kernel-doc-ification?) -Kees -- Kees Cook Pixel Security -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] doc: convert printk-formats.txt to rst
On Thu, Dec 7, 2017 at 3:44 PM, Tobin C. Harding <m...@tobin.cc> wrote: > Cheers Kees. FTR, changes to implement are: > > - Fix the capitalization of 'kernel'. I don't really have an opinion about which way is right. I personally don't capitalize it unless I speak about it as a single thing "The Linux Kernel". In this case I just noticed you had mixed usage. > - Add ESCAPE_* flags back into kernel-docs in lib/vsprintf.c I actually meant each of the sections. Several of the formats have per-item breakdowns that went missing in the new kernel-doc (ESCAPE_* was just an example). -Kees -- Kees Cook Pixel Security -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] doc: convert printk-formats.txt to rst
gt; - * - '[Ii][4S][hnbl]' IPv4 addresses in host, network, big or little endian
> order
> - * - 'I[6S]c' for IPv6 addresses printed as specified by
> - * http://tools.ietf.org/html/rfc5952
> - * - 'E[achnops]' For an escaped buffer, where rules are defined by
> combination
> - *of the following flags (see string_escape_mem() for the
> - *details):
> - * a - ESCAPE_ANY
> - * c - ESCAPE_SPECIAL
> - * h - ESCAPE_HEX
> - * n - ESCAPE_NULL
> - * o - ESCAPE_OCTAL
> - * p - ESCAPE_NP
> - * s - ESCAPE_SPACE
> - *By default ESCAPE_ANY_NP is used.
Is there no way to retain these per-flag details in the kernel-doc?
Seems a shame to split up the docs if we can keep it together in here.
> - * - 'U' For a 16 byte UUID/GUID, it prints the UUID/GUID in the form
> - * "----"
> - * Options for %pU are:
> - * b big endian lower case hex (default)
> - * B big endian UPPER case hex
> - * l little endian lower case hex
> - * L little endian UPPER case hex
> - * big endian output byte order is:
> - * [0][1][2][3]-[4][5]-[6][7]-[8][9]-[10][11][12][13][14][15]
> - * little endian output byte order is:
> - * [3][2][1][0]-[5][4]-[7][6]-[8][9]-[10][11][12][13][14][15]
> - * - 'V' For a struct va_format which contains a format string * and va_list
> *,
> - * call vsnprintf(->format, *->va_list).
> - * Implements a "recursive vsnprintf".
> - * Do not use this feature without some mechanism to verify the
> - * correctness of the format string and va_list arguments.
> - * - 'K' For a kernel pointer that should be hidden from unprivileged users
> - * - 'NF' For a netdev_features_t
> - * - 'h[CDN]' For a variable-length buffer, it prints it as a hex string with
> - *a certain separator (' ' by default):
> - * C colon
> - * D dash
> - * N no separator
> - *The maximum supported length is 64 bytes of the input. Consider
> - *to use print_hex_dump() for the larger input.
> - * - 'a[pd]' For address types [p] phys_addr_t, [d] dma_addr_t and
> derivatives
> - * (default assumed to be phys_addr_t, passed by reference)
> - * - 'd[234]' For a dentry name (optionally 2-4 last components)
> - * - 'D[234]' Same as 'd' but for a struct file
> - * - 'g' For block_device name (gendisk + partition number)
> - * - 'C' For a clock, it prints the name (Common Clock Framework) or address
> - * (legacy clock framework) of the clock
> - * - 'Cn' For a clock, it prints the name (Common Clock Framework) or address
> - *(legacy clock framework) of the clock
> - * - 'Cr' For a clock, it prints the current rate of the clock
> - * - 'G' For flags to be printed as a collection of symbolic strings that
> would
> - * construct the specific value. Supported flags given by option:
> - * p page flags (see struct page) given as pointer to unsigned long
> - * g gfp flags (GFP_* and __GFP_*) given as pointer to gfp_t
> - * v vma flags (VM_*) given as pointer to unsigned long
> - * - 'O' For a kobject based struct. Must be one of the following:
> - * - 'OF[fnpPcCF]' For a device tree object
> - * Without any optional arguments prints the full_name
> - *f device node full_name
> - *n device node name
> - *p device node phandle
> - *P device node path spec (name + @unit)
> - *F device node flags
> - *c major compatible string
> - *C full compatible string
> - *
> - * - 'x' For printing the address. Equivalent to "%lx".
> - *
> - * ** Please update also Documentation/printk-formats.txt when making
> changes **
> - *
> * Note: The difference between 'S' and 'F' is that on ia64 and ppc64
> * function pointers are really function descriptors, which contain a
> * pointer to the real address.
> --
> 2.7.4
>
Nice work!
-Kees
--
Kees Cook
Pixel Security
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] docs: refcount_t documentation
On Thu, Dec 7, 2017 at 9:58 AM, Jonathan Corbet <cor...@lwn.net> wrote: > On Tue, 5 Dec 2017 12:46:35 +0200 > Elena Reshetova <elena.reshet...@intel.com> wrote: > >> Some functions from refcount_t API provide different >> memory ordering guarantees that their atomic counterparts. >> This adds a document outlining these differences ( >> Documentation/core-api/refcount-vs-atomic.rst) as well as >> some other minor improvements. > > This seems generally good to me. Did you want me to take it through the > docs tree (including the refcount.h change) or did you have some other > path in mind? FWIW, I had assumed this would go via the docs tree. -Kees -- Kees Cook Pixel Security -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] docs: add documentation on printing kernel addresses
On Wed, Dec 6, 2017 at 4:26 PM, Tobin C. Harding <m...@tobin.cc> wrote: > Hashing addresses printed with printk specifier %p was implemented > recently. During development a number of issues were raised regarding > leaking kernel addresses to userspace. We should update the > documentation appropriately. > > Add documentation regarding printing kernel addresses. > > Signed-off-by: Tobin C. Harding <m...@tobin.cc> Acked-by: Kees Cook <keesc...@chromium.org> > --- > > Is there a proffered method for subscripts in sphinx kernel docs? Here > we use '[*]' Great question... I can't find an answer to this. :P > > thanks, > Tobin. > > Documentation/security/self-protection.rst | 14 ++ > 1 file changed, 14 insertions(+) > > diff --git a/Documentation/security/self-protection.rst > b/Documentation/security/self-protection.rst > index 60c8bd8b77bf..e711280cfdd7 100644 > --- a/Documentation/security/self-protection.rst > +++ b/Documentation/security/self-protection.rst > @@ -270,6 +270,20 @@ attacks, it is important to defend against exposure of > both kernel memory > addresses and kernel memory contents (since they may contain kernel > addresses or other sensitive things like canary values). > > +Kernel addresses > + > + > +Printing kernel addresses to userspace leaks sensitive information about > +the kernel memory layout. Care should be exercised when using any printk > +specifier that prints the raw address, currently %px, %p[ad], (and %p[sSb] > +in certain circumstances [*]). Any file written to using one of these > +specifiers should be readable only by privileged processes. > + > +Kernels 4.14 and older printed the raw address using %p. As of 4.15-rc1 > +addresses printed with the specifier %p are hashed before printing. > + > +[*] If symbol lookup fails, the raw address is currently printed. Is there a plan to adjust this case? Thanks! -Kees > + > Unique identifiers > -- > > -- > 2.7.4 > -- Kees Cook Pixel Security -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] doc: update 'unique identifiers'
On Mon, Dec 4, 2017 at 3:39 PM, Tobin C. Harding <m...@tobin.cc> wrote: > On Mon, Dec 04, 2017 at 01:51:42PM -0800, Kees Cook wrote: >> On Mon, Dec 4, 2017 at 1:44 PM, Tobin C. Harding <m...@tobin.cc> wrote: >> > On Mon, Dec 04, 2017 at 01:28:45PM -0800, Kees Cook wrote: >> >> On Mon, Dec 4, 2017 at 1:22 PM, Tobin C. Harding <m...@tobin.cc> wrote: >> >> > Advice about what to use as a unique identifier is no longer valid since >> >> > patch series was merged to hash pointers printed with %p. We can use >> >> > this as a unique identifier now. >> >> > >> >> > Signed-off-by: Tobin C. Harding <m...@tobin.cc> >> >> >> >> I don't agree: %p should still not be encouraged. Exposing an >> >> identifier to userspace needs careful consideration, and atomics, >> >> idrs, etc, continue to be a good recommendation here, as far as I'm >> >> concerned. >> > >> > Ok no worries, so these docs are valid and current as is? I have no >> > agenda with this patch, just attempting to keep the docs in line with >> > the code :) >> >> I think a section could be added/updated discussing leaks and %p (in >> that it is hashing now), that would be quite welcome! >> >> I do, probably need to go through this document and update a few things. > > How about I do whatever generates the least amount of work for you. Is > it easier if I add the %p stuff for you to review or is it easier to > just leave it for you to do in your own time? If you can write a section on %p leaks, that would be great! I can clean up other things as work on top of that. Thanks! -Kees > > thanks, > Tobin. -- Kees Cook Pixel Security -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] doc: update 'unique identifiers'
On Mon, Dec 4, 2017 at 1:44 PM, Tobin C. Harding <m...@tobin.cc> wrote: > On Mon, Dec 04, 2017 at 01:28:45PM -0800, Kees Cook wrote: >> On Mon, Dec 4, 2017 at 1:22 PM, Tobin C. Harding <m...@tobin.cc> wrote: >> > Advice about what to use as a unique identifier is no longer valid since >> > patch series was merged to hash pointers printed with %p. We can use >> > this as a unique identifier now. >> > >> > Signed-off-by: Tobin C. Harding <m...@tobin.cc> >> >> I don't agree: %p should still not be encouraged. Exposing an >> identifier to userspace needs careful consideration, and atomics, >> idrs, etc, continue to be a good recommendation here, as far as I'm >> concerned. > > Ok no worries, so these docs are valid and current as is? I have no > agenda with this patch, just attempting to keep the docs in line with > the code :) I think a section could be added/updated discussing leaks and %p (in that it is hashing now), that would be quite welcome! I do, probably need to go through this document and update a few things. -Kees -- Kees Cook Pixel Security -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] doc: update 'unique identifiers'
On Mon, Dec 4, 2017 at 1:22 PM, Tobin C. Harding <m...@tobin.cc> wrote: > Advice about what to use as a unique identifier is no longer valid since > patch series was merged to hash pointers printed with %p. We can use > this as a unique identifier now. > > Signed-off-by: Tobin C. Harding <m...@tobin.cc> I don't agree: %p should still not be encouraged. Exposing an identifier to userspace needs careful consideration, and atomics, idrs, etc, continue to be a good recommendation here, as far as I'm concerned. -Kees > --- > Documentation/security/self-protection.rst | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/Documentation/security/self-protection.rst > b/Documentation/security/self-protection.rst > index 60c8bd8b77bf..f10f47cad825 100644 > --- a/Documentation/security/self-protection.rst > +++ b/Documentation/security/self-protection.rst > @@ -274,8 +274,8 @@ Unique identifiers > -- > > Kernel memory addresses must never be used as identifiers exposed to > -userspace. Instead, use an atomic counter, an idr, or similar unique > -identifier. > +userspace. Printk specifier %p hashes addresses by default now and can be > +used as a unique identifier. > > Memory initialization > - > -- > 2.7.4 > -- Kees Cook Pixel Security -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] docs: Expand refcount_t documentation
This updates basics.rst to include refcount_t so it can be referenced by other .rst files, fixes a kernel-doc typo in refcount.h so the struct will be documented, and enhances the markup of the refcount-vs-atomic doc. Signed-off-by: Kees Cook <keesc...@chromium.org> --- This builds on the "refcount-vs-atomic.rst" patch... Elena, feel free to fold this into your refcount_t doc. Future revisions should likely get sent to Jon Corbet and CC linux-doc. --- Documentation/core-api/refcount-vs-atomic.rst | 84 --- Documentation/driver-api/basics.rst | 21 +-- include/linux/refcount.h | 2 +- 3 files changed, 67 insertions(+), 40 deletions(-) diff --git a/Documentation/core-api/refcount-vs-atomic.rst b/Documentation/core-api/refcount-vs-atomic.rst index 5619d486e70e..315d5f882331 100644 --- a/Documentation/core-api/refcount-vs-atomic.rst +++ b/Documentation/core-api/refcount-vs-atomic.rst @@ -2,11 +2,16 @@ refcount_t API compared to atomic_t === +.. contents:: :local: + +Introduction + + The goal of refcount_t API is to provide a minimal API for implementing an object's reference counters. While a generic architecture-independent implementation from lib/refcount.c uses atomic operations underneath, -there are a number of differences between some of the refcount_*() and -atomic_*() functions with regards to the memory ordering guarantees. +there are a number of differences between some of the ``refcount_*()`` and +``atomic_*()`` functions with regards to the memory ordering guarantees. This document outlines the differences and provides respective examples in order to help maintainers validate their code against the change in these memory ordering guarantees. @@ -17,17 +22,17 @@ memory ordering in general and for atomic operations specifically. Relevant types of memory ordering = -**Note**: the following section only covers some of the memory -ordering types that are relevant for the atomics and reference -counters and used through this document. For a much broader picture -please consult memory-barriers.txt document. +.. note:: The following section only covers some of the memory + ordering types that are relevant for the atomics and reference + counters and used through this document. For a much broader picture + please consult memory-barriers.txt document. In the absence of any memory ordering guarantees (i.e. fully unordered) atomics & refcounters only provide atomicity and program order (po) relation (on the same CPU). It guarantees that -each atomic_*() and refcount_*() operation is atomic and instructions +each ``atomic_*()`` and ``refcount_*()`` operation is atomic and instructions are executed in program order on a single CPU. -This is implemented using READ_ONCE()/WRITE_ONCE() and +This is implemented using :c:func:`READ_ONCE`/:c:func:`WRITE_ONCE` and compare-and-swap primitives. A strong (full) memory ordering guarantees that all prior loads and @@ -36,14 +41,15 @@ before any po-later instruction is executed on the same CPU. It also guarantees that all po-earlier stores on the same CPU and all propagated stores from other CPUs must propagate to all other CPUs before any po-later instruction is executed on the original -CPU (A-cumulative property). This is implemented using smp_mb(). +CPU (A-cumulative property). This is implemented using :c:func:`smp_mb`. A RELEASE memory ordering guarantees that all prior loads and stores (all po-earlier instructions) on the same CPU are completed before the operation. It also guarantees that all po-earlier stores on the same CPU and all propagated stores from other CPUs must propagate to all other CPUs before the release operation -(A-cumulative property). This is implemented using smp_store_release(). +(A-cumulative property). This is implemented using +:c:func:`smp_store_release`. A control dependency (on success) for refcounters guarantees that if a reference for an object was successfully obtained (reference @@ -61,59 +67,70 @@ case 1) - non-"Read/Modify/Write" (RMW) ops --- Function changes: -atomic_set() --> refcount_set() -atomic_read() --> refcount_read() + + * :c:func:`atomic_set` --> :c:func:`refcount_set` + * :c:func:`atomic_read` --> :c:func:`refcount_read` Memory ordering guarantee changes: -none (both fully unordered) + + * none (both fully unordered) + case 2) - increment-based ops that return no value -- Function changes: -atomic_inc() --> refcount_inc() -atomic_add() --> refcount_add() + + * :c:func:`atomic_inc` --> :c:func:`refcount_inc` + * :c:func:`atomic_add` --> :c:func:`refcount_add` Memory ordering guarantee changes: -
Re: [PATCH] refcount_t: documentation for memory ordering differences
On Wed, Nov 29, 2017 at 4:36 AM, Elena Reshetova <elena.reshet...@intel.com> wrote: > Some functions from refcount_t API provide different > memory ordering guarantees that their atomic counterparts. > This adds a document outlining these differences. > > Signed-off-by: Elena Reshetova <elena.reshet...@intel.com> Thanks for the improvements! I have some markup changes to add, but I'll send that as a separate patch. Acked-by: Kees Cook <keesc...@chromium.org> -Kees > --- > Documentation/core-api/index.rst | 1 + > Documentation/core-api/refcount-vs-atomic.rst | 129 > ++ > 2 files changed, 130 insertions(+) > create mode 100644 Documentation/core-api/refcount-vs-atomic.rst > > diff --git a/Documentation/core-api/index.rst > b/Documentation/core-api/index.rst > index d5bbe03..d4d54b0 100644 > --- a/Documentation/core-api/index.rst > +++ b/Documentation/core-api/index.rst > @@ -14,6 +14,7 @@ Core utilities > kernel-api > assoc_array > atomic_ops > + refcount-vs-atomic > cpu_hotplug > local_ops > workqueue > diff --git a/Documentation/core-api/refcount-vs-atomic.rst > b/Documentation/core-api/refcount-vs-atomic.rst > new file mode 100644 > index 000..5619d48 > --- /dev/null > +++ b/Documentation/core-api/refcount-vs-atomic.rst > @@ -0,0 +1,129 @@ > +=== > +refcount_t API compared to atomic_t > +=== > + > +The goal of refcount_t API is to provide a minimal API for implementing > +an object's reference counters. While a generic architecture-independent > +implementation from lib/refcount.c uses atomic operations underneath, > +there are a number of differences between some of the refcount_*() and > +atomic_*() functions with regards to the memory ordering guarantees. > +This document outlines the differences and provides respective examples > +in order to help maintainers validate their code against the change in > +these memory ordering guarantees. > + > +memory-barriers.txt and atomic_t.txt provide more background to the > +memory ordering in general and for atomic operations specifically. > + > +Relevant types of memory ordering > += > + > +**Note**: the following section only covers some of the memory > +ordering types that are relevant for the atomics and reference > +counters and used through this document. For a much broader picture > +please consult memory-barriers.txt document. > + > +In the absence of any memory ordering guarantees (i.e. fully unordered) > +atomics & refcounters only provide atomicity and > +program order (po) relation (on the same CPU). It guarantees that > +each atomic_*() and refcount_*() operation is atomic and instructions > +are executed in program order on a single CPU. > +This is implemented using READ_ONCE()/WRITE_ONCE() and > +compare-and-swap primitives. > + > +A strong (full) memory ordering guarantees that all prior loads and > +stores (all po-earlier instructions) on the same CPU are completed > +before any po-later instruction is executed on the same CPU. > +It also guarantees that all po-earlier stores on the same CPU > +and all propagated stores from other CPUs must propagate to all > +other CPUs before any po-later instruction is executed on the original > +CPU (A-cumulative property). This is implemented using smp_mb(). > + > +A RELEASE memory ordering guarantees that all prior loads and > +stores (all po-earlier instructions) on the same CPU are completed > +before the operation. It also guarantees that all po-earlier > +stores on the same CPU and all propagated stores from other CPUs > +must propagate to all other CPUs before the release operation > +(A-cumulative property). This is implemented using smp_store_release(). > + > +A control dependency (on success) for refcounters guarantees that > +if a reference for an object was successfully obtained (reference > +counter increment or addition happened, function returned true), > +then further stores are ordered against this operation. > +Control dependency on stores are not implemented using any explicit > +barriers, but rely on CPU not to speculate on stores. This is only > +a single CPU relation and provides no guarantees for other CPUs. > + > + > +Comparison of functions > +=== > + > +case 1) - non-"Read/Modify/Write" (RMW) ops > +--- > + > +Function changes: > +atomic_set() --> refcount_set() > +atomic_read() --> refcount_read() > + > +Memory ordering guarantee changes: > +none (both fully
Re: [PATCH v2 00/15] ima: digest list feature
On Tue, Nov 7, 2017 at 8:45 AM, Roberto Sassu <roberto.sa...@huawei.com> wrote: > On 11/7/2017 2:37 PM, Mimi Zohar wrote: >> Normally, the protection of kernel memory is out of scope for IMA. >> This patch set introduces an in kernel white list, which would be a >> prime target for attackers looking for ways of by-passing IMA- >> measurement, IMA-appraisal and IMA-audit. Others might disagree, but >> from my perspective, this risk is too high. BTW, which part of the series does the whitelist? I'd agree generally, though: we don't want to make things writable if they're normally read-only. > It would be much easier for an attacker to just set ima_policy_flag to > zero. That's a fair point. I wonder if ima_policy_flag could be marked __ro_after_init? Most of the writes are from __init sections, but I haven't looked closely at when ima_update_policy() gets called. -Kees -- Kees Cook Pixel Security -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] refcount_t: documentation for memory ordering differences
; + > +Memory ordering guarantee changes: > +fully unordered --> fully unordered Same. > +case 3) - decrement-based RMW ops that return no value > +-- > +Function changes: > +atomic_dec() --> refcount_dec() > + > +Memory ordering guarantee changes: > +fully unordered --> RELEASE ordering Should the sections where there is a change include an example of how this might matter to a developer? > + > + > +case 4) - increment-based RMW ops that return a value > +- > + > +Function changes: > +atomic_inc_not_zero() --> refcount_inc_not_zero() > +no atomic counterpart --> refcount_add_not_zero() > + > +Memory ordering guarantees changes: > +fully ordered --> control dependency on success for stores > + > +*Note*: we really assume here that necessary ordering is provided as a result > +of obtaining pointer to the object! Same. > + > + > +case 5) - decrement-based RMW ops that return a value > +- > + > +Function changes: > +atomic_dec_and_test() --> refcount_dec_and_test() > +atomic_sub_and_test() --> refcount_sub_and_test() > +no atomic counterpart --> refcount_dec_if_one() > +atomic_add_unless(, -1, 1) --> refcount_dec_not_one() > + > +Memory ordering guarantees changes: > +fully ordered --> RELEASE ordering + control dependency > + > +Note: atomic_add_unless() only provides full order on success. Same. > + > + > +case 6) - lock-based RMW > + > + > +Function changes: > + > +atomic_dec_and_lock() --> refcount_dec_and_lock() > +atomic_dec_and_mutex_lock() --> refcount_dec_and_mutex_lock() > + > +Memory ordering guarantees changes: > +fully ordered --> RELEASE ordering + control dependency + > + hold spin_lock() on success Same. This looks like a good start to helping people answer questions about refcount_t memory ordering. Thanks! -Kees -- Kees Cook Pixel Security -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/7] Support for automatic checkpatch running in the kernel
On Thu, Nov 16, 2017 at 9:01 AM, Knut Omang <knut.om...@oracle.com> wrote: > The most important checkpatch feature added is the --ignore-cfg feature, which > takes a file argument and parses that file according to this minimal language: > ># comments >line_len >except checkpatch_type [files ...] >pervasive checkpatch_type1 [checkpatch_type2 ...] > > With "make P=2" checkpatch is called with "--file" and "--ignore_cfg > checkpatch.cfg" which causes it to look for a file named 'checkpatch.cfg' in > the > same directory as the source file. If that file exists, checkpatch will be run > with an implicit --strict and with the @ignore list expanded with content from > the configuration file. If it does not exist, make will simply silently > ignore > the file. Will these configurations be cascading? (For example, all of net/ uses a different comment style, so having that recorded in a single file would be nice.) -- Kees Cook Pixel Security -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: LSM docs.
On Sun, Nov 5, 2017 at 7:04 PM, Randy Dunlap <rdun...@infradead.org> wrote: > Hello. World. > > $kerneltree/Documentation/security/LSM.rst contains: > > a new LSM is accepted into the kernel when its intent (a description of > what it tries to protect against and in what cases one would expect to > use it) has been appropriately documented in ``Documentation/security/LSM``. > > but that latter file or directory does not exist. > Is this supposed to refer to Documentation/admin-guide/LSM/ instead? I probably missed that when doing the ReSTification of the security tree. > Since someone was scripting file reference cleanups, hopefully this has > already been found & fixed... If it isn't and you don't send a patch yourself, just let me know and I'll write one up. Thanks for catching that! -Kees > > > Jon, your docs tree is not web viewable, is it? I would look at it (but not > clone it) to check this. > > ta. > -- > ~Randy -- Kees Cook Pixel Security -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 0/4] seccomp: Implement SECCOMP_RET_KILL_PROCESS action
On Mon, Aug 14, 2017 at 1:46 PM, Paul Moore <p...@paul-moore.com> wrote: > On Fri, Aug 11, 2017 at 6:05 PM, Kees Cook <keesc...@chromium.org> wrote: >> This series is the result of Fabricio, Tyler, Will and I going around a >> few times on possible solutions for finding a way to enhance RET_KILL >> to kill the process group. There's a lot of ways this could be done, >> but I wanted something that felt cleanest. My sense of what constitutes >> "clean" has shifted a few times, and after continually running into >> weird corner cases, I decided to make changes to the seccomp action mask, >> which shouldn't be too invasive to userspace as it turns out. Everything >> else becomes much easier, especially after being able to use Tyler's >> new SECCOMP_GET_ACTION_AVAIL operation. >> >> This renames SECCOMP_RET_KILL to SECCOMP_RET_KILL_THREAD and adds >> SECCOMP_RET_KILL_PROCESS. > > I just took a very quick look and I'm not seeing anything that would > cause any backwards compatibility issues for libseccomp. You could > try running the libseccomp tests against a patched kernel to make > sure; the README has all the info you need (pay special attention to > the "live" tests, although those are pretty meager at the moment). Ah-ha, perfect. Ran it now and yup, these all pass. Thanks! -Kees -- Kees Cook Pixel Security -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v4 3/4] seccomp: Implement SECCOMP_RET_KILL_PROCESS action
Right now, SECCOMP_RET_KILL_THREAD (neé SECCOMP_RET_KILL) kills the
current thread. There have been a few requests for this to kill the entire
process (the thread group). This cannot be just changed (discovered when
adding coredump support since coredumping kills the entire process)
because there are userspace programs depending on the thread-kill
behavior.
Instead, implement SECCOMP_RET_KILL_PROCESS, which is 0x8000, and can
be processed as "-1" by the kernel, below the existing RET_KILL that is
ABI-set to "0". For userspace, SECCOMP_RET_ACTION_FULL is added to expand
the mask to the signed bit. Old userspace using the SECCOMP_RET_ACTION
mask will see SECCOMP_RET_KILL_PROCESS as 0 still, but this would only
be visible when examining the siginfo in a core dump from a RET_KILL_*,
where it will think it was thread-killed instead of process-killed.
Attempts to introduce this behavior via other ways (filter flags,
seccomp struct flags, masked RET_DATA bits) all come with weird
side-effects and baggage. This change preserves the central behavioral
expectations of the seccomp filter engine without putting too great
a burden on changes needed in userspace to use the new action.
The new action is discoverable by userspace through either the new
actions_avail sysctl or through the SECCOMP_GET_ACTION_AVAIL seccomp
operation. If used without checking for availability, old kernels
will treat RET_KILL_PROCESS as RET_KILL_THREAD (since the old mask
will produce RET_KILL_THREAD).
Cc: Paul Moore <p...@paul-moore.com>
Cc: Fabricio Voznika <fvozn...@google.com>
Signed-off-by: Kees Cook <keesc...@chromium.org>
---
Documentation/userspace-api/seccomp_filter.rst | 7 ++-
include/uapi/linux/seccomp.h | 1 +
kernel/seccomp.c | 9 +++--
3 files changed, 14 insertions(+), 3 deletions(-)
diff --git a/Documentation/userspace-api/seccomp_filter.rst
b/Documentation/userspace-api/seccomp_filter.rst
index d76396f2d8ed..099c412951d6 100644
--- a/Documentation/userspace-api/seccomp_filter.rst
+++ b/Documentation/userspace-api/seccomp_filter.rst
@@ -87,10 +87,15 @@ Return values
A seccomp filter may return any of the following values. If multiple
filters exist, the return value for the evaluation of a given system
call will always use the highest precedent value. (For example,
-``SECCOMP_RET_KILL_THREAD`` will always take precedence.)
+``SECCOMP_RET_KILL_PROCESS`` will always take precedence.)
In precedence order, they are:
+``SECCOMP_RET_KILL_PROCESS``:
+ Results in the entire process exiting immediately without executing
+ the system call. The exit status of the task (``status & 0x7f``)
+ will be ``SIGSYS``, not ``SIGKILL``.
+
``SECCOMP_RET_KILL_THREAD``:
Results in the task exiting immediately without executing the
system call. The exit status of the task (``status & 0x7f``) will
diff --git a/include/uapi/linux/seccomp.h b/include/uapi/linux/seccomp.h
index 7e77c92df78a..f6bc1dea3247 100644
--- a/include/uapi/linux/seccomp.h
+++ b/include/uapi/linux/seccomp.h
@@ -38,6 +38,7 @@
#define SECCOMP_RET_ALLOW 0x7fffU /* allow */
/* Masks for the return value sections. */
+#define SECCOMP_RET_ACTION_FULL0xU
#define SECCOMP_RET_ACTION 0x7fffU
#define SECCOMP_RET_DATA 0xU
diff --git a/kernel/seccomp.c b/kernel/seccomp.c
index 5c7299b9d953..c24579dfa7a1 100644
--- a/kernel/seccomp.c
+++ b/kernel/seccomp.c
@@ -181,6 +181,7 @@ static int seccomp_check_filter(struct sock_filter *filter,
unsigned int flen)
*
* Returns valid seccomp BPF response codes.
*/
+#define ACTION_ONLY(ret) ((s32)((ret) & (SECCOMP_RET_ACTION_FULL)))
static u32 seccomp_run_filters(const struct seccomp_data *sd,
struct seccomp_filter **match)
{
@@ -206,7 +207,7 @@ static u32 seccomp_run_filters(const struct seccomp_data
*sd,
for (; f; f = f->prev) {
u32 cur_ret = BPF_PROG_RUN(f->prog, sd);
- if ((cur_ret & SECCOMP_RET_ACTION) < (ret &
SECCOMP_RET_ACTION)) {
+ if (ACTION_ONLY(cur_ret) < ACTION_ONLY(ret)) {
ret = cur_ret;
*match = f;
}
@@ -650,7 +651,7 @@ static int __seccomp_filter(int this_syscall, const struct
seccomp_data *sd,
filter_ret = seccomp_run_filters(sd, );
data = filter_ret & SECCOMP_RET_DATA;
- action = filter_ret & SECCOMP_RET_ACTION;
+ action = filter_ret & SECCOMP_RET_ACTION_FULL;
switch (action) {
case SECCOMP_RET_ERRNO:
@@ -890,6 +891,7 @@ static long seccomp_get_action_avail(const char __user
*uaction)
return -EFAULT;
switch (action) {
+ case SECCOMP_RET_KILL_PROCESS:
case SECCOMP_RET_KILL_THREAD:
case SECCOMP_RET_TRAP:
case SECCOMP_RET_ERRNO:
@@ -
[PATCH v4 1/4] seccomp: Rename SECCOMP_RET_KILL to SECCOMP_RET_KILL_THREAD
In preparation for adding SECCOMP_RET_KILL_PROCESS, rename SECCOMP_RET_KILL
to the more accurate SECCOMP_RET_KILL_THREAD.
The existing selftest values are intentionally left as SECCOMP_RET_KILL
just to be sure we're exercising the alias.
Signed-off-by: Kees Cook <keesc...@chromium.org>
---
Documentation/networking/filter.txt| 2 +-
Documentation/userspace-api/seccomp_filter.rst | 4 +--
include/uapi/linux/seccomp.h | 3 +-
kernel/seccomp.c | 39 ++
samples/seccomp/bpf-direct.c | 4 +--
samples/seccomp/bpf-helper.h | 2 +-
tools/testing/selftests/seccomp/seccomp_bpf.c | 17 ++-
7 files changed, 39 insertions(+), 32 deletions(-)
diff --git a/Documentation/networking/filter.txt
b/Documentation/networking/filter.txt
index b69b205501de..73aa0f12156d 100644
--- a/Documentation/networking/filter.txt
+++ b/Documentation/networking/filter.txt
@@ -337,7 +337,7 @@ Examples for low-level BPF:
jeq #14, good /* __NR_rt_sigprocmask */
jeq #13, good /* __NR_rt_sigaction */
jeq #35, good /* __NR_nanosleep */
- bad: ret #0 /* SECCOMP_RET_KILL */
+ bad: ret #0 /* SECCOMP_RET_KILL_THREAD */
good: ret #0x7fff /* SECCOMP_RET_ALLOW */
The above example code can be placed into a file (here called "foo"), and
diff --git a/Documentation/userspace-api/seccomp_filter.rst
b/Documentation/userspace-api/seccomp_filter.rst
index f4977357daf2..d76396f2d8ed 100644
--- a/Documentation/userspace-api/seccomp_filter.rst
+++ b/Documentation/userspace-api/seccomp_filter.rst
@@ -87,11 +87,11 @@ Return values
A seccomp filter may return any of the following values. If multiple
filters exist, the return value for the evaluation of a given system
call will always use the highest precedent value. (For example,
-``SECCOMP_RET_KILL`` will always take precedence.)
+``SECCOMP_RET_KILL_THREAD`` will always take precedence.)
In precedence order, they are:
-``SECCOMP_RET_KILL``:
+``SECCOMP_RET_KILL_THREAD``:
Results in the task exiting immediately without executing the
system call. The exit status of the task (``status & 0x7f``) will
be ``SIGSYS``, not ``SIGKILL``.
diff --git a/include/uapi/linux/seccomp.h b/include/uapi/linux/seccomp.h
index f94433263e4b..5a03f699eb17 100644
--- a/include/uapi/linux/seccomp.h
+++ b/include/uapi/linux/seccomp.h
@@ -27,7 +27,8 @@
* The ordering ensures that a min_t() over composed return values always
* selects the least permissive choice.
*/
-#define SECCOMP_RET_KILL 0xU /* kill the task immediately */
+#define SECCOMP_RET_KILL_THREAD0xU /* kill the thread */
+#define SECCOMP_RET_KILL SECCOMP_RET_KILL_THREAD
#define SECCOMP_RET_TRAP 0x0003U /* disallow and force a SIGSYS */
#define SECCOMP_RET_ERRNO 0x0005U /* returns an errno */
#define SECCOMP_RET_TRACE 0x7ff0U /* pass to a tracer or disallow */
diff --git a/kernel/seccomp.c b/kernel/seccomp.c
index 59cde2ed3b92..95ac54cff00f 100644
--- a/kernel/seccomp.c
+++ b/kernel/seccomp.c
@@ -192,7 +192,7 @@ static u32 seccomp_run_filters(const struct seccomp_data
*sd,
/* Ensure unexpected behavior doesn't result in failing open. */
if (unlikely(WARN_ON(f == NULL)))
- return SECCOMP_RET_KILL;
+ return SECCOMP_RET_KILL_THREAD;
if (!sd) {
populate_seccomp_data(_local);
@@ -529,15 +529,17 @@ static void seccomp_send_sigsys(int syscall, int reason)
#endif /* CONFIG_SECCOMP_FILTER */
/* For use with seccomp_actions_logged */
-#define SECCOMP_LOG_KILL (1 << 0)
+#define SECCOMP_LOG_KILL_THREAD(1 << 0)
#define SECCOMP_LOG_TRAP (1 << 2)
#define SECCOMP_LOG_ERRNO (1 << 3)
#define SECCOMP_LOG_TRACE (1 << 4)
#define SECCOMP_LOG_LOG(1 << 5)
#define SECCOMP_LOG_ALLOW (1 << 6)
-static u32 seccomp_actions_logged = SECCOMP_LOG_KILL | SECCOMP_LOG_TRAP |
- SECCOMP_LOG_ERRNO | SECCOMP_LOG_TRACE |
+static u32 seccomp_actions_logged = SECCOMP_LOG_KILL_THREAD |
+ SECCOMP_LOG_TRAP |
+ SECCOMP_LOG_ERRNO |
+ SECCOMP_LOG_TRACE |
SECCOMP_LOG_LOG;
static inline void seccomp_log(unsigned long syscall, long signr, u32 action,
@@ -560,13 +562,13 @@ static inline void seccomp_log(unsigned long syscall,
long signr, u32 action,
case SECCOMP_RET_LOG:
log = seccomp_actions_logged & SECCOMP_LOG_LOG;
break;
- case SECCOMP_RET_KILL:
+ case SECCOMP_RET_KILL_THREAD:
default:
- log = seccomp_actions_logge
[PATCH v4 0/4] seccomp: Implement SECCOMP_RET_KILL_PROCESS action
This series is the result of Fabricio, Tyler, Will and I going around a few times on possible solutions for finding a way to enhance RET_KILL to kill the process group. There's a lot of ways this could be done, but I wanted something that felt cleanest. My sense of what constitutes "clean" has shifted a few times, and after continually running into weird corner cases, I decided to make changes to the seccomp action mask, which shouldn't be too invasive to userspace as it turns out. Everything else becomes much easier, especially after being able to use Tyler's new SECCOMP_GET_ACTION_AVAIL operation. This renames SECCOMP_RET_KILL to SECCOMP_RET_KILL_THREAD and adds SECCOMP_RET_KILL_PROCESS. Please take a look! Thanks, -Kees v4: - basically only kept the selftests, tossed everything else - expanded SECCOMP_RET_ACTION mask into SECCOMP_RET_ACTION_FULL - renamed SECCOMP_RET_KILL to SECCOMP_RET_KILL_THREAD - added SECCOMP_RET_KILL_PROCESS as "-1" value, below 0 (RET_KILL) - use signed tests in the kernel for seccomp actions v3: - adjust seccomp_run_filters() to avoid later filters from masking kill-process RET_KILL actions (drewry) - add test for masked RET_KILL. v2: - moved kill_process bool into struct padding gap (tyhicks) - improved comments/docs in various places for clarify (tyhicks) - use ASSERT_TRUE() for WIFEXITED and WIFSIGNALLED (tyhicks) - adding Reviewed-bys from tyhicks -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v4 2/4] seccomp: Introduce SECCOMP_RET_KILL_PROCESS
This introduces the BPF return value for SECCOMP_RET_KILL_PROCESS to kill
an entire process. This cannot yet be reached by seccomp, but it changes
the default-kill behavior (for unknown return values) from kill-thread to
kill-process.
Signed-off-by: Kees Cook <keesc...@chromium.org>
---
include/uapi/linux/seccomp.h | 18 ++
kernel/seccomp.c | 22 --
2 files changed, 26 insertions(+), 14 deletions(-)
diff --git a/include/uapi/linux/seccomp.h b/include/uapi/linux/seccomp.h
index 5a03f699eb17..7e77c92df78a 100644
--- a/include/uapi/linux/seccomp.h
+++ b/include/uapi/linux/seccomp.h
@@ -22,18 +22,20 @@
/*
* All BPF programs must return a 32-bit value.
* The bottom 16-bits are for optional return data.
- * The upper 16-bits are ordered from least permissive values to most.
+ * The upper 16-bits are ordered from least permissive values to most,
+ * as a signed value (so 0x800 is negative).
*
* The ordering ensures that a min_t() over composed return values always
* selects the least permissive choice.
*/
-#define SECCOMP_RET_KILL_THREAD0xU /* kill the thread */
-#define SECCOMP_RET_KILL SECCOMP_RET_KILL_THREAD
-#define SECCOMP_RET_TRAP 0x0003U /* disallow and force a SIGSYS */
-#define SECCOMP_RET_ERRNO 0x0005U /* returns an errno */
-#define SECCOMP_RET_TRACE 0x7ff0U /* pass to a tracer or disallow */
-#define SECCOMP_RET_LOG0x7ffcU /* allow after logging */
-#define SECCOMP_RET_ALLOW 0x7fffU /* allow */
+#define SECCOMP_RET_KILL_PROCESS 0x8000U /* kill the process */
+#define SECCOMP_RET_KILL_THREAD 0xU /* kill the thread */
+#define SECCOMP_RET_KILLSECCOMP_RET_KILL_THREAD
+#define SECCOMP_RET_TRAP0x0003U /* disallow and force a SIGSYS */
+#define SECCOMP_RET_ERRNO 0x0005U /* returns an errno */
+#define SECCOMP_RET_TRACE 0x7ff0U /* pass to a tracer or disallow */
+#define SECCOMP_RET_LOG 0x7ffcU /* allow after logging */
+#define SECCOMP_RET_ALLOW 0x7fffU /* allow */
/* Masks for the return value sections. */
#define SECCOMP_RET_ACTION 0x7fffU
diff --git a/kernel/seccomp.c b/kernel/seccomp.c
index 95ac54cff00f..5c7299b9d953 100644
--- a/kernel/seccomp.c
+++ b/kernel/seccomp.c
@@ -192,7 +192,7 @@ static u32 seccomp_run_filters(const struct seccomp_data
*sd,
/* Ensure unexpected behavior doesn't result in failing open. */
if (unlikely(WARN_ON(f == NULL)))
- return SECCOMP_RET_KILL_THREAD;
+ return SECCOMP_RET_KILL_PROCESS;
if (!sd) {
populate_seccomp_data(_local);
@@ -529,14 +529,16 @@ static void seccomp_send_sigsys(int syscall, int reason)
#endif /* CONFIG_SECCOMP_FILTER */
/* For use with seccomp_actions_logged */
-#define SECCOMP_LOG_KILL_THREAD(1 << 0)
+#define SECCOMP_LOG_KILL_PROCESS (1 << 0)
+#define SECCOMP_LOG_KILL_THREAD(1 << 1)
#define SECCOMP_LOG_TRAP (1 << 2)
#define SECCOMP_LOG_ERRNO (1 << 3)
#define SECCOMP_LOG_TRACE (1 << 4)
#define SECCOMP_LOG_LOG(1 << 5)
#define SECCOMP_LOG_ALLOW (1 << 6)
-static u32 seccomp_actions_logged = SECCOMP_LOG_KILL_THREAD |
+static u32 seccomp_actions_logged = SECCOMP_LOG_KILL_PROCESS |
+ SECCOMP_LOG_KILL_THREAD |
SECCOMP_LOG_TRAP |
SECCOMP_LOG_ERRNO |
SECCOMP_LOG_TRACE |
@@ -563,8 +565,11 @@ static inline void seccomp_log(unsigned long syscall, long
signr, u32 action,
log = seccomp_actions_logged & SECCOMP_LOG_LOG;
break;
case SECCOMP_RET_KILL_THREAD:
- default:
log = seccomp_actions_logged & SECCOMP_LOG_KILL_THREAD;
+ break;
+ case SECCOMP_RET_KILL_PROCESS:
+ default:
+ log = seccomp_actions_logged & SECCOMP_LOG_KILL_PROCESS;
}
/*
@@ -719,10 +724,12 @@ static int __seccomp_filter(int this_syscall, const
struct seccomp_data *sd,
return 0;
case SECCOMP_RET_KILL_THREAD:
+ case SECCOMP_RET_KILL_PROCESS:
default:
seccomp_log(this_syscall, SIGSYS, action, true);
/* Dump core only if this is the last remaining thread. */
- if (get_nr_threads(current) == 1) {
+ if (action == SECCOMP_RET_KILL_PROCESS ||
+ get_nr_threads(current) == 1) {
siginfo_t info;
/* Show the original registers in the dump. */
@@ -731,7 +738,10 @@ static int __seccomp_filter(int this_syscall, const struct
seccomp_data *sd,
seccomp_init_
[PATCH v4 4/4] selftests/seccomp: Test thread vs process killing
This verifies that SECCOMP_RET_KILL_PROCESS is higher priority than
SECCOMP_RET_KILL_THREAD. (This also moves a bunch of defines up earlier
in the file to use them earlier.)
Signed-off-by: Kees Cook <keesc...@chromium.org>
Reviewed-by: Tyler Hicks <tyhi...@canonical.com>
---
tools/testing/selftests/seccomp/seccomp_bpf.c | 228 +++---
1 file changed, 168 insertions(+), 60 deletions(-)
diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c
b/tools/testing/selftests/seccomp/seccomp_bpf.c
index 5680e3ae33fd..fa097a270616 100644
--- a/tools/testing/selftests/seccomp/seccomp_bpf.c
+++ b/tools/testing/selftests/seccomp/seccomp_bpf.c
@@ -68,7 +68,17 @@
#define SECCOMP_MODE_FILTER 2
#endif
-#ifndef SECCOMP_RET_KILL_THREAD
+#ifndef SECCOMP_RET_ALLOW
+struct seccomp_data {
+ int nr;
+ __u32 arch;
+ __u64 instruction_pointer;
+ __u64 args[6];
+};
+#endif
+
+#ifndef SECCOMP_RET_KILL_PROCESS
+#define SECCOMP_RET_KILL_PROCESS 0x8000U /* kill the process */
#define SECCOMP_RET_KILL_THREAD 0xU /* kill the thread */
#endif
#ifndef SECCOMP_RET_KILL
@@ -82,17 +92,53 @@
#define SECCOMP_RET_LOG 0x7ffcU /* allow after logging */
#endif
-#ifndef SECCOMP_RET_ACTION
-/* Masks for the return value sections. */
-#define SECCOMP_RET_ACTION 0x7fffU
-#define SECCOMP_RET_DATA0xU
+#ifndef __NR_seccomp
+# if defined(__i386__)
+# define __NR_seccomp 354
+# elif defined(__x86_64__)
+# define __NR_seccomp 317
+# elif defined(__arm__)
+# define __NR_seccomp 383
+# elif defined(__aarch64__)
+# define __NR_seccomp 277
+# elif defined(__hppa__)
+# define __NR_seccomp 338
+# elif defined(__powerpc__)
+# define __NR_seccomp 358
+# elif defined(__s390__)
+# define __NR_seccomp 348
+# else
+# warning "seccomp syscall number unknown for this architecture"
+# define __NR_seccomp 0x
+# endif
+#endif
-struct seccomp_data {
- int nr;
- __u32 arch;
- __u64 instruction_pointer;
- __u64 args[6];
-};
+#ifndef SECCOMP_SET_MODE_STRICT
+#define SECCOMP_SET_MODE_STRICT 0
+#endif
+
+#ifndef SECCOMP_SET_MODE_FILTER
+#define SECCOMP_SET_MODE_FILTER 1
+#endif
+
+#ifndef SECCOMP_GET_ACTION_AVAIL
+#define SECCOMP_GET_ACTION_AVAIL 2
+#endif
+
+#ifndef SECCOMP_FILTER_FLAG_TSYNC
+#define SECCOMP_FILTER_FLAG_TSYNC 1
+#endif
+
+#ifndef SECCOMP_FILTER_FLAG_LOG
+#define SECCOMP_FILTER_FLAG_LOG 2
+#endif
+
+#ifndef seccomp
+int seccomp(unsigned int op, unsigned int flags, void *args)
+{
+ errno = 0;
+ return syscall(__NR_seccomp, op, flags, args);
+}
#endif
#if __BYTE_ORDER == __LITTLE_ENDIAN
@@ -550,6 +596,117 @@ TEST_SIGNAL(KILL_one_arg_six, SIGSYS)
close(fd);
}
+/* This is a thread task to die via seccomp filter violation. */
+void *kill_thread(void *data)
+{
+ bool die = (bool)data;
+
+ if (die) {
+ prctl(PR_GET_SECCOMP, 0, 0, 0, 0);
+ return (void *)SIBLING_EXIT_FAILURE;
+ }
+
+ return (void *)SIBLING_EXIT_UNKILLED;
+}
+
+/* Prepare a thread that will kill itself or both of us. */
+void kill_thread_or_group(struct __test_metadata *_metadata, bool kill_process)
+{
+ pthread_t thread;
+ void *status;
+ /* Kill only when calling __NR_prctl. */
+ struct sock_filter filter_thread[] = {
+ BPF_STMT(BPF_LD|BPF_W|BPF_ABS,
+ offsetof(struct seccomp_data, nr)),
+ BPF_JUMP(BPF_JMP|BPF_JEQ|BPF_K, __NR_prctl, 0, 1),
+ BPF_STMT(BPF_RET|BPF_K, SECCOMP_RET_KILL_THREAD),
+ BPF_STMT(BPF_RET|BPF_K, SECCOMP_RET_ALLOW),
+ };
+ struct sock_fprog prog_thread = {
+ .len = (unsigned short)ARRAY_SIZE(filter_thread),
+ .filter = filter_thread,
+ };
+ struct sock_filter filter_process[] = {
+ BPF_STMT(BPF_LD|BPF_W|BPF_ABS,
+ offsetof(struct seccomp_data, nr)),
+ BPF_JUMP(BPF_JMP|BPF_JEQ|BPF_K, __NR_prctl, 0, 1),
+ BPF_STMT(BPF_RET|BPF_K, SECCOMP_RET_KILL_PROCESS),
+ BPF_STMT(BPF_RET|BPF_K, SECCOMP_RET_ALLOW),
+ };
+ struct sock_fprog prog_process = {
+ .len = (unsigned short)ARRAY_SIZE(filter_process),
+ .filter = filter_process,
+ };
+
+ ASSERT_EQ(0, prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0)) {
+ TH_LOG("Kernel does not support PR_SET_NO_NEW_PRIVS!");
+ }
+
+ ASSERT_EQ(0, seccomp(SECCOMP_SET_MODE_FILTER, 0,
+kill_process ? _process : _thread));
+
+ /*
+* Add the KILL_THREAD rule again to make sure that the KILL_PROCESS
+* flag cannot be downgraded by a new filter.
+*/
+ ASSERT_EQ(0, seccomp(SECCOMP_SET_MODE_FILTER, 0, _thread));
+
+ /* Start a thread that will exit immediately. */
+ ASSERT_EQ(0, pthread_create(, NULL, kill_thread, (void *)false));
+
Re: [PATCH 01/11] arm64: docs: describe ELF hwcaps
On Wed, Jul 19, 2017 at 9:01 AM, Mark Rutland <mark.rutl...@arm.com> wrote: > We don't document our ELF hwcaps, leaving developers to interpret them > according to hearsay, guesswork, or (in exceptional cases) inspection of > the current kernel code. > > This is less than optimal, and it would be far better if we had some > definitive description of each of the ELF hwcaps that developers could > refer to. > > This patch adds a document describing the (native) arm64 ELF hwcaps. > > Signed-off-by: Mark Rutland <mark.rutl...@arm.com> > Cc: Catalin Marinas <catalin.mari...@arm.com> > Cc: Dave Martin <dave.mar...@arm.com> > Cc: Suzuki K Poulose <suzuki.poul...@arm.com> > Cc: Will Deacon <will.dea...@arm.com> > --- > Documentation/arm64/elf_hwcaps.txt | 133 > + With the kernel docs moving to ReST markup[1], perhaps reformat this to a .rst file and link to it from somewhere sensible in the ReST tree, perhaps the userspace API section in Documentation/userspace-api/index.rst? -Kees [1] https://www.kernel.org/doc/html/latest/doc-guide/sphinx.html -- Kees Cook Pixel Security -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v8 0/2] dm: boot a mapped device without an initramfs
On Fri, May 19, 2017 at 12:11 AM, Enric Balletbo i Serra <enric.balle...@collabora.com> wrote: > Dear all, > > So here is a new version of the patches to be reviewed, this time as > suggested by Alasdair the patches are reworked to match with the new > dmsetup bootformat feature [1]. These patches are not reviewed yet but > the format was discussed in the IRC and was suggested to send the > kernel patches in parallel. Just pinging on this thread again. Alasdair, how does this look to you? Thanks! -Kees > > Changes since v7: > - Fix build error due commit > e516db4f67 (dm ioctl: add a new DM_DEV_ARM_POLL ioctl) > > Changes since v6: > - Add a new function to issue the equivalent of a DM ioctl programatically. > - Use the new ioctl interface to create the devices. > - Use a comma-delimited and semi-colon delimited dmsetup-like commands. > > Changes since v5: > - https://www.redhat.com/archives/dm-devel/2016-February/msg00112.html > > [1] https://www.redhat.com/archives/linux-lvm/2017-May/msg00047.html > > Wating for your feedback, > > Enric Balletbo i Serra (1): > dm ioctl: add a device mapper ioctl function. > > Will Drewry (1): > init: add support to directly boot to a mapped device > > Documentation/admin-guide/kernel-parameters.rst | 1 + > Documentation/admin-guide/kernel-parameters.txt | 3 + > Documentation/device-mapper/dm-boot.txt | 65 > drivers/md/dm-ioctl.c | 50 +++ > include/linux/device-mapper.h | 6 + > init/Makefile | 1 + > init/do_mounts.c| 1 + > init/do_mounts.h| 10 + > init/do_mounts_dm.c | 459 > > 9 files changed, 596 insertions(+) > create mode 100644 Documentation/device-mapper/dm-boot.txt > create mode 100644 init/do_mounts_dm.c > > -- > 2.9.3 > -- Kees Cook Pixel Security -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] mm: Allow slab_nomerge to be set at build time
On Fri, Jun 23, 2017 at 7:06 AM, Michal Hocko <mho...@kernel.org> wrote: > On Tue 20-06-17 16:09:11, Kees Cook wrote: >> Some hardened environments want to build kernels with slab_nomerge >> already set (so that they do not depend on remembering to set the kernel >> command line option). This is desired to reduce the risk of kernel heap >> overflows being able to overwrite objects from merged caches and changes >> the requirements for cache layout control, increasing the difficulty of >> these attacks. By keeping caches unmerged, these kinds of exploits can >> usually only damage objects in the same cache (though the risk to metadata >> exploitation is unchanged). > > Do we really want to have a dedicated config for each hardening specific > kernel command line? I believe we have quite a lot of config options > already. Can we rather have a CONFIG_HARDENED_CMD_OPIONS and cover all > those defauls there instead? There's not been a lot of success with grouped Kconfigs in the past (e.g. CONFIG_EXPERIMENTAL), but one thing that has been suggested is a defconfig-like make target that would collect all the things together. I haven't had time for that, but that would let us group the various configs. Additionally, using something like CONFIG_CMDLINE seems a little clunky to me. -Kees -- Kees Cook Pixel Security -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 0/4] kmod: help make deterministic
On Tue, Jun 20, 2017 at 1:56 PM, Luis R. Rodriguez <mcg...@kernel.org> wrote: > On Fri, May 26, 2017 at 02:12:24PM -0700, Luis R. Rodriguez wrote: >> This v3 nukes the proc sysctl interface in favor for just letting userspace >> just check kernel revision. Prior to whenever this is merged userspace should >> try to avoid hammering more than 50 kmod threads as they can fail and it'd >> get -ENOMEM. >> >> We do away with the old heuristics on assuming you could end up with >> less than max_threads/2 < 50 threads as Dmitry notes this would mean having >> a system with 16 MiB of RAM with modules enabled. It simplifies our patch >> "kmod: reduce atomic operations on kmod_concurrent" considerbly. >> >> Since the sysctl interface is gone, this no longer depends on any >> other patches, the series is independent. As usual the series is >> available on my linux-next 20170526-kmod-only branch which is based >> on next-20170526. >> >> [0] >> https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux-next.git/log/?h=20170526-kmod-only >> >> Luis >> >> Luis R. Rodriguez (4): >> module: use list_for_each_entry_rcu() on find_module_all() >> kmod: reduce atomic operations on kmod_concurrent and simplify >> kmod: add test driver to stress test the module loader >> kmod: throttle kmod thread limit > > About a month now with no further nitpicks. What tree should these changes > go through if there are no issues? Andrew's, Jessica's ? Seems like going through Jessica's would make the most sense? -Kees -- Kees Cook Pixel Security -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] mm: Allow slab_nomerge to be set at build time
On Tue, Jun 20, 2017 at 4:16 PM, Randy Dunlap <rdun...@infradead.org> wrote: > On 06/20/2017 04:09 PM, Kees Cook wrote: >> Some hardened environments want to build kernels with slab_nomerge >> already set (so that they do not depend on remembering to set the kernel >> command line option). This is desired to reduce the risk of kernel heap >> overflows being able to overwrite objects from merged caches and changes >> the requirements for cache layout control, increasing the difficulty of >> these attacks. By keeping caches unmerged, these kinds of exploits can >> usually only damage objects in the same cache (though the risk to metadata >> exploitation is unchanged). >> >> Cc: Daniel Micay <danielmi...@gmail.com> >> Cc: David Windsor <d...@nullcore.net> >> Cc: Eric Biggers <ebigge...@gmail.com> >> Signed-off-by: Kees Cook <keesc...@chromium.org> >> --- >> v2: split out of slab whitelisting series >> --- >> Documentation/admin-guide/kernel-parameters.txt | 10 -- >> init/Kconfig| 14 ++ >> mm/slab_common.c| 5 ++--- >> 3 files changed, 24 insertions(+), 5 deletions(-) > >> diff --git a/init/Kconfig b/init/Kconfig >> index 1d3475fc9496..ce813acf2f4f 100644 >> --- a/init/Kconfig >> +++ b/init/Kconfig >> @@ -1891,6 +1891,20 @@ config SLOB >> >> endchoice >> >> +config SLAB_MERGE_DEFAULT >> + bool "Allow slab caches to be merged" >> + default y >> + help >> + For reduced kernel memory fragmentation, slab caches can be >> + merged when they share the same size and other characteristics. >> + This carries a risk of kernel heap overflows being able to >> + overwrite objects from merged caches (and more easily control >> + cache layout), which makes such heap attacks easier to exploit >> + by attackers. By keeping caches unmerged, these kinds of exploits >> + can usually only damage objects in the same cache. To disable >> + merging at runtime, "slab_nomerge" can be passed on the kernel >> + command line. > > command line or this option can be disabled in the kernel config. Isn't that implicit in that it is Kconfig help text? Happy to add it, but seems redundant to me. -Kees > >> + >> config SLAB_FREELIST_RANDOM >> default n >> depends on SLAB || SLUB > > -- > ~Randy -- Kees Cook Pixel Security -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2] mm: Allow slab_nomerge to be set at build time
Some hardened environments want to build kernels with slab_nomerge
already set (so that they do not depend on remembering to set the kernel
command line option). This is desired to reduce the risk of kernel heap
overflows being able to overwrite objects from merged caches and changes
the requirements for cache layout control, increasing the difficulty of
these attacks. By keeping caches unmerged, these kinds of exploits can
usually only damage objects in the same cache (though the risk to metadata
exploitation is unchanged).
Cc: Daniel Micay <danielmi...@gmail.com>
Cc: David Windsor <d...@nullcore.net>
Cc: Eric Biggers <ebigge...@gmail.com>
Signed-off-by: Kees Cook <keesc...@chromium.org>
---
v2: split out of slab whitelisting series
---
Documentation/admin-guide/kernel-parameters.txt | 10 --
init/Kconfig| 14 ++
mm/slab_common.c| 5 ++---
3 files changed, 24 insertions(+), 5 deletions(-)
diff --git a/Documentation/admin-guide/kernel-parameters.txt
b/Documentation/admin-guide/kernel-parameters.txt
index 7737ab5d04b2..94d8b8195cb8 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -3715,8 +3715,14 @@
slab_nomerge[MM]
Disable merging of slabs with similar size. May be
necessary if there is some reason to distinguish
- allocs to different slabs. Debug options disable
- merging on their own.
+ allocs to different slabs, especially in hardened
+ environments where the risk of heap overflows and
+ layout control by attackers can usually be
+ frustrated by disabling merging. This will reduce
+ most of the exposure of a heap attack to a single
+ cache (risks via metadata attacks are mostly
+ unchanged). Debug options disable merging on their
+ own.
For more information see Documentation/vm/slub.txt.
slab_max_order= [MM, SLAB]
diff --git a/init/Kconfig b/init/Kconfig
index 1d3475fc9496..ce813acf2f4f 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -1891,6 +1891,20 @@ config SLOB
endchoice
+config SLAB_MERGE_DEFAULT
+ bool "Allow slab caches to be merged"
+ default y
+ help
+ For reduced kernel memory fragmentation, slab caches can be
+ merged when they share the same size and other characteristics.
+ This carries a risk of kernel heap overflows being able to
+ overwrite objects from merged caches (and more easily control
+ cache layout), which makes such heap attacks easier to exploit
+ by attackers. By keeping caches unmerged, these kinds of exploits
+ can usually only damage objects in the same cache. To disable
+ merging at runtime, "slab_nomerge" can be passed on the kernel
+ command line.
+
config SLAB_FREELIST_RANDOM
default n
depends on SLAB || SLUB
diff --git a/mm/slab_common.c b/mm/slab_common.c
index 01a0fe2eb332..904a83be82de 100644
--- a/mm/slab_common.c
+++ b/mm/slab_common.c
@@ -47,13 +47,12 @@ static DECLARE_WORK(slab_caches_to_rcu_destroy_work,
/*
* Merge control. If this is set then no merging of slab caches will occur.
- * (Could be removed. This was introduced to pacify the merge skeptics.)
*/
-static int slab_nomerge;
+static bool slab_nomerge = !IS_ENABLED(CONFIG_SLAB_MERGE_DEFAULT);
static int __init setup_slab_nomerge(char *str)
{
- slab_nomerge = 1;
+ slab_nomerge = true;
return 1;
}
--
2.7.4
--
Kees Cook
Pixel Security
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 22/31] gcc-plugins.txt: standardize document format
On Sat, Jun 17, 2017 at 8:25 AM, Mauro Carvalho Chehab <mche...@s-opensource.com> wrote: > Each text file under Documentation follows a different > format. Some doesn't even have titles! > > Change its representation to follow the adopted standard, > using ReST markups for it to be parseable by Sphinx: > > - promote main title; > - use the right markup for footnotes; > - use bold markup for files name; > - identify literal blocks; > - add blank lines to avoid Sphinx to complain; > - remove numeration from titles. > > Signed-off-by: Mauro Carvalho Chehab <mche...@s-opensource.com> Thanks! This should maybe get moved/indexed in dev-tools/. What do you think? Regardless: Acked-by: Kees Cook <keesc...@chromium.org> -Kees -- Kees Cook Pixel Security -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 next 1/3] modules:capabilities: allow __request_module() to take a capability argument
On Thu, Jun 1, 2017 at 7:56 AM, Djalal Harouni <tix...@gmail.com> wrote:
> module_require_cap = 0;
>
> if (autoload == MODULES_AUTOLOAD_DISABLED)
> return -EPERM;
>
> if (autoload == MODULES_AUTOLOAD_PRIVILEGED || require_cap > 0) {
> if (prefix != NULL && *prefix != '\0')
> /*
> * Allow non-CAP_SYS_MODULE caps when
> * using a distinct prefix.
> */
> module_require_cap = require_cap;
> else
> /*
> * Otherwise always require CAP_SYS_MODULE if no
> * valid prefix. Callers that do not provide a valid
> prefix
> * should not provide a require_cap > 0
> */
> module_require_cap = CAP_SYS_MODULE;
> }
>
> /* If autoload allowed and 'module_require_cap' was *never* set,
> allow */
> if (module_require_cap == 0 && autoload == MODULES_AUTOLOAD_ALLOWED)
> return 0;
>
> return capable(module_require_cap) ? 0 : -EPERM;
>
> Maybe you will agree :-) ?
Yes! Looks good. I was accidentally still thinking about the caps
checks being in the net code, but obviously, that wouldn't be the case
anymore. Thanks for the catch. :)
> BTW Kees, also in next version I won't remove the
> capable(CAP_NET_ADMIN) check from [1]
> even if there is the new request_module_cap(), I would like it to be
> in a different patches, this way we go incremental
> and maybe it is better to merge what we have now ? and follow up
> later, and of course if other maintainers agree too!
Yes, incremental. I would suggest first creating the API changes to
move a basic require_cap test into the LSM (which would drop the
open-coded capable() checks in the net code), and then add the
autoload logic in the following patches. That way the "infrastructure"
changes happen separately and do not change any behaviors, but moves
the caps test down where its wanted in the LSM, before then augmenting
the logic.
> I just need a bit of free time to check again everything and will send
> a v5 with all requested changes.
Great, thank you!
-Kees
--
Kees Cook
Pixel Security
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 next 1/3] modules:capabilities: allow __request_module() to take a capability argument
On Wed, May 24, 2017 at 7:16 AM, Djalal Harouni <tix...@gmail.com> wrote:
> On Tue, May 23, 2017 at 9:19 PM, Kees Cook <keesc...@google.com> wrote:
>> On Tue, May 23, 2017 at 3:29 AM, Djalal Harouni <tix...@gmail.com> wrote:
>> Even in the existing code, there is a sense about CAP_NET_ADMIN and
>> CAP_SYS_MODULE having different privilege levels, in that
>> CAP_NET_ADMIN can only load netdev-%s modules, but CAP_SYS_MODULE can
>> load any module. What about refining request_module_cap() to _require_
>> an explicit string prefix instead of an arbitrary format string? e.g.
>> request_module_cap(CAP_NET_ADMIN, "netdev", "%s", name) which would
>> make requests for ("netdev-%s", name)
>>
>> I see a few options:
>>
>> 1) keep what you have for v4, and hope other places don't use
>> __request_module. (I'm not a fan of this.)
>
> Yes even if it is documented I wouldn't bet on it, though. :-)
Okay, we seem to agree: we'll not use #1.
>> 2) switch the logic on autoload==1 from OR to AND: both the specified
>> caps _and_ CAP_SYS_MODULE are required. (This seems like it might make
>> autoload==1 less useful.)
>
> That will restrict some userspace that works only with CAP_NET_ADMIN.
Nor #2.
>> 3) use the request_module_cap() outlined above, which requires that
>> modules being loaded under a CAP_SYS_MODULE-aliased capability are at
>> least restricted to a subset of kernel module names.
>
> This one tends to allow usability.
Right, discussed below...
>> 4) same as 3 but also insert autoload==2 level that switches from OR
>> to AND (bumping existing ==2 to ==3).
>
> I wouldn't expose autoload to callers, I think it is better if it
> stays a property of the module subsystem. But lets use the bump idea,
> please see below.
If we can't agree below, I think #4 would be a good way to allow for
both states.
>> What do you think?
>
> Ok so given that we already have modules_autoload_mode=2 disabled,
> maybe we go with 3) like this ?
>
> int __request_module(bool wait, int required_cap, const char *prefix,
> const char *name, ...);
> #define request_module(mod...) \
> __request_module(true, -1, NULL, mod)
> #define request_module_cap(required_cap, prefix, mod...) \
> __request_module(true, required_cap, prefix, mod)
>
> and we require allow_cap and prefix to be set.
>
> request_module_cap(CAP_NET_ADMIN, "netdev-", "%s", name) for
> net/core/dev_ioctl.c:dev_load()
>
> request_module_cap(CAP_NET_ADMIN, "tcp_", "%s", name) for
> net/ipv4/tcp_cong.c functions.
>
>
> Then
> __request_module()
> -> security_kernel_module_request(module_name, required_cap, prefix)
> -> may_autoload_module(current, module_name, required_cap, prefix)
>
>
> And update may_autoload_module() as below ? we hard code CAP_NET_ADMIN
> and CAP_SYS_MODULE inside and make them the only capabilities needed
> for a privileged auto-load operation.
I still think making a specific exception for CAP_NET_ADMIN is not the
right solution, instead allowing for non-CAP_SYS_MODULE caps when
using a distinct prefix.
> request_module_cap(CAP_SYS_MODULE, ...) or
> request_module_cap(CAP_NET_ADMIN, ...) if the autoload should be a
> privileged operation.
>
> Kees will this work ?
>
> Jessica, Rusty, Serge. What do you think ? I definitively think that
> module_autoload should be contained only inside the module subsystem..
I'd change it like this:
> +int may_autoload_module(struct task_struct *task, char *kmod_name,
> + int require_cap, char *prefix)
> +{
> + unsigned int autoload;
> + int module_require_cap = 0;
I'd initialize this to module_require_cap = CAP_SYS_MODULE;
> +
> + if (require_cap > 0) {
> + if (prefix == NULL || *prefix == '\0')
> + return -EPERM;
Since an unprefixed module load should only be CAP_SYS_MODULE, change
the above "if" to:
if (require_cap > 0 && prefix != NULL && *prefix != '\0')
> +
> + /*
> +* We only allow CAP_SYS_MODULE or CAP_NET_ADMIN for
> +* 'netdev-%s' modules for backward compatibility.
> +* Please do not overload capabilities.
> +*/
> + if (require_cap == CAP_SYS_MODULE ||
> + require_cap == CAP_NET_ADMIN)
> + module_require_cap = require_cap;
> + else
> + return -EPERM;
> + }
And then drop all these checks, leaving only:
module_require_cap = require_cap;
> +
Re: [PATCH v5 0/7] Add kselftest_harness.h
On Fri, May 26, 2017 at 11:43 AM, Mickaël Salaün <m...@digikod.net> wrote:
> Hi,
>
> This patch series make the seccomp/test_harness.h more generally available [1]
> and update the kselftest documentation in the Sphinx format. It also improve
> the Makefile of seccomp tests to take into account any kselftest_harness.h
> update.
>
> [1]
> https://lkml.kernel.org/r/CAGXu5j+8CVz8vL51DRYXqOY=xc3zuKFf=ptene88xyhzfyi...@mail.gmail.com
>
> Regards,
>
> Mickaël Salaün (7):
> selftests: Make test_harness.h more generally available
> selftests: Cosmetic renames in kselftest_harness.h
> selftests/seccomp: Force rebuild according to dependencies
> Documentation/dev-tools: Add kselftest
> Documentation/dev-tools: Use reStructuredText markups for kselftest
> selftests: Remove the TEST_API() wrapper from kselftest_harness.h
> Documentation/dev-tools: Add kselftest_harness documentation
I think this series looks great; I've added my two remaining Acks.
Shuah, when you have time, please pull these into the selftest tree.
Thanks!
-Kees
>
> Documentation/00-INDEX | 2 -
> Documentation/dev-tools/index.rst | 1 +
> .../{kselftest.txt => dev-tools/kselftest.rst} | 101 ++-
> MAINTAINERS| 1 +
> .../test_harness.h => kselftest_harness.h} | 691
> +
> tools/testing/selftests/seccomp/Makefile | 2 +
> tools/testing/selftests/seccomp/seccomp_bpf.c | 2 +-
> 7 files changed, 520 insertions(+), 280 deletions(-)
> rename Documentation/{kselftest.txt => dev-tools/kselftest.rst} (52%)
> rename tools/testing/selftests/{seccomp/test_harness.h =>
> kselftest_harness.h} (52%)
>
> --
> 2.11.0
>
--
Kees Cook
Pixel Security
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 6/7] selftests: Remove the TEST_API() wrapper from kselftest_harness.h
On Fri, May 26, 2017 at 11:44 AM, Mickaël Salaün <m...@digikod.net> wrote:
> Remove the TEST_API() wrapper to expose the underlying macro arguments
> to the documentation tools.
>
> Use "git diff --patience" to get a more readable patch.
>
> Changes since v4:
> * standalone patch to ease the review (requested by Kees Cook)
>
> Signed-off-by: Mickaël Salaün <m...@digikod.net>
> Cc: Andy Lutomirski <l...@amacapital.net>
> Cc: Jonathan Corbet <cor...@lwn.net>
> Cc: Kees Cook <keesc...@chromium.org>
> Cc: Shuah Khan <sh...@kernel.org>
> Cc: Will Drewry <w...@chromium.org>
Thanks for the split!
Acked-by: Kees Cook <keesc...@chromium.org>
-Kees
> ---
> tools/testing/selftests/kselftest_harness.h | 349
>
> 1 file changed, 147 insertions(+), 202 deletions(-)
>
> diff --git a/tools/testing/selftests/kselftest_harness.h
> b/tools/testing/selftests/kselftest_harness.h
> index 171e70aead9c..45f807ce37e1 100644
> --- a/tools/testing/selftests/kselftest_harness.h
> +++ b/tools/testing/selftests/kselftest_harness.h
> @@ -51,147 +51,6 @@
> #include
> #include
>
> -/* All exported functionality should be declared through this macro. */
> -#define TEST_API(x) _##x
> -
> -/*
> - * Exported APIs
> - */
> -
> -/* TEST(name) { implementation }
> - * Defines a test by name.
> - * Names must be unique and tests must not be run in parallel. The
> - * implementation containing block is a function and scoping should be
> treated
> - * as such. Returning early may be performed with a bare "return;"
> statement.
> - *
> - * EXPECT_* and ASSERT_* are valid in a TEST() { } context.
> - */
> -#define TEST TEST_API(TEST)
> -
> -/* TEST_SIGNAL(name, signal) { implementation }
> - * Defines a test by name and the expected term signal.
> - * Names must be unique and tests must not be run in parallel. The
> - * implementation containing block is a function and scoping should be
> treated
> - * as such. Returning early may be performed with a bare "return;"
> statement.
> - *
> - * EXPECT_* and ASSERT_* are valid in a TEST() { } context.
> - */
> -#define TEST_SIGNAL TEST_API(TEST_SIGNAL)
> -
> -/* FIXTURE(datatype name) {
> - * type property1;
> - * ...
> - * };
> - * Defines the data provided to TEST_F()-defined tests as |self|. It should
> be
> - * populated and cleaned up using FIXTURE_SETUP and FIXTURE_TEARDOWN.
> - */
> -#define FIXTURE TEST_API(FIXTURE)
> -
> -/* FIXTURE_DATA(datatype name)
> - * This call may be used when the type of the fixture data
> - * is needed. In general, this should not be needed unless
> - * the |self| is being passed to a helper directly.
> - */
> -#define FIXTURE_DATA TEST_API(FIXTURE_DATA)
> -
> -/* FIXTURE_SETUP(fixture name) { implementation }
> - * Populates the required "setup" function for a fixture. An instance of the
> - * datatype defined with _FIXTURE_DATA will be exposed as |self| for the
> - * implementation.
> - *
> - * ASSERT_* are valid for use in this context and will prempt the execution
> - * of any dependent fixture tests.
> - *
> - * A bare "return;" statement may be used to return early.
> - */
> -#define FIXTURE_SETUP TEST_API(FIXTURE_SETUP)
> -
> -/* FIXTURE_TEARDOWN(fixture name) { implementation }
> - * Populates the required "teardown" function for a fixture. An instance of
> the
> - * datatype defined with _FIXTURE_DATA will be exposed as |self| for the
> - * implementation to clean up.
> - *
> - * A bare "return;" statement may be used to return early.
> - */
> -#define FIXTURE_TEARDOWN TEST_API(FIXTURE_TEARDOWN)
> -
> -/* TEST_F(fixture, name) { implementation }
> - * Defines a test that depends on a fixture (e.g., is part of a test case).
> - * Very similar to TEST() except that |self| is the setup instance of
> fixture's
> - * datatype exposed for use by the implementation.
> - */
> -#define TEST_F TEST_API(TEST_F)
> -
> -#define TEST_F_SIGNAL TEST_API(TEST_F_SIGNAL)
> -
> -/* Use once to append a main() to the test file. E.g.,
> - * TEST_HARNESS_MAIN
> - */
> -#define TEST_HARNESS_MAIN TEST_API(TEST_HARNESS_MAIN)
> -
> -/*
> - * Operators for use in TEST and TEST_F.
> - * ASSERT_* calls will stop test execution immediately.
> - * EXPECT_* calls will emit a failure warning, note it, and continue.
> - */
> -
> -/* ASSERT_EQ(expected, measured): expected == measured */
> -#define ASSERT_EQ TEST_API(ASSERT_EQ)
> -/* ASSERT_NE(expected, measured): expected != measured */
> -#define ASSERT_NE TEST_API(ASSERT_NE)
> -/* ASSERT_LT(expected, m
Re: [PATCH v5 7/7] Documentation/dev-tools: Add kselftest_harness documentation
On Fri, May 26, 2017 at 11:44 AM, Mickaël Salaün <m...@digikod.net> wrote:
> Add ReST metadata to kselftest_harness.h to be able to include the
> comments in the Sphinx documentation.
>
> Changes since v4:
> * exclude the TEST_API() changes (requested by Kees Cook)
>
> Changes since v3:
> * document macros as actual functions (suggested by Jonathan Corbet)
> * remove the TEST_API() wrapper to expose the underlying macro arguments
> to the documentation tools
> * move and cleanup comments
>
> Changes since v2:
> * add reference to the full documentation in the header file (suggested
> by Kees Cook)
>
> Signed-off-by: Mickaël Salaün <m...@digikod.net>
> Cc: Andy Lutomirski <l...@amacapital.net>
> Cc: Jonathan Corbet <cor...@lwn.net>
> Cc: Kees Cook <keesc...@chromium.org>
> Cc: Shuah Khan <sh...@kernel.org>
> Cc: Will Drewry <w...@chromium.org>
Awesome, this is great to have. :) I wonder if we have a self-test doc
section yet. Hmmm.
Acked-by: Kees Cook <keesc...@chromium.org>
-Kees
> ---
> Documentation/dev-tools/kselftest.rst | 34 +++
> tools/testing/selftests/kselftest_harness.h | 415
> ++--
> 2 files changed, 364 insertions(+), 85 deletions(-)
>
> diff --git a/Documentation/dev-tools/kselftest.rst
> b/Documentation/dev-tools/kselftest.rst
> index 9232ce94612c..a92fa181b6cf 100644
> --- a/Documentation/dev-tools/kselftest.rst
> +++ b/Documentation/dev-tools/kselftest.rst
> @@ -120,3 +120,37 @@ Contributing new tests (details)
> executable which is not tested by default.
> TEST_FILES, TEST_GEN_FILES mean it is the file which is used by
> test.
> +
> +Test Harness
> +
> +
> +The kselftest_harness.h file contains useful helpers to build tests. The
> tests
> +from tools/testing/selftests/seccomp/seccomp_bpf.c can be used as example.
> +
> +Example
> +---
> +
> +.. kernel-doc:: tools/testing/selftests/kselftest_harness.h
> +:doc: example
> +
> +
> +Helpers
> +---
> +
> +.. kernel-doc:: tools/testing/selftests/kselftest_harness.h
> +:functions: TH_LOG TEST TEST_SIGNAL FIXTURE FIXTURE_DATA FIXTURE_SETUP
> +FIXTURE_TEARDOWN TEST_F TEST_HARNESS_MAIN
> +
> +Operators
> +-
> +
> +.. kernel-doc:: tools/testing/selftests/kselftest_harness.h
> +:doc: operators
> +
> +.. kernel-doc:: tools/testing/selftests/kselftest_harness.h
> +:functions: ASSERT_EQ ASSERT_NE ASSERT_LT ASSERT_LE ASSERT_GT ASSERT_GE
> +ASSERT_NULL ASSERT_TRUE ASSERT_NULL ASSERT_TRUE ASSERT_FALSE
> +ASSERT_STREQ ASSERT_STRNE EXPECT_EQ EXPECT_NE EXPECT_LT
> +EXPECT_LE EXPECT_GT EXPECT_GE EXPECT_NULL EXPECT_TRUE
> +EXPECT_FALSE EXPECT_STREQ EXPECT_STRNE
> +
> diff --git a/tools/testing/selftests/kselftest_harness.h
> b/tools/testing/selftests/kselftest_harness.h
> index 45f807ce37e1..c56f72e07cd7 100644
> --- a/tools/testing/selftests/kselftest_harness.h
> +++ b/tools/testing/selftests/kselftest_harness.h
> @@ -4,41 +4,49 @@
> *
> * kselftest_harness.h: simple C unit test helper.
> *
> - * Usage:
> - * #include "../kselftest_harness.h"
> - * TEST(standalone_test) {
> - * do_some_stuff;
> - * EXPECT_GT(10, stuff) {
> - *stuff_state_t state;
> - *enumerate_stuff_state();
> - *TH_LOG("expectation failed with state: %s", state.msg);
> - * }
> - * more_stuff;
> - * ASSERT_NE(some_stuff, NULL) TH_LOG("how did it happen?!");
> - * last_stuff;
> - * EXPECT_EQ(0, last_stuff);
> - * }
> - *
> - * FIXTURE(my_fixture) {
> - * mytype_t *data;
> - * int awesomeness_level;
> - * };
> - * FIXTURE_SETUP(my_fixture) {
> - * self->data = mytype_new();
> - * ASSERT_NE(NULL, self->data);
> - * }
> - * FIXTURE_TEARDOWN(my_fixture) {
> - * mytype_free(self->data);
> - * }
> - * TEST_F(my_fixture, data_is_good) {
> - * EXPECT_EQ(1, is_my_data_good(self->data));
> - * }
> - *
> - * TEST_HARNESS_MAIN
> + * See documentation in Documentation/dev-tools/kselftest.rst
> *
> * API inspired by code.google.com/p/googletest
> */
>
> +/**
> + * DOC: example
> + *
> + * .. code-block:: c
> + *
> + *#include "../kselftest_harness.h"
> + *
> + *TEST(standalone_test) {
> + * do_some_stuff;
> + * EXPECT_GT(10, stuff) {
> + * stuff_state_t state;
> + * enumerate_stuff_state();
> + * TH_LOG("expectation failed with state: %s", state.msg);
> + * }
> + *
Re: [PATCH v4 6/6] Documentation/dev-tools: Add kselftest_harness documentation
On Thu, May 25, 2017 at 5:20 PM, Mickaël Salaün <m...@digikod.net> wrote:
> Add metadata to kselftest_harness.h to be able to include the comments
> in the Sphinx documentation.
>
> Changes since v3:
> * document macros as actual functions (suggested by Jonathan Corbet)
> * remove the TEST_API() wrapper to expose the underlying macro arguments
> to the documentation tools
> * move and cleanup comments
To aid review, can you actually split this into 2 patches with the
renaming after the removal of TEST_API() in the first and the addition
of the ReST docs in the second? Regardless, it looks good. Thanks!
-Kees
>
> Changes since v2:
> * add reference to the full documentation in the header file (suggested
> by Kees Cook)
>
> Signed-off-by: Mickaël Salaün <m...@digikod.net>
> Acked-by: Kees Cook <keesc...@chromium.org>
> Cc: Andy Lutomirski <l...@amacapital.net>
> Cc: Jonathan Corbet <cor...@lwn.net>
> Cc: Shuah Khan <sh...@kernel.org>
> Cc: Will Drewry <w...@chromium.org>
> ---
> Documentation/dev-tools/kselftest.rst | 34 ++
> tools/testing/selftests/kselftest_harness.h | 678
> ++--
> 2 files changed, 468 insertions(+), 244 deletions(-)
>
> diff --git a/Documentation/dev-tools/kselftest.rst
> b/Documentation/dev-tools/kselftest.rst
> index 9232ce94612c..a92fa181b6cf 100644
> --- a/Documentation/dev-tools/kselftest.rst
> +++ b/Documentation/dev-tools/kselftest.rst
> @@ -120,3 +120,37 @@ Contributing new tests (details)
> executable which is not tested by default.
> TEST_FILES, TEST_GEN_FILES mean it is the file which is used by
> test.
> +
> +Test Harness
> +
> +
> +The kselftest_harness.h file contains useful helpers to build tests. The
> tests
> +from tools/testing/selftests/seccomp/seccomp_bpf.c can be used as example.
> +
> +Example
> +---
> +
> +.. kernel-doc:: tools/testing/selftests/kselftest_harness.h
> +:doc: example
> +
> +
> +Helpers
> +---
> +
> +.. kernel-doc:: tools/testing/selftests/kselftest_harness.h
> +:functions: TH_LOG TEST TEST_SIGNAL FIXTURE FIXTURE_DATA FIXTURE_SETUP
> +FIXTURE_TEARDOWN TEST_F TEST_HARNESS_MAIN
> +
> +Operators
> +-
> +
> +.. kernel-doc:: tools/testing/selftests/kselftest_harness.h
> +:doc: operators
> +
> +.. kernel-doc:: tools/testing/selftests/kselftest_harness.h
> +:functions: ASSERT_EQ ASSERT_NE ASSERT_LT ASSERT_LE ASSERT_GT ASSERT_GE
> +ASSERT_NULL ASSERT_TRUE ASSERT_NULL ASSERT_TRUE ASSERT_FALSE
> +ASSERT_STREQ ASSERT_STRNE EXPECT_EQ EXPECT_NE EXPECT_LT
> +EXPECT_LE EXPECT_GT EXPECT_GE EXPECT_NULL EXPECT_TRUE
> +EXPECT_FALSE EXPECT_STREQ EXPECT_STRNE
> +
> diff --git a/tools/testing/selftests/kselftest_harness.h
> b/tools/testing/selftests/kselftest_harness.h
> index 171e70aead9c..8f623a4e1889 100644
> --- a/tools/testing/selftests/kselftest_harness.h
> +++ b/tools/testing/selftests/kselftest_harness.h
> @@ -4,41 +4,49 @@
> *
> * kselftest_harness.h: simple C unit test helper.
> *
> - * Usage:
> - * #include "../kselftest_harness.h"
> - * TEST(standalone_test) {
> - * do_some_stuff;
> - * EXPECT_GT(10, stuff) {
> - *stuff_state_t state;
> - *enumerate_stuff_state();
> - *TH_LOG("expectation failed with state: %s", state.msg);
> - * }
> - * more_stuff;
> - * ASSERT_NE(some_stuff, NULL) TH_LOG("how did it happen?!");
> - * last_stuff;
> - * EXPECT_EQ(0, last_stuff);
> - * }
> - *
> - * FIXTURE(my_fixture) {
> - * mytype_t *data;
> - * int awesomeness_level;
> - * };
> - * FIXTURE_SETUP(my_fixture) {
> - * self->data = mytype_new();
> - * ASSERT_NE(NULL, self->data);
> - * }
> - * FIXTURE_TEARDOWN(my_fixture) {
> - * mytype_free(self->data);
> - * }
> - * TEST_F(my_fixture, data_is_good) {
> - * EXPECT_EQ(1, is_my_data_good(self->data));
> - * }
> - *
> - * TEST_HARNESS_MAIN
> + * See documentation in Documentation/dev-tools/kselftest.rst
> *
> * API inspired by code.google.com/p/googletest
> */
>
> +/**
> + * DOC: example
> + *
> + * .. code-block:: c
> + *
> + *#include "../kselftest_harness.h"
> + *
> + *TEST(standalone_test) {
> + * do_some_stuff;
> + * EXPECT_GT(10, stuff) {
> + * stuff_state_t state;
> + * enumerate_stuff_state();
> + * TH_LOG("expectation failed with state: %s", state.msg);
> + * }
> + * more_stuff;
> + *
Re: [PATCH 23/31] gcc-plugins.txt: standardize document format
On Thu, May 18, 2017 at 6:22 PM, Mauro Carvalho Chehab <mche...@s-opensource.com> wrote: > Each text file under Documentation follows a different > format. Some doesn't even have titles! > > Change its representation to follow the adopted standard, > using ReST markups for it to be parseable by Sphinx: > > - promote main title; > - use the right markup for footnotes; > - use bold markup for files name; > - identify literal blocks; > - add blank lines to avoid Sphinx to complain; > - remove numeration from titles. > > Signed-off-by: Mauro Carvalho Chehab <mche...@s-opensource.com> Acked-by: Kees Cook <keesc...@chromium.org> This should probably get moved under "Kernel API documentation" but may need a new sub-category, maybe "instrumentation"? Things like KASan could be put under that too. -Kees -- Kees Cook Pixel Security -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 next 1/3] modules:capabilities: allow __request_module() to take a capability argument
On Tue, May 23, 2017 at 3:29 AM, Djalal Harouni <tix...@gmail.com> wrote:
> On Tue, May 23, 2017 at 12:20 AM, Kees Cook <keesc...@chromium.org> wrote:
>> On Mon, May 22, 2017 at 4:57 AM, Djalal Harouni <tix...@gmail.com> wrote:
>>> This is a preparation patch for the module auto-load restriction feature.
>>>
>>> In order to restrict module auto-load operations we need to check if the
>>> caller has CAP_SYS_MODULE capability. This allows to align security
>>> checks of automatic module loading with the checks of the explicit
>>> operations.
>>>
>>> However for "netdev-%s" modules, they are allowed to be loaded if
>>> CAP_NET_ADMIN is set. Therefore, in order to not break this assumption,
>>> and allow userspace to only load "netdev-%s" modules with CAP_NET_ADMIN
>>> capability which is considered a privileged operation, we have two
>>> choices: 1) parse "netdev-%s" alias and check the capability or 2) hand
>>> the capability form request_module() to security_kernel_module_request()
>>> hook and let the capability subsystem decide.
>>>
>>> After a discussion with Rusty Russell [1], the suggestion was to pass
>>> the capability from request_module() to security_kernel_module_request()
>>> for 'netdev-%s' modules that need CAP_NET_ADMIN.
>>>
>>> The patch does not update request_module(), it updates the internal
>>> __request_module() that will take an extra "allow_cap" argument. If
>>> positive, then automatic module load operation can be allowed.
>>
>> I find this refactor slightly confusing. I would expect to collapse
>> the existing caps checks in net/core/dev_ioctl.c and
>> net/ipv4/tcp_cong.c, and make this a "required cap" argument, and to
>> add a new non-__ function instead of requiring callers use
>> __request_module.
>>
>> request_module_capable(int cap_required, fmt, args);
>>
>> adjust __request_module() for the new arg, and when cap_required !=
>> -1, perform a cap check.
>>
>> Then make request_module pass -1 to __request_module(), and change
>> dev_ioctl.c (and tcp_cong.c) from:
>>
>> if (no_module && capable(CAP_NET_ADMIN))
>> no_module = request_module("netdev-%s", name);
>> if (no_module && capable(CAP_SYS_MODULE))
>> request_module("%s", name);
>>
>> to:
>>
>> if (no_module)
>> no_module = request_module_capable(CAP_NET_ADMIN,
>> "netdev-%s", name);
>> if (no_module)
>> no_module = request_module_capable(CAP_SYS_MODULE, "%s",
>> name);
>>
>> that'll make the code cleaner, too.
>
> The refactoring in the patch is more for backward compatibility with
> CAP_NET_ADMIN,
> as discussed here: https://lkml.org/lkml/2017/4/26/147
I think Rusty and I are saying the same thing here, and I must be not
understanding something you're trying to explain. Apologies for being
dense.
> I think if there is an interface request_module_capable() , then code
> will use it. The DCCP code path did not check capabilities at all and
> called request_module(), other code does the same.
>
> A new interface can be abused, the result of this: we may break
> "modules_autoload_mode" in mode 0 and 1. In the long term code will
> want to change may_autoload_module() to also allow mode 1 to load a
> module with CAP_NET_ADMIN or other caps in its own userns, resulting
> in "modules_autoload_mode == 0 == 1". Without userns in the game we
> may just see request_module_capable(CAP_SYS_ADMIN, ...) . There is
> already some code maybe phonet sockets ? that require CAP_SYS_ADMIN to
> get the appropriate protocol and no one will be able to review all
> this code or track new patches with request_module_capable() callers.
I'm having some trouble following what you're saying here, but if I
understand, you're worried about getting the kernel into a state where
autoload state 0 == 1. Autoload 0 is "business as usual", and autoload
1 is "CAP_SYS_MODULE required to be able to trigger a module auto-load
operation, or CAP_NET_ADMIN for modules with a 'netdev-%s' alias."
In the v4 patch, under autoload==1, CAP_NET_ADMIN is needed to load
netdev- modules:
if (no_module && capable(CAP_NET_ADMIN))
no_module = __request_module(true, CAP_NET_ADMIN,
"netdev-%s", name);
and in the LSM hook, CAP_NET_ADMIN is passed as an allowable "alias"
for the CAP_SYS
Re: [kernel-hardening] [PATCH v4 next 0/3] modules: automatic module loading restrictions
On Tue, May 23, 2017 at 12:48 AM, Solar Designer <so...@openwall.com> wrote: > For modules_autoload_mode=2, we already seem to have the equivalent of > modprobe=/bin/true (or does it differ subtly, maybe in return values?), > which I already use at startup on a GPU box like this (preloading > modules so that the OpenCL backends wouldn't need the autoloading): > > nvidia-smi > nvidia-modprobe -u -c=0 > #modprobe nvidia_uvm > #modprobe fglrx > > sysctl -w kernel.modprobe=/bin/true > sysctl -w kernel.hotplug=/bin/true > > but it's good to also have this supported more explicitly and more > consistently through modules_autoload_mode=2 while we're at it. So I > support having this mode as well. I just question the need to have it > non-resettable. I agree it's useful to have the explicit =2 state just to avoid confusion when more systems start implementing CONFIG_STATIC_USERMODEHELPER and kernel.modprobe becomes read-only (though the userspace implementation may allow for some way to disable it, etc). I just like avoiding the upcall to modprobe at all. -Kees -- Kees Cook Pixel Security -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [kernel-hardening] [PATCH v4 next 0/3] modules: automatic module loading restrictions
On Mon, May 22, 2017 at 12:55 PM, Djalal Harouni <tix...@gmail.com> wrote:
> On Mon, May 22, 2017 at 6:43 PM, Solar Designer <so...@openwall.com> wrote:
>> On Mon, May 22, 2017 at 03:49:15PM +0200, Djalal Harouni wrote:
>>> On Mon, May 22, 2017 at 2:08 PM, Solar Designer <so...@openwall.com> wrote:
>>> > On Mon, May 22, 2017 at 01:57:03PM +0200, Djalal Harouni wrote:
>>> >> *) When modules_autoload_mode is set to (2), automatic module loading is
>>> >> disabled for all. Once set, this value can not be changed.
>>> >
>>> > What purpose does this securelevel-like property ("Once set, this value
>>> > can not be changed.") serve here? I think this mode 2 is needed, but
>>> > without this extra property, which is bypassable by e.g. explicitly
>>> > loaded kernel modules anyway (and that's OK).
>>>
>>> My reasoning about "Once set, this value can not be changed" is mainly for:
>>>
>>> If you have some systems where modules are not updated for any given
>>> reason, then the only one who will be able to load a module is an
>>> administrator, basically this is a shortcut for:
>>>
>>> * Apps/services can run with CAP_NET_ADMIN but they are not allowed to
>>> auto-load 'netdev' modules.
>>>
>>> * Explicitly loading modules can be guarded by seccomp filters *per*
>>> app, so even if these apps have
>>> CAP_SYS_MODULE they won't be able to explicitly load modules, one
>>> has to remount some sysctl /proc/ entries read-only here and remove
>>> CAP_SYS_ADMIN for all apps anyway.
>>>
>>> This mainly serves the purpose of these systems that do not receive
>>> updates, if I don't want to expose those kernel interfaces what should
>>> I do ? then if I want to unload old versions and replace them with new
>>> ones what operation should be allowed ? and only real root of the
>>> system can do it. Hence, the "Once set, this value can not be changed"
>>> is more of a shortcut, also the idea was put in my mind based on how
>>> "modules_disabled" is disabled forever, and some other interfaces. I
>>> would say: it is easy to handle a transition from 1) "hey this system
>>> is still up to date, some features should be exposed" to 2) "this
>>> system is not up to date anymore, only root should expose some
>>> features..."
>>>
>>> Hmm, I am not sure if this answers your question ? :-)
>>
>> This answers my question, but in a way that I summarize as "there's no
>> good reason to include this securelevel-like property".
>>
>
> Hmm, sorry I did forget to add in my previous comment that with such
> systems, CAP_SYS_MODULE can be used to reset the
> "modules_autoload_mode" sysctl back from mode 2 to mode 1, even if we
> disable it privileged tasks can be triggered to overwrite the sysctl
> flag and get it back unless /proc is read-only... that's one of the
> points, it should not be so easy to relax it.
I'm on the fence. For modules_disabled and Yama, it was tied to
CAP_SYS_ADMIN, basically designed to be a at-boot setting that could
not later be undone by an attacker gaining that privilege, keeping
them out of either kernel memory or existing user process memory.
Here, it's CAP_SYS_MODULE... it's hard to imagine the situation where
a CAP_SYS_MODULE-capable process could write to this sysctl but NOT
issue direct modprobe requests, but it's _possible_ via crazy symlink
games to trick capable processes into writing to sysctls. We've seen
this multiple times before, and it's a way for attackers to turn a
single privileged write into a privileged exec.
I might turn the question around, though: why would we want to have it
changeable at this setting?
I'm fine leaving that piece off, either way.
-Kees
--
Kees Cook
Pixel Security
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 next 2/3] modules:capabilities: automatic module loading restriction
On Mon, May 22, 2017 at 4:57 AM, Djalal Harouni <tix...@gmail.com> wrote:
> [...]
> diff --git a/kernel/module.c b/kernel/module.c
> index 4a3665f..ce7a146 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -282,6 +282,8 @@ module_param(sig_enforce, bool_enable_only, 0644);
>
> /* Block module loading/unloading? */
> int modules_disabled = 0;
> +int modules_autoload_mode = MODULES_AUTOLOAD_ALLOWED;
> +const int modules_autoload_max = MODULES_AUTOLOAD_DISABLED;
> core_param(nomodule, modules_disabled, bint, 0);
>
> /* Waiting for a module to finish initializing? */
> @@ -4296,6 +4298,46 @@ struct module *__module_text_address(unsigned long
> addr)
> }
> EXPORT_SYMBOL_GPL(__module_text_address);
>
> +/**
> + * may_autoload_module - Determine whether a module auto-load operation
> + * is permitted
> + * @kmod_name: The module name
> + * @allow_cap: if positive, may allow to auto-load the module if this
> capability
> + * is set
> + *
> + * Determine whether a module auto-load operation is allowed or not. The
> check
> + * uses the sysctl "modules_autoload_mode" value.
> + *
> + * This allows to have more control on automatic module loading, and align it
> + * with explicit load/unload module operations. The kernel contains several
> + * modules, some of them are not updated often and may contain bugs and
> + * vulnerabilities.
> + *
> + * The "allow_cap" is passed by callers to explicitly note that the module
> has
> + * the appropriate alias and that the "allow_cap" capability is set. This is
> + * for backward compatibility, the aim is to have a clear picture where:
> + *
> + * 1) Implicit module loading is allowed
> + * 2) Implicit module loading as with the explicit one requires
> CAP_SYS_MODULE.
> + * 3) Implicit module loading as with the explicit one can be disabled.
> + *
> + * Returns 0 if the module request is allowed or -EPERM if not.
> + */
> +int may_autoload_module(char *kmod_name, int allow_cap)
> +{
> + if (modules_autoload_mode == MODULES_AUTOLOAD_ALLOWED)
> + return 0;
> + else if (modules_autoload_mode == MODULES_AUTOLOAD_PRIVILEGED) {
> + /* Check CAP_SYS_MODULE then allow_cap if valid */
> + if (capable(CAP_SYS_MODULE) ||
> + (allow_cap > 0 && capable(allow_cap)))
With the allow_cap check already happening in my suggestion for
__request_module(), it's not needed here. (In fact, it's not even
really needed to plumb this into the hook, I don't think?
Regardless, I remain a fan. :)
-Kees
--
Kees Cook
Pixel Security
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 next 1/3] modules:capabilities: allow __request_module() to take a capability argument
On Mon, May 22, 2017 at 4:57 AM, Djalal Harouni <tix...@gmail.com> wrote:
> This is a preparation patch for the module auto-load restriction feature.
>
> In order to restrict module auto-load operations we need to check if the
> caller has CAP_SYS_MODULE capability. This allows to align security
> checks of automatic module loading with the checks of the explicit operations.
>
> However for "netdev-%s" modules, they are allowed to be loaded if
> CAP_NET_ADMIN is set. Therefore, in order to not break this assumption,
> and allow userspace to only load "netdev-%s" modules with CAP_NET_ADMIN
> capability which is considered a privileged operation, we have two
> choices: 1) parse "netdev-%s" alias and check the capability or 2) hand
> the capability form request_module() to security_kernel_module_request()
> hook and let the capability subsystem decide.
>
> After a discussion with Rusty Russell [1], the suggestion was to pass
> the capability from request_module() to security_kernel_module_request()
> for 'netdev-%s' modules that need CAP_NET_ADMIN.
>
> The patch does not update request_module(), it updates the internal
> __request_module() that will take an extra "allow_cap" argument. If
> positive, then automatic module load operation can be allowed.
I find this refactor slightly confusing. I would expect to collapse
the existing caps checks in net/core/dev_ioctl.c and
net/ipv4/tcp_cong.c, and make this a "required cap" argument, and to
add a new non-__ function instead of requiring callers use
__request_module.
request_module_capable(int cap_required, fmt, args);
adjust __request_module() for the new arg, and when cap_required !=
-1, perform a cap check.
Then make request_module pass -1 to __request_module(), and change
dev_ioctl.c (and tcp_cong.c) from:
if (no_module && capable(CAP_NET_ADMIN))
no_module = request_module("netdev-%s", name);
if (no_module && capable(CAP_SYS_MODULE))
request_module("%s", name);
to:
if (no_module)
no_module = request_module_capable(CAP_NET_ADMIN,
"netdev-%s", name);
if (no_module)
no_module = request_module_capable(CAP_SYS_MODULE, "%s", name);
that'll make the code cleaner, too.
> __request_module() will be only called by networking code which is the
> exception to this, so we do not break userspace and CAP_NET_ADMIN can
> continue to load 'netdev-%s' modules. Other kernel code should continue
> to use request_module() which calls security_kernel_module_request() and
> will check for CAP_SYS_MODULE capability in next patch. Allowing more
> control on who can trigger automatic module loading.
>
> This patch updates security_kernel_module_request() to take the
> 'allow_cap' argument and SELinux which is currently the only user of
> security_kernel_module_request() hook.
>
> Based on patch by Rusty Russell:
> https://lkml.org/lkml/2017/4/26/735
>
> Cc: Serge Hallyn <se...@hallyn.com>
> Cc: Andy Lutomirski <l...@kernel.org>
> Suggested-by: Rusty Russell <ru...@rustcorp.com.au>
> Suggested-by: Kees Cook <keesc...@chromium.org>
> Signed-off-by: Djalal Harouni <tix...@gmail.com>
>
> [1] https://lkml.org/lkml/2017/4/24/7
> ---
> include/linux/kmod.h | 15 ---
> include/linux/lsm_hooks.h | 4 +++-
> include/linux/security.h | 4 ++--
> kernel/kmod.c | 15 +--
> net/core/dev_ioctl.c | 10 +-
> security/security.c | 4 ++--
> security/selinux/hooks.c | 2 +-
> 7 files changed, 38 insertions(+), 16 deletions(-)
>
> diff --git a/include/linux/kmod.h b/include/linux/kmod.h
> index c4e441e..a314432 100644
> --- a/include/linux/kmod.h
> +++ b/include/linux/kmod.h
> @@ -32,18 +32,19 @@
> extern char modprobe_path[]; /* for sysctl */
> /* modprobe exit status on success, -ve on error. Return value
> * usually useless though. */
> -extern __printf(2, 3)
> -int __request_module(bool wait, const char *name, ...);
> -#define request_module(mod...) __request_module(true, mod)
> -#define request_module_nowait(mod...) __request_module(false, mod)
> +extern __printf(3, 4)
> +int __request_module(bool wait, int allow_cap, const char *name, ...);
> #define try_then_request_module(x, mod...) \
> - ((x) ?: (__request_module(true, mod), (x)))
> + ((x) ?: (__request_module(true, -1, mod), (x)))
> #else
> -static inline int request_module(const char *name, ...) { return -ENOSYS; }
> -static inline int request_module_nowait(const char *name, ...) { return
> -ENOSYS; }
> +static inline __printf(3, 4)
> +int __request_module(bool wait, int allow_cap, const ch
Re: [PATCH 00/17] convert/reorganize Documentation/security/
On Thu, May 18, 2017 at 9:49 AM, Jonathan Corbet <cor...@lwn.net> wrote: > On Sat, 13 May 2017 04:51:36 -0700 > Kees Cook <keesc...@chromium.org> wrote: > >> This ReSTifies everything under Documentation/security/, and reorganizes >> some of it (mainly the LSMs) under /admin-guide/ per Jon's request. Since >> /security/ is already being indexed under the kernel development portion >> of the sphinx index, I didn't move it, keeping only things that were >> directly related to internal kernel development (keys, creds, etc). >> >> I also updated some path references, and MAINTAINERS lines. Some of the >> conversion could probably do with some tweaks, but I think this is a >> good first step in the right direction. > > I agree with the need for tweaks :) I fixed up key-request.rst a bit > since I just couldn't stand it. > > Anyway, the set is applied, many thanks for doing this! Awesome, thanks for the fixes! -Kees -- Kees Cook Pixel Security -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 00/17] convert/reorganize Documentation/security/
On Mon, May 15, 2017 at 10:26 AM, Jonathan Corbet <cor...@lwn.net> wrote: > On Sat, 13 May 2017 04:51:36 -0700 > Kees Cook <keesc...@chromium.org> wrote: > >> This ReSTifies everything under Documentation/security/, and reorganizes >> some of it (mainly the LSMs) under /admin-guide/ per Jon's request. Since >> /security/ is already being indexed under the kernel development portion >> of the sphinx index, I didn't move it, keeping only things that were >> directly related to internal kernel development (keys, creds, etc). >> >> I also updated some path references, and MAINTAINERS lines. Some of the >> conversion could probably do with some tweaks, but I think this is a >> good first step in the right direction. > > This all looks pretty good to me, though I'll confess I haven't actually > built the resulting docs yet. Assuming no issues turn up there, I'd be > happy to just apply these and let any follow-on tweaks go from there. > Thanks for doing this, and for humoring me on the organizational issues :) My local tree builds the docs sanely from what I can see, so if it looks good to you too, yeah, please take these as they are. I'm sure we'll need tweaks going forward, but this seems like the bulk of the organizational and basic ReST work. BTW, something I noticed in while doing this conversion is the difference between the section headings and the left-side nav bar at the top level: https://www.kernel.org/doc/html/latest/index.html The section names don't match, and each of the Kernel API Documentation sections is at the top level in the nav bar. I think this would be cleaner if everything matched up, but I didn't yet dig into figuring out why they were different. -Kees -- Kees Cook Pixel Security -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 06/17] doc: security: minor cleanups to build kernel-doc
On Sun, May 14, 2017 at 5:00 PM, Casey Schaufler <ca...@schaufler-ca.com> wrote: > On 5/13/2017 4:51 AM, Kees Cook wrote: >> These fixes were needed to parse lsm_hooks.h kernel-doc. More work is >> needed, but this is the first step. >> >> Cc: Casey Schaufler <ca...@schaufler-ca.com> >> Signed-off-by: Kees Cook <keesc...@chromium.org> > > Acked_by: Casey Schaufler <ca...@schaufler-ca.com> > > Tell me more about the additional work that's needed. What I wanted to do was insert the kernel-doc from lsm_hooks.h into the LSM kernel API documentation .rst file (via the special ReST markup that includes structure documentation). There is, however, free-form text in the existing union security_list_options kernel-doc to announce related function groups which ReST just kind of skips over and the collects all at the end in the HTML output. It also orders the HTML doc output by the struct ordering, which makes things even stranger to parse. And additionally, kernel-doc for fields that are function pointers is especially hard to read. So, while this patch fixes the kernel-doc to at least be parsed without errors, it doesn't really fix the overall appearance, which I'm not sure how to fix yet. It might be possible to do per-struct "/** DOC:" markup, but I decided to leave that for another pass in the future. If there is a v2 of this series, I'll update the changelog to include these details. :) -Kees >> --- >> include/linux/lsm_hooks.h | 25 - >> 1 file changed, 12 insertions(+), 13 deletions(-) >> >> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h >> index 080f34e66017..a1eeaf603d2f 100644 >> --- a/include/linux/lsm_hooks.h >> +++ b/include/linux/lsm_hooks.h >> @@ -29,6 +29,8 @@ >> #include >> >> /** >> + * union security_list_options - Linux Security Module hook function list >> + * >> * Security hooks for program execution operations. >> * >> * @bprm_set_creds: >> @@ -193,8 +195,8 @@ >> * @value will be set to the allocated attribute value. >> * @len will be set to the length of the value. >> * Returns 0 if @name and @value have been successfully set, >> - * -EOPNOTSUPP if no security attribute is needed, or >> - * -ENOMEM on memory allocation failure. >> + * -EOPNOTSUPP if no security attribute is needed, or >> + * -ENOMEM on memory allocation failure. >> * @inode_create: >> * Check permission to create a regular file. >> * @dir contains inode structure of the parent of the new file. >> @@ -510,8 +512,7 @@ >> * process @tsk. Note that this hook is sometimes called from interrupt. >> * Note that the fown_struct, @fown, is never outside the context of a >> * struct file, so the file structure (and associated security >> information) >> - * can always be obtained: >> - * container_of(fown, struct file, f_owner) >> + * can always be obtained: container_of(fown, struct file, f_owner) >> * @tsk contains the structure of task receiving signal. >> * @fown contains the file owner information. >> * @sig is the signal that will be sent. When 0, kernel sends SIGIO. >> @@ -521,7 +522,7 @@ >> * to receive an open file descriptor via socket IPC. >> * @file contains the file structure being received. >> * Return 0 if permission is granted. >> - * @file_open >> + * @file_open: >> * Save open-time permission checking state for later use upon >> * file_permission, and recheck access if anything has changed >> * since inode_permission. >> @@ -1143,7 +1144,7 @@ >> * @sma contains the semaphore structure. May be NULL. >> * @cmd contains the operation to be performed. >> * Return 0 if permission is granted. >> - * @sem_semop >> + * @sem_semop: >> * Check permissions before performing operations on members of the >> * semaphore set @sma. If the @alter flag is nonzero, the semaphore set >> * may be modified. >> @@ -1153,20 +1154,20 @@ >> * @alter contains the flag indicating whether changes are to be made. >> * Return 0 if permission is granted. >> * >> - * @binder_set_context_mgr >> + * @binder_set_context_mgr: >> * Check whether @mgr is allowed to be the binder context manager. >> * @mgr contains the task_struct for the task being registered. >> * Return 0 if permission is granted. >> - * @binder_transaction >> + * @binder_transaction: >> * Check whether @from is allowed to invoke a binder transaction call >> * to @to. >>
[PATCH 03/17] doc: ReSTify IMA-templates.txt
Adjust IMA-templates.txt for ReST markup and add to the index for
security/, under the Kernel API Documentation.
Cc: Mimi Zohar <zo...@linux.vnet.ibm.com>
Signed-off-by: Kees Cook <keesc...@chromium.org>
---
Documentation/security/00-INDEX| 2 -
.../{IMA-templates.txt => IMA-templates.rst} | 46 --
Documentation/security/index.rst | 4 +-
3 files changed, 29 insertions(+), 23 deletions(-)
rename Documentation/security/{IMA-templates.txt => IMA-templates.rst} (72%)
diff --git a/Documentation/security/00-INDEX b/Documentation/security/00-INDEX
index 45c82fd3e9d3..414235c1fcfc 100644
--- a/Documentation/security/00-INDEX
+++ b/Documentation/security/00-INDEX
@@ -22,5 +22,3 @@ keys.txt
- description of the kernel key retention service.
tomoyo.txt
- documentation on the TOMOYO Linux Security Module.
-IMA-templates.txt
- - documentation on the template management mechanism for IMA.
diff --git a/Documentation/security/IMA-templates.txt
b/Documentation/security/IMA-templates.rst
similarity index 72%
rename from Documentation/security/IMA-templates.txt
rename to Documentation/security/IMA-templates.rst
index 839b5dad9226..2cd0e273cc9a 100644
--- a/Documentation/security/IMA-templates.txt
+++ b/Documentation/security/IMA-templates.rst
@@ -1,9 +1,12 @@
- IMA Template Management Mechanism
+=
+IMA Template Management Mechanism
+=
- INTRODUCTION
+Introduction
+
-The original 'ima' template is fixed length, containing the filedata hash
+The original ``ima`` template is fixed length, containing the filedata hash
and pathname. The filedata hash is limited to 20 bytes (md5/sha1).
The pathname is a null terminated string, limited to 255 characters.
To overcome these limitations and to add additional file metadata, it is
@@ -28,61 +31,64 @@ a new data type, developers define the field identifier and
implement
two functions, init() and show(), respectively to generate and display
measurement entries. Defining a new template descriptor requires
specifying the template format (a string of field identifiers separated
-by the '|' character) through the 'ima_template_fmt' kernel command line
+by the ``|`` character) through the ``ima_template_fmt`` kernel command line
parameter. At boot time, IMA initializes the chosen template descriptor
by translating the format into an array of template fields structures taken
from the set of the supported ones.
-After the initialization step, IMA will call ima_alloc_init_template()
+After the initialization step, IMA will call ``ima_alloc_init_template()``
(new function defined within the patches for the new template management
mechanism) to generate a new measurement entry by using the template
descriptor chosen through the kernel configuration or through the newly
-introduced 'ima_template' and 'ima_template_fmt' kernel command line
parameters.
+introduced ``ima_template`` and ``ima_template_fmt`` kernel command line
parameters.
It is during this phase that the advantages of the new architecture are
clearly shown: the latter function will not contain specific code to handle
-a given template but, instead, it simply calls the init() method of the
template
+a given template but, instead, it simply calls the ``init()`` method of the
template
fields associated to the chosen template descriptor and store the result
(pointer to allocated data and data length) in the measurement entry structure.
The same mechanism is employed to display measurements entries.
-The functions ima[_ascii]_measurements_show() retrieve, for each entry,
+The functions ``ima[_ascii]_measurements_show()`` retrieve, for each entry,
the template descriptor used to produce that entry and call the show()
method for each item of the array of template fields structures.
- SUPPORTED TEMPLATE FIELDS AND DESCRIPTORS
+Supported Template Fields and Descriptors
+=
In the following, there is the list of supported template fields
-('': description), that can be used to define new template
+``('': description)``, that can be used to define new template
descriptors by adding their identifier to the format string
(support for more data types will be added later):
- 'd': the digest of the event (i.e. the digest of a measured file),
-calculated with the SHA1 or MD5 hash algorithm;
+ calculated with the SHA1 or MD5 hash algorithm;
- 'n': the name of the event (i.e. the file name), with size up to 255 bytes;
- 'd-ng': the digest of the event, calculated with an arbitrary hash
- algorithm (field format: [:]digest, where the digest
- prefix is shown only if the hash algorithm is not SHA1 or MD5);
+ algorithm (field format: [:]digest, where the digest
+ prefix is shown only if the hash algorithm
[PATCH 04/17] doc: ReSTify credentials.txt
This updates the credentials API documentation to ReST markup and moves
it under the security subsection of kernel API documentation.
Cc: David Howells <dhowe...@redhat.com>
Signed-off-by: Kees Cook <keesc...@chromium.org>
---
Documentation/security/00-INDEX| 2 -
.../security/{credentials.txt => credentials.rst} | 275 ++---
Documentation/security/index.rst | 1 +
include/linux/cred.h | 2 +-
kernel/cred.c | 2 +-
5 files changed, 127 insertions(+), 155 deletions(-)
rename Documentation/security/{credentials.txt => credentials.rst} (72%)
diff --git a/Documentation/security/00-INDEX b/Documentation/security/00-INDEX
index 414235c1fcfc..c4df62a9ae5b 100644
--- a/Documentation/security/00-INDEX
+++ b/Documentation/security/00-INDEX
@@ -10,8 +10,6 @@ Yama.txt
- documentation on the Yama Linux Security Module.
apparmor.txt
- documentation on the AppArmor security extension.
-credentials.txt
- - documentation about credentials in Linux.
keys-ecryptfs.txt
- description of the encryption keys for the ecryptfs filesystem.
keys-request-key.txt
diff --git a/Documentation/security/credentials.txt
b/Documentation/security/credentials.rst
similarity index 72%
rename from Documentation/security/credentials.txt
rename to Documentation/security/credentials.rst
index 86257052e31a..038a7e19eff9 100644
--- a/Documentation/security/credentials.txt
+++ b/Documentation/security/credentials.rst
@@ -1,38 +1,18 @@
-
-CREDENTIALS IN LINUX
-
+
+Credentials in Linux
+
By: David Howells <dhowe...@redhat.com>
-Contents:
-
- (*) Overview.
-
- (*) Types of credentials.
-
- (*) File markings.
-
- (*) Task credentials.
+.. contents:: :local:
- - Immutable credentials.
- - Accessing task credentials.
- - Accessing another task's credentials.
- - Altering credentials.
- - Managing credentials.
-
- (*) Open file credentials.
-
- (*) Overriding the VFS's use of credentials.
-
-
-
-OVERVIEW
+Overview
There are several parts to the security check performed by Linux when one
object acts upon another:
- (1) Objects.
+ 1. Objects.
Objects are things in the system that may be acted upon directly by
userspace programs. Linux has a variety of actionable objects, including:
@@ -48,7 +28,7 @@ object acts upon another:
As a part of the description of all these objects there is a set of
credentials. What's in the set depends on the type of object.
- (2) Object ownership.
+ 2. Object ownership.
Amongst the credentials of most objects, there will be a subset that
indicates the ownership of that object. This is used for resource
@@ -57,7 +37,7 @@ object acts upon another:
In a standard UNIX filesystem, for instance, this will be defined by the
UID marked on the inode.
- (3) The objective context.
+ 3. The objective context.
Also amongst the credentials of those objects, there will be a subset that
indicates the 'objective context' of that object. This may or may not be
@@ -67,7 +47,7 @@ object acts upon another:
The objective context is used as part of the security calculation that is
carried out when an object is acted upon.
- (4) Subjects.
+ 4. Subjects.
A subject is an object that is acting upon another object.
@@ -77,10 +57,10 @@ object acts upon another:
Objects other than tasks may under some circumstances also be subjects.
For instance an open file may send SIGIO to a task using the UID and EUID
- given to it by a task that called fcntl(F_SETOWN) upon it. In this case,
+ given to it by a task that called ``fcntl(F_SETOWN)`` upon it. In this
case,
the file struct will have a subjective context too.
- (5) The subjective context.
+ 5. The subjective context.
A subject has an additional interpretation of its credentials. A subset
of its credentials forms the 'subjective context'. The subjective context
@@ -92,7 +72,7 @@ object acts upon another:
from the real UID and GID that normally form the objective context of the
task.
- (6) Actions.
+ 6. Actions.
Linux has a number of actions available that a subject may perform upon an
object. The set of actions available depends on the nature of the subject
@@ -101,7 +81,7 @@ object acts upon another:
Actions include reading, writing, creating and deleting files; forking or
signalling and tracing tasks.
- (7) Rules, access control lists and security calculations.
+ 7. Rules, access control lists and security calculations.
When a subject acts upon an object, a security calculation is made. This
involves takin
[PATCH 06/17] doc: security: minor cleanups to build kernel-doc
These fixes were needed to parse lsm_hooks.h kernel-doc. More work is
needed, but this is the first step.
Cc: Casey Schaufler <ca...@schaufler-ca.com>
Signed-off-by: Kees Cook <keesc...@chromium.org>
---
include/linux/lsm_hooks.h | 25 -
1 file changed, 12 insertions(+), 13 deletions(-)
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 080f34e66017..a1eeaf603d2f 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -29,6 +29,8 @@
#include
/**
+ * union security_list_options - Linux Security Module hook function list
+ *
* Security hooks for program execution operations.
*
* @bprm_set_creds:
@@ -193,8 +195,8 @@
* @value will be set to the allocated attribute value.
* @len will be set to the length of the value.
* Returns 0 if @name and @value have been successfully set,
- * -EOPNOTSUPP if no security attribute is needed, or
- * -ENOMEM on memory allocation failure.
+ * -EOPNOTSUPP if no security attribute is needed, or
+ * -ENOMEM on memory allocation failure.
* @inode_create:
* Check permission to create a regular file.
* @dir contains inode structure of the parent of the new file.
@@ -510,8 +512,7 @@
* process @tsk. Note that this hook is sometimes called from interrupt.
* Note that the fown_struct, @fown, is never outside the context of a
* struct file, so the file structure (and associated security information)
- * can always be obtained:
- * container_of(fown, struct file, f_owner)
+ * can always be obtained: container_of(fown, struct file, f_owner)
* @tsk contains the structure of task receiving signal.
* @fown contains the file owner information.
* @sig is the signal that will be sent. When 0, kernel sends SIGIO.
@@ -521,7 +522,7 @@
* to receive an open file descriptor via socket IPC.
* @file contains the file structure being received.
* Return 0 if permission is granted.
- * @file_open
+ * @file_open:
* Save open-time permission checking state for later use upon
* file_permission, and recheck access if anything has changed
* since inode_permission.
@@ -1143,7 +1144,7 @@
* @sma contains the semaphore structure. May be NULL.
* @cmd contains the operation to be performed.
* Return 0 if permission is granted.
- * @sem_semop
+ * @sem_semop:
* Check permissions before performing operations on members of the
* semaphore set @sma. If the @alter flag is nonzero, the semaphore set
* may be modified.
@@ -1153,20 +1154,20 @@
* @alter contains the flag indicating whether changes are to be made.
* Return 0 if permission is granted.
*
- * @binder_set_context_mgr
+ * @binder_set_context_mgr:
* Check whether @mgr is allowed to be the binder context manager.
* @mgr contains the task_struct for the task being registered.
* Return 0 if permission is granted.
- * @binder_transaction
+ * @binder_transaction:
* Check whether @from is allowed to invoke a binder transaction call
* to @to.
* @from contains the task_struct for the sending task.
* @to contains the task_struct for the receiving task.
- * @binder_transfer_binder
+ * @binder_transfer_binder:
* Check whether @from is allowed to transfer a binder reference to @to.
* @from contains the task_struct for the sending task.
* @to contains the task_struct for the receiving task.
- * @binder_transfer_file
+ * @binder_transfer_file:
* Check whether @from is allowed to transfer @file to @to.
* @from contains the task_struct for the sending task.
* @file contains the struct file being transferred.
@@ -1214,7 +1215,7 @@
* @cred contains the credentials to use.
* @ns contains the user namespace we want the capability in
* @cap contains the capability .
- * @audit: Whether to write an audit message or not
+ * @audit contains whether to write an audit message or not
* Return 0 if the capability is granted for @tsk.
* @syslog:
* Check permission before accessing the kernel message ring or changing
@@ -1336,9 +1337,7 @@
* @inode we wish to get the security context of.
* @ctx is a pointer in which to place the allocated security context.
* @ctxlen points to the place to put the length of @ctx.
- * This is the main security structure.
*/
-
union security_list_options {
int (*binder_set_context_mgr)(struct task_struct *mgr);
int (*binder_transaction)(struct task_struct *from,
--
2.7.4
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 02/17] doc: ReSTify no_new_privs.txt
This updates no_new_privs documentation to ReST markup and adds it to
the user-space API documentation.
Signed-off-by: Kees Cook <keesc...@chromium.org>
---
Documentation/userspace-api/index.rst | 1 +
.../no_new_privs.rst} | 44 --
2 files changed, 26 insertions(+), 19 deletions(-)
rename Documentation/{prctl/no_new_privs.txt =>
userspace-api/no_new_privs.rst} (54%)
diff --git a/Documentation/userspace-api/index.rst
b/Documentation/userspace-api/index.rst
index 15ff12342db8..7b2eb1b7d4ca 100644
--- a/Documentation/userspace-api/index.rst
+++ b/Documentation/userspace-api/index.rst
@@ -16,6 +16,7 @@ place where this information is gathered.
.. toctree::
:maxdepth: 2
+ no_new_privs
seccomp_filter
unshare
diff --git a/Documentation/prctl/no_new_privs.txt
b/Documentation/userspace-api/no_new_privs.rst
similarity index 54%
rename from Documentation/prctl/no_new_privs.txt
rename to Documentation/userspace-api/no_new_privs.rst
index f7be84fba910..d060ea217ea1 100644
--- a/Documentation/prctl/no_new_privs.txt
+++ b/Documentation/userspace-api/no_new_privs.rst
@@ -1,3 +1,7 @@
+==
+No New Privileges Flag
+==
+
The execve system call can grant a newly-started program privileges that
its parent did not have. The most obvious examples are setuid/setgid
programs and file capabilities. To prevent the parent program from
@@ -5,53 +9,55 @@ gaining these privileges as well, the kernel and user code
must be
careful to prevent the parent from doing anything that could subvert the
child. For example:
- - The dynamic loader handles LD_* environment variables differently if
+ - The dynamic loader handles ``LD_*`` environment variables differently if
a program is setuid.
- chroot is disallowed to unprivileged processes, since it would allow
- /etc/passwd to be replaced from the point of view of a process that
+ ``/etc/passwd`` to be replaced from the point of view of a process that
inherited chroot.
- The exec code has special handling for ptrace.
-These are all ad-hoc fixes. The no_new_privs bit (since Linux 3.5) is a
+These are all ad-hoc fixes. The ``no_new_privs`` bit (since Linux 3.5) is a
new, generic mechanism to make it safe for a process to modify its
execution environment in a manner that persists across execve. Any task
-can set no_new_privs. Once the bit is set, it is inherited across fork,
-clone, and execve and cannot be unset. With no_new_privs set, execve
+can set ``no_new_privs``. Once the bit is set, it is inherited across fork,
+clone, and execve and cannot be unset. With ``no_new_privs`` set, ``execve()``
promises not to grant the privilege to do anything that could not have
been done without the execve call. For example, the setuid and setgid
bits will no longer change the uid or gid; file capabilities will not
add to the permitted set, and LSMs will not relax constraints after
execve.
-To set no_new_privs, use prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0).
+To set ``no_new_privs``, use::
+
+prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0);
Be careful, though: LSMs might also not tighten constraints on exec
-in no_new_privs mode. (This means that setting up a general-purpose
-service launcher to set no_new_privs before execing daemons may
+in ``no_new_privs`` mode. (This means that setting up a general-purpose
+service launcher to set ``no_new_privs`` before execing daemons may
interfere with LSM-based sandboxing.)
-Note that no_new_privs does not prevent privilege changes that do not
-involve execve. An appropriately privileged task can still call
-setuid(2) and receive SCM_RIGHTS datagrams.
+Note that ``no_new_privs`` does not prevent privilege changes that do not
+involve ``execve()``. An appropriately privileged task can still call
+``setuid(2)`` and receive SCM_RIGHTS datagrams.
-There are two main use cases for no_new_privs so far:
+There are two main use cases for ``no_new_privs`` so far:
- Filters installed for the seccomp mode 2 sandbox persist across
execve and can change the behavior of newly-executed programs.
Unprivileged users are therefore only allowed to install such filters
- if no_new_privs is set.
+ if ``no_new_privs`` is set.
- - By itself, no_new_privs can be used to reduce the attack surface
+ - By itself, ``no_new_privs`` can be used to reduce the attack surface
available to an unprivileged user. If everything running with a
- given uid has no_new_privs set, then that uid will be unable to
+ given uid has ``no_new_privs`` set, then that uid will be unable to
escalate its privileges by directly attacking setuid, setgid, and
fcap-using binaries; it will need to compromise something without the
- no_new_privs bit set first.
+ ``no_new_privs`` bit set first.
In the future, other potentially dangerous kernel features could become
-available to unprivile
[PATCH 11/17] doc: ReSTify Yama.txt
Adjusts for ReST markup and moves under LSM admin guide.
Signed-off-by: Kees Cook <keesc...@chromium.org>
---
.../Yama.txt => admin-guide/LSM/Yama.rst} | 55 --
Documentation/admin-guide/LSM/index.rst| 1 +
Documentation/security/00-INDEX| 2 -
MAINTAINERS| 1 +
security/yama/Kconfig | 3 +-
5 files changed, 33 insertions(+), 29 deletions(-)
rename Documentation/{security/Yama.txt => admin-guide/LSM/Yama.rst} (60%)
diff --git a/Documentation/security/Yama.txt
b/Documentation/admin-guide/LSM/Yama.rst
similarity index 60%
rename from Documentation/security/Yama.txt
rename to Documentation/admin-guide/LSM/Yama.rst
index d9ee7d7a6c7f..13468ea696b7 100644
--- a/Documentation/security/Yama.txt
+++ b/Documentation/admin-guide/LSM/Yama.rst
@@ -1,13 +1,14 @@
+
+Yama
+
+
Yama is a Linux Security Module that collects system-wide DAC security
protections that are not handled by the core kernel itself. This is
-selectable at build-time with CONFIG_SECURITY_YAMA, and can be controlled
-at run-time through sysctls in /proc/sys/kernel/yama:
-
-- ptrace_scope
+selectable at build-time with ``CONFIG_SECURITY_YAMA``, and can be controlled
+at run-time through sysctls in ``/proc/sys/kernel/yama``:
-==
-
-ptrace_scope:
+ptrace_scope
+
As Linux grows in popularity, it will become a larger target for
malware. One particularly troubling weakness of the Linux process
@@ -25,47 +26,49 @@ exist and remain possible if ptrace is allowed to operate
as before.
Since ptrace is not commonly used by non-developers and non-admins, system
builders should be allowed the option to disable this debugging system.
-For a solution, some applications use prctl(PR_SET_DUMPABLE, ...) to
+For a solution, some applications use ``prctl(PR_SET_DUMPABLE, ...)`` to
specifically disallow such ptrace attachment (e.g. ssh-agent), but many
do not. A more general solution is to only allow ptrace directly from a
parent to a child process (i.e. direct "gdb EXE" and "strace EXE" still
-work), or with CAP_SYS_PTRACE (i.e. "gdb --pid=PID", and "strace -p PID"
+work), or with ``CAP_SYS_PTRACE`` (i.e. "gdb --pid=PID", and "strace -p PID"
still work as root).
In mode 1, software that has defined application-specific relationships
between a debugging process and its inferior (crash handlers, etc),
-prctl(PR_SET_PTRACER, pid, ...) can be used. An inferior can declare which
-other process (and its descendants) are allowed to call PTRACE_ATTACH
+``prctl(PR_SET_PTRACER, pid, ...)`` can be used. An inferior can declare which
+other process (and its descendants) are allowed to call ``PTRACE_ATTACH``
against it. Only one such declared debugging process can exists for
each inferior at a time. For example, this is used by KDE, Chromium, and
Firefox's crash handlers, and by Wine for allowing only Wine processes
to ptrace each other. If a process wishes to entirely disable these ptrace
-restrictions, it can call prctl(PR_SET_PTRACER, PR_SET_PTRACER_ANY, ...)
+restrictions, it can call ``prctl(PR_SET_PTRACER, PR_SET_PTRACER_ANY, ...)``
so that any otherwise allowed process (even those in external pid namespaces)
may attach.
-The sysctl settings (writable only with CAP_SYS_PTRACE) are:
+The sysctl settings (writable only with ``CAP_SYS_PTRACE``) are:
-0 - classic ptrace permissions: a process can PTRACE_ATTACH to any other
+0 - classic ptrace permissions:
+a process can ``PTRACE_ATTACH`` to any other
process running under the same uid, as long as it is dumpable (i.e.
did not transition uids, start privileged, or have called
-prctl(PR_SET_DUMPABLE...) already). Similarly, PTRACE_TRACEME is
+``prctl(PR_SET_DUMPABLE...)`` already). Similarly, ``PTRACE_TRACEME`` is
unchanged.
-1 - restricted ptrace: a process must have a predefined relationship
-with the inferior it wants to call PTRACE_ATTACH on. By default,
+1 - restricted ptrace:
+a process must have a predefined relationship
+with the inferior it wants to call ``PTRACE_ATTACH`` on. By default,
this relationship is that of only its descendants when the above
classic criteria is also met. To change the relationship, an
-inferior can call prctl(PR_SET_PTRACER, debugger, ...) to declare
-an allowed debugger PID to call PTRACE_ATTACH on the inferior.
-Using PTRACE_TRACEME is unchanged.
+inferior can call ``prctl(PR_SET_PTRACER, debugger, ...)`` to declare
+an allowed debugger PID to call ``PTRACE_ATTACH`` on the inferior.
+Using ``PTRACE_TRACEME`` is unchanged.
-2 - admin-only attach: only processes with CAP_SYS_PTRACE may use ptrace
-with PTRACE_ATTACH, or through children calling PTRACE_TRACEME.
+2 - admin-only attach:
+only pr
[PATCH 13/17] doc: ReSTify Smack.txt
Adjusts for ReST markup and moves under LSM admin guide.
Cc: Casey Schaufler <ca...@schaufler-ca.com>
Signed-off-by: Kees Cook <keesc...@chromium.org>
---
.../Smack.txt => admin-guide/LSM/Smack.rst}| 273 ++---
Documentation/admin-guide/LSM/index.rst| 1 +
Documentation/security/00-INDEX| 2 -
MAINTAINERS| 2 +-
4 files changed, 191 insertions(+), 87 deletions(-)
rename Documentation/{security/Smack.txt => admin-guide/LSM/Smack.rst} (85%)
diff --git a/Documentation/security/Smack.txt
b/Documentation/admin-guide/LSM/Smack.rst
similarity index 85%
rename from Documentation/security/Smack.txt
rename to Documentation/admin-guide/LSM/Smack.rst
index 945cc633d883..6a5826a13aea 100644
--- a/Documentation/security/Smack.txt
+++ b/Documentation/admin-guide/LSM/Smack.rst
@@ -1,3 +1,6 @@
+=
+Smack
+=
"Good for you, you've decided to clean the elevator!"
@@ -14,6 +17,7 @@ available to determine which is best suited to the problem
at hand.
Smack consists of three major components:
+
- The kernel
- Basic utilities, which are helpful but not required
- Configuration data
@@ -39,16 +43,24 @@ The current git repository for Smack user space is:
This should make and install on most modern distributions.
There are five commands included in smackutil:
-chsmack- display or set Smack extended attribute values
-smackctl - load the Smack access rules
-smackaccess - report if a process with one label has access
- to an object with another
+chsmack:
+ display or set Smack extended attribute values
+
+smackctl:
+ load the Smack access rules
+
+smackaccess:
+ report if a process with one label has access
+ to an object with another
These two commands are obsolete with the introduction of
the smackfs/load2 and smackfs/cipso2 interfaces.
-smackload - properly formats data for writing to smackfs/load
-smackcipso - properly formats data for writing to smackfs/cipso
+smackload:
+ properly formats data for writing to smackfs/load
+
+smackcipso:
+ properly formats data for writing to smackfs/cipso
In keeping with the intent of Smack, configuration data is
minimal and not strictly required. The most important
@@ -56,15 +68,15 @@ configuration step is mounting the smackfs pseudo
filesystem.
If smackutil is installed the startup script will take care
of this, but it can be manually as well.
-Add this line to /etc/fstab:
+Add this line to ``/etc/fstab``::
smackfs /sys/fs/smackfs smackfs defaults 0 0
-The /sys/fs/smackfs directory is created by the kernel.
+The ``/sys/fs/smackfs`` directory is created by the kernel.
Smack uses extended attributes (xattrs) to store labels on filesystem
objects. The attributes are stored in the extended attribute security
-name space. A process must have CAP_MAC_ADMIN to change any of these
+name space. A process must have ``CAP_MAC_ADMIN`` to change any of these
attributes.
The extended attributes that Smack uses are:
@@ -73,14 +85,17 @@ SMACK64
Used to make access control decisions. In almost all cases
the label given to a new filesystem object will be the label
of the process that created it.
+
SMACK64EXEC
The Smack label of a process that execs a program file with
this attribute set will run with this attribute's value.
+
SMACK64MMAP
Don't allow the file to be mmapped by a process whose Smack
label does not allow all of the access permitted to a process
with the label contained in this attribute. This is a very
specific use case for shared libraries.
+
SMACK64TRANSMUTE
Can only have the value "TRUE". If this attribute is present
on a directory when an object is created in the directory and
@@ -89,27 +104,29 @@ SMACK64TRANSMUTE
gets the label of the directory instead of the label of the
creating process. If the object being created is a directory
the SMACK64TRANSMUTE attribute is set as well.
+
SMACK64IPIN
This attribute is only available on file descriptors for sockets.
Use the Smack label in this attribute for access control
decisions on packets being delivered to this socket.
+
SMACK64IPOUT
This attribute is only available on file descriptors for sockets.
Use the Smack label in this attribute for access control
decisions on packets coming from this socket.
-There are multiple ways to set a Smack label on a file:
+There are multiple ways to set a Smack label on a file::
# attr -S -s SMACK64 -V "value" path
# chsmack -a value path
A process can see the Smack label it is running with by
-reading /proc/self/attr/current. A process with CAP_MAC_ADMIN
+reading ``/proc/self/attr/current``. A process with ``CAP_MAC_ADMIN``
can set the process Sma
[PATCH 14/17] doc: ReSTify keys.txt
This creates a new section in the security development index for kernel
keys, and adjusts for ReST markup.
Cc: David Howells <dhowe...@redhat.com>
Signed-off-by: Kees Cook <keesc...@chromium.org>
---
Documentation/crypto/asymmetric-keys.txt | 2 +-
Documentation/security/00-INDEX| 2 -
Documentation/security/index.rst | 1 +
Documentation/security/{keys.txt => keys/core.rst} | 314 ++---
Documentation/security/keys/index.rst | 8 +
MAINTAINERS| 2 +-
include/linux/key.h| 2 +-
7 files changed, 163 insertions(+), 168 deletions(-)
rename Documentation/security/{keys.txt => keys/core.rst} (89%)
create mode 100644 Documentation/security/keys/index.rst
diff --git a/Documentation/crypto/asymmetric-keys.txt
b/Documentation/crypto/asymmetric-keys.txt
index 5ad6480e3fb9..b82b6ad48488 100644
--- a/Documentation/crypto/asymmetric-keys.txt
+++ b/Documentation/crypto/asymmetric-keys.txt
@@ -265,7 +265,7 @@ mandatory:
The caller passes a pointer to the following struct with all of the fields
cleared, except for data, datalen and quotalen [see
- Documentation/security/keys.txt].
+ Documentation/security/keys/core.rst].
struct key_preparsed_payload {
char*description;
diff --git a/Documentation/security/00-INDEX b/Documentation/security/00-INDEX
index cdb2294ec047..a840095bb11c 100644
--- a/Documentation/security/00-INDEX
+++ b/Documentation/security/00-INDEX
@@ -6,5 +6,3 @@ keys-request-key.txt
- description of the kernel key request service.
keys-trusted-encrypted.txt
- info on the Trusted and Encrypted keys in the kernel key ring service.
-keys.txt
- - description of the kernel key retention service.
diff --git a/Documentation/security/index.rst b/Documentation/security/index.rst
index 94ba1cfc01c5..298a94a33f05 100644
--- a/Documentation/security/index.rst
+++ b/Documentation/security/index.rst
@@ -7,6 +7,7 @@ Security Documentation
credentials
IMA-templates
+ keys/index
LSM
self-protection
tpm/index
diff --git a/Documentation/security/keys.txt
b/Documentation/security/keys/core.rst
similarity index 89%
rename from Documentation/security/keys.txt
rename to Documentation/security/keys/core.rst
index cd5019934d7f..0d831a7afe4f 100644
--- a/Documentation/security/keys.txt
+++ b/Documentation/security/keys/core.rst
@@ -1,6 +1,6 @@
-
-KERNEL KEY RETENTION SERVICE
-
+
+Kernel Key Retention Service
+
This service allows cryptographic keys, authentication tokens, cross-domain
user mappings, and similar to be cached in the kernel for the use of
@@ -29,8 +29,7 @@ This document has the following sections:
- Garbage collection
-
-KEY OVERVIEW
+Key Overview
In this context, keys represent units of cryptographic data, authentication
@@ -47,14 +46,14 @@ Each key has a number of attributes:
- State.
- (*) Each key is issued a serial number of type key_serial_t that is unique for
+ * Each key is issued a serial number of type key_serial_t that is unique for
the lifetime of that key. All serial numbers are positive non-zero 32-bit
integers.
Userspace programs can use a key's serial numbers as a way to gain access
to it, subject to permission checking.
- (*) Each key is of a defined "type". Types must be registered inside the
+ * Each key is of a defined "type". Types must be registered inside the
kernel by a kernel service (such as a filesystem) before keys of that type
can be added or used. Userspace programs cannot define new types directly.
@@ -64,18 +63,18 @@ Each key has a number of attributes:
Should a type be removed from the system, all the keys of that type will
be invalidated.
- (*) Each key has a description. This should be a printable string. The key
+ * Each key has a description. This should be a printable string. The key
type provides an operation to perform a match between the description on a
key and a criterion string.
- (*) Each key has an owner user ID, a group ID and a permissions mask. These
+ * Each key has an owner user ID, a group ID and a permissions mask. These
are used to control what a process may do to a key from userspace, and
whether a kernel service will be able to find the key.
- (*) Each key can be set to expire at a specific time by the key type's
+ * Each key can be set to expire at a specific time by the key type's
instantiation function. Keys can also be immortal.
- (*) Each key can have a payload. This is a quantity of data that represent
Re: [PATCH v4 1/2] tiocsti-restrict : Add owner user namespace to tty_struct
On Wed, May 3, 2017 at 1:02 PM, Matt Brown <m...@nmatt.com> wrote: > On 05/03/2017 03:45 PM, Greg KH wrote: >> >> On Wed, May 03, 2017 at 12:32:07PM -0700, Kees Cook wrote: >>> >>> On Mon, Apr 24, 2017 at 6:57 AM, Serge E. Hallyn <se...@hallyn.com> >>> wrote: >>>> >>>> Quoting Matt Brown (m...@nmatt.com): >>>>> >>>>> This patch adds struct user_namespace *owner_user_ns to the tty_struct. >>>>> Then it is set to current_user_ns() in the alloc_tty_struct function. >>>>> >>>>> This is done to facilitate capability checks against the original user >>>>> namespace that allocated the tty. >>>>> >>>>> E.g. ns_capable(tty->owner_user_ns,CAP_SYS_ADMIN) >>>>> >>>>> This combined with the use of user namespace's will allow hardening >>>>> protections to be built to mitigate container escapes that utilize TTY >>>>> ioctls such as TIOCSTI. >>>>> >>>>> See: https://bugzilla.redhat.com/show_bug.cgi?id=1411256 >>>>> >>>>> Signed-off-by: Matt Brown <m...@nmatt.com> >>>> >>>> >>>> Acked-by: Serge Hallyn <se...@hallyn.com> >>> >>> >>> This Ack didn't end up in the v5, but I think it stands, yes? >>> >>> Greg, is the v5 okay to pull for you or would a v6 with Acks/Reviews >>> included be preferred? >> >> >> v6 would be great, and we are dropping patch 2 from the series, right? >> I was expecting this to be resent. I'll start looking at new patches >> like this after 4.12-rc1 is out. >> > > I will create a v6 with the Acks/Reviews. I'd like to keep patch 2 in > since that got acked by at least Serge. (Kees also? or just patch 1?) Sorry, I meant that patch 2's ack from serge got dropped accidentally. i.e. he Acked v4, but it wasn't in v5. Serge, just to double-check, does your Ack stand? -Kees -- Kees Cook Pixel Security -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 1/2] tiocsti-restrict : Add owner user namespace to tty_struct
On Mon, Apr 24, 2017 at 6:57 AM, Serge E. Hallyn <se...@hallyn.com> wrote:
> Quoting Matt Brown (m...@nmatt.com):
>> This patch adds struct user_namespace *owner_user_ns to the tty_struct.
>> Then it is set to current_user_ns() in the alloc_tty_struct function.
>>
>> This is done to facilitate capability checks against the original user
>> namespace that allocated the tty.
>>
>> E.g. ns_capable(tty->owner_user_ns,CAP_SYS_ADMIN)
>>
>> This combined with the use of user namespace's will allow hardening
>> protections to be built to mitigate container escapes that utilize TTY
>> ioctls such as TIOCSTI.
>>
>> See: https://bugzilla.redhat.com/show_bug.cgi?id=1411256
>>
>> Signed-off-by: Matt Brown <m...@nmatt.com>
>
> Acked-by: Serge Hallyn <se...@hallyn.com>
This Ack didn't end up in the v5, but I think it stands, yes?
Greg, is the v5 okay to pull for you or would a v6 with Acks/Reviews
included be preferred?
-Kees
>
>> ---
>> drivers/tty/tty_io.c | 2 ++
>> include/linux/tty.h | 2 ++
>> 2 files changed, 4 insertions(+)
>>
>> diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
>> index e6d1a65..c276814 100644
>> --- a/drivers/tty/tty_io.c
>> +++ b/drivers/tty/tty_io.c
>> @@ -171,6 +171,7 @@ static void free_tty_struct(struct tty_struct *tty)
>> put_device(tty->dev);
>> kfree(tty->write_buf);
>> tty->magic = 0xDEADDEAD;
>> + put_user_ns(tty->owner_user_ns);
>> kfree(tty);
>> }
>>
>> @@ -3191,6 +3192,7 @@ struct tty_struct *alloc_tty_struct(struct tty_driver
>> *driver, int idx)
>> tty->index = idx;
>> tty_line_name(driver, idx, tty->name);
>> tty->dev = tty_get_device(tty);
>> + tty->owner_user_ns = get_user_ns(current_user_ns());
>>
>> return tty;
>> }
>> diff --git a/include/linux/tty.h b/include/linux/tty.h
>> index 1017e904..d902d42 100644
>> --- a/include/linux/tty.h
>> +++ b/include/linux/tty.h
>> @@ -12,6 +12,7 @@
>> #include
>> #include
>> #include
>> +#include
>>
>>
>> /*
>> @@ -333,6 +334,7 @@ struct tty_struct {
>> /* If the tty has a pending do_SAK, queue it here - akpm */
>> struct work_struct SAK_work;
>> struct tty_port *port;
>> + struct user_namespace *owner_user_ns;
>> };
>>
>> /* Each of a tty's open files has private_data pointing to tty_file_private
>> */
>> --
>> 2.10.2
--
Kees Cook
Pixel Security
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 0/2] security: tty: make TIOCSTI ioctl require CAP_SYS_ADMIN
On Mon, Apr 24, 2017 at 9:15 PM, Matt Brown <m...@nmatt.com> wrote: > This patchset introduces the tiocsti_restrict sysctl, whose default is > controlled via CONFIG_SECURITY_TIOCSTI_RESTRICT. When activated, this > control restricts all TIOCSTI ioctl calls from non CAP_SYS_ADMIN users. > > This patch was inspired from GRKERNSEC_HARDEN_TTY. > > This patch would have prevented > https://bugzilla.redhat.com/show_bug.cgi?id=1411256 under the following > conditions: > * non-privileged container > * container run inside new user namespace > > Possible effects on userland: > > There could be a few user programs that would be effected by this > change. > See: <https://codesearch.debian.net/search?q=ioctl%5C%28.*TIOCSTI> > notable programs are: agetty, csh, xemacs and tcsh > > However, I still believe that this change is worth it given that the > Kconfig defaults to n. This will be a feature that is turned on for the > same reason that people activate it when using grsecurity. Users of this > opt-in feature will realize that they are choosing security over some OS > features like unprivileged TIOCSTI ioctls, as should be clear in the > Kconfig help message. > > Threat Model/Patch Rational: > > From grsecurity's config for GRKERNSEC_HARDEN_TTY. > > | There are very few legitimate uses for this functionality and it > | has made vulnerabilities in several 'su'-like programs possible in > | the past. Even without these vulnerabilities, it provides an > | attacker with an easy mechanism to move laterally among other > | processes within the same user's compromised session. > > So if one process within a tty session becomes compromised it can follow > that additional processes, that are thought to be in different security > boundaries, can be compromised as a result. When using a program like su > or sudo, these additional processes could be in a tty session where TTY file > descriptors are indeed shared over privilege boundaries. > > This is also an excellent writeup about the issue: > <http://www.halfdog.net/Security/2012/TtyPushbackPrivilegeEscalation/> > > When user namespaces are in use, the check for the capability > CAP_SYS_ADMIN is done against the user namespace that originally opened > the tty. This looks like it's ready to go. Greg, can you include this in your tree? That seems like the best place, even though it touches a few areas. Please consider it: Reviewed-by: Kees Cook <keesc...@chromium.org> Thanks! -Kees > > # Changes since v4: > * fixed typo > > # Changes since v3: > * use get_user_ns and put_user_ns to take and drop references to the owner > user namespace because CONFIG_USER_NS is an option > > # Changes since v2: > * take/drop reference to user namespace on tty struct alloc/free to prevent > use-after-free. > > # Changes since v1: > * added owner_user_ns to tty_struct to enable capability checks against > the namespace that created the tty. > * rewording in different places to make patchset purpose clear > * Added Documentation -- Kees Cook Pixel Security -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 0/6] Add kselftest_harness.h
On Tue, May 2, 2017 at 3:26 PM, Mickaël Salaün <m...@digikod.net> wrote:
> Hi,
>
> This second patch series make the seccomp/test_harness.h more generally
> available [1] and update the kselftest documentation with the Sphinx format.
> It
> also improve the Makefile of seccomp tests to take into account any
> kselftest_harness.h update.
>
> [1]
> https://lkml.kernel.org/r/CAGXu5j+8CVz8vL51DRYXqOY=xc3zuKFf=ptene88xyhzfyi...@mail.gmail.com
>
> Regards,
>
> Mickaël Salaün (6):
> selftests: Make test_harness.h more generally available
> selftests: Cosmetic renames in kselftest_harness.h
> selftests/seccomp: Force rebuild according to dependencies
> Documentation/dev-tools: Add kselftest
For these four:
Acked-by: Kees Cook <keesc...@chromium.org>
> Documentation/dev-tools: Use reStructuredText markups for kselftest
> Documentation/dev-tools: Add kselftest_harness documentation
These two have some minor nits I emailed about, but if those are
fixed, consider them Acked-by me too.
Thanks for this!
-Kees
>
> Documentation/00-INDEX | 2 -
> Documentation/dev-tools/index.rst | 1 +
> .../{kselftest.txt => dev-tools/kselftest.rst} | 134 +--
> MAINTAINERS| 1 +
> .../test_harness.h => kselftest_harness.h} | 268
> +++--
> tools/testing/selftests/seccomp/Makefile | 2 +
> tools/testing/selftests/seccomp/seccomp_bpf.c | 2 +-
> 7 files changed, 307 insertions(+), 103 deletions(-)
> rename Documentation/{kselftest.txt => dev-tools/kselftest.rst} (52%)
> rename tools/testing/selftests/{seccomp/test_harness.h =>
> kselftest_harness.h} (81%)
>
> --
> 2.11.0
>
--
Kees Cook
Pixel Security
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 6/6] Documentation/dev-tools: Add kselftest_harness documentation
On Tue, May 2, 2017 at 3:26 PM, Mickaël Salaün <m...@digikod.net> wrote:
> Add metadata to kselftest_harness.h to be able to include the comments
> in the Sphinx documentation.
>
> Signed-off-by: Mickaël Salaün <m...@digikod.net>
> Cc: Andy Lutomirski <l...@amacapital.net>
> Cc: Jonathan Corbet <cor...@lwn.net>
> Cc: Kees Cook <keesc...@chromium.org>
> Cc: Shuah Khan <sh...@kernel.org>
> Cc: Will Drewry <w...@chromium.org>
> ---
> Documentation/dev-tools/kselftest.rst | 57 ++
> tools/testing/selftests/kselftest_harness.h | 259
>
> 2 files changed, 243 insertions(+), 73 deletions(-)
>
> diff --git a/Documentation/dev-tools/kselftest.rst
> b/Documentation/dev-tools/kselftest.rst
> index 39af2cb3d248..cf1e1f262c44 100644
> --- a/Documentation/dev-tools/kselftest.rst
> +++ b/Documentation/dev-tools/kselftest.rst
> @@ -138,3 +138,60 @@ Contributing new tests (details)
> executable which is not tested by default.
> TEST_FILES, TEST_GEN_FILES mean it is the file which is used by
> test.
> +
> +Test Harness
> +
> +
> +The *kselftest_harness.h* file contains useful helpers to build tests. The
> +tests from seccomp/seccomp_bpf.c can be used as examples.
> +
> +Example
> +~~~
> +
> +.. code-block:: c
> +
> +#include "../kselftest_harness.h"
> +
> +TEST(standalone_test) {
> + do_some_stuff;
> + EXPECT_GT(10, stuff) {
> + stuff_state_t state;
> + enumerate_stuff_state();
> + TH_LOG("expectation failed with state: %s", state.msg);
> + }
> + more_stuff;
> + ASSERT_NE(some_stuff, NULL) TH_LOG("how did it happen?!");
> + last_stuff;
> + EXPECT_EQ(0, last_stuff);
> +}
> +
> +FIXTURE(my_fixture) {
> + mytype_t *data;
> + int awesomeness_level;
> +};
> +FIXTURE_SETUP(my_fixture) {
> + self->data = mytype_new();
> + ASSERT_NE(NULL, self->data);
> +}
> +FIXTURE_TEARDOWN(my_fixture) {
> + mytype_free(self->data);
> +}
> +TEST_F(my_fixture, data_is_good) {
> + EXPECT_EQ(1, is_my_data_good(self->data));
> +}
> +
> +TEST_HARNESS_MAIN
> +
> +
> +Helpers
> +~
> +
> +.. kernel-doc:: tools/testing/selftests/kselftest_harness.h
> +:doc: helpers
> +
> +
> +Operators
> +~
> +
> +.. kernel-doc:: tools/testing/selftests/kselftest_harness.h
> +:doc: operators
Awesome! Yay docs!
> diff --git a/tools/testing/selftests/kselftest_harness.h
> b/tools/testing/selftests/kselftest_harness.h
> index 8ba227db46aa..efe50c80 100644
> --- a/tools/testing/selftests/kselftest_harness.h
> +++ b/tools/testing/selftests/kselftest_harness.h
> @@ -4,38 +4,6 @@
> *
> * kselftest_harness.h: simple C unit test helper.
> *
> - * Usage:
Can you add a pointer to the .rst file so someone looking at the
header file will find out where to get an example?
Otherwise, looks great!
-Kees
> - * #include "../kselftest_harness.h"
> - * TEST(standalone_test) {
> - * do_some_stuff;
> - * EXPECT_GT(10, stuff) {
> - *stuff_state_t state;
> - *enumerate_stuff_state();
> - *TH_LOG("expectation failed with state: %s", state.msg);
> - * }
> - * more_stuff;
> - * ASSERT_NE(some_stuff, NULL) TH_LOG("how did it happen?!");
> - * last_stuff;
> - * EXPECT_EQ(0, last_stuff);
> - * }
> - *
> - * FIXTURE(my_fixture) {
> - * mytype_t *data;
> - * int awesomeness_level;
> - * };
> - * FIXTURE_SETUP(my_fixture) {
> - * self->data = mytype_new();
> - * ASSERT_NE(NULL, self->data);
> - * }
> - * FIXTURE_TEARDOWN(my_fixture) {
> - * mytype_free(self->data);
> - * }
> - * TEST_F(my_fixture, data_is_good) {
> - * EXPECT_EQ(1, is_my_data_good(self->data));
> - * }
> - *
> - * TEST_HARNESS_MAIN
> - *
> * API inspired by code.google.com/p/googletest
> */
>
> @@ -58,7 +26,13 @@
> * Exported APIs
> */
>
> -/* TEST(name) { implementation }
> +/**
> + * DOC: helpers
> + *
> + * .. code-block:: c
> + *
> + * TEST(name) { implementation }
> + *
> * Defines a test by name.
> * Names must be unique and tests must not be run in parallel. The
> * implementation containing block is a function and scoping should be
> treated
> @@ -68,7 +42,13 @@
> */
> #define TEST TEST_API(TEST)
>
> -/* TEST_SIGNAL(name, signal) { implementation }
> +/**
> + * DOC: helpers
> + *
> + * .. code-block
Re: [PATCH v2 5/6] Documentation/dev-tools: Use reStructuredText markups for kselftest
-- > > Kselftest install as well as the Kselftest tarball provide a script > named "run_kselftest.sh" to run the tests. > @@ -79,11 +108,13 @@ named "run_kselftest.sh" to run the tests. > You can simply do the following to run the installed Kselftests. Please > note some tests will require root privileges. > > -cd kselftest > -./run_kselftest.sh > +.. code-block:: sh > + > +cd kselftest > +./run_kselftest.sh > > Contributing new tests > -== > +-- > > In general, the rules for selftests are > > @@ -96,8 +127,8 @@ In general, the rules for selftests are > * Don't cause the top-level "make run_tests" to fail if your feature is > unconfigured. > > -Contributing new tests(details) > -=== > +Contributing new tests (details) > + > > * Use TEST_GEN_XXX if such binaries or files are generated during > compiling. > -- > 2.11.0 > -Kees -- Kees Cook Pixel Security -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
ReST style guide?
Hi, While doing some .txt conversions to .rst, I ended up with several questions about markup style, and in looking at existing .rst files, there didn't seem to be a common methodology. I'd love to have definitive guide to how to do these things in the kernel documentation: - how to mark and link to CONFIG items? (e.g. CONFIG_HARDENED_USERCOPY) - how to mark and link to kernel command line arguments? (e.g. loadpin.enabled=1) - how to mark and link to functions? (e.g. put_seccomp()) - how to mark and link to fields? (e.g. siginfo->si_arch) - how to mark and link to defines/literals? (e.g PR_SET_NO_NEW_PRIVS) - how to mark and link to sysctls? (e.g. net.syn_cookies) - how to mark and link to manpage-look-up-able things? (e.g. setuid(2)) - how to mark internal filepaths? (e.g. samples/seccomp/) - how to mark external filepaths? (e.g. /etc/passwd) - how to mark environment variables (e.g. LD_*) It seems most aren't explicitly marked up in existing docs. Sometimes functions are wrapped in `` marks, same for pathnames. Any opinions would be appreciated. :) -- Kees Cook Pixel Security -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: converting Documentation/security/* to .rst
On Mon, May 1, 2017 at 8:11 AM, Jonathan Corbet <cor...@lwn.net> wrote:
> On Fri, 28 Apr 2017 13:24:36 -0700
> Kees Cook <keesc...@google.com> wrote:
>
>> I was curious if the conversion of security/ (and prctl/ which only
>> has two files that should probably both be moved to security/) was
>> already on someone's TODO list? I'd love to get these done (I refer
>> people regularly to seccomp_filter.txt and self-protection.txt), but I
>> didn't want to duplicate any efforts.
>
> If anybody is working in that area, they've not told me about it.
I might take a shot at this, time permitting.
>> I read about various tools to help with auto-converting files to kind
>> of help speed up the process, but I couldn't find what seemed a
>> canonical answer to what to use as a helper. Is there one? (Perhaps
>> this was only for DocBook?)
>
> The tools, such as they are, are for the conversion of DocBook template
> files. Most of the kernel's .txt files are already in something quite
> close to RST already, so the conversion is trivial.
>
> The real question would be one of organization. Most of the security
> stuff looks like it properly belongs in the admin guide, but that's not
> universally the case.
Are the index area "purposes" documented anywhere? The admin guide
seems to cover things outside of "administration" (like reporting
security bugs, which is a developer/researcher activity usually),
There's already a top-level "security documentation" with some TPM
stuff in it.
Both things in prctl/ are "here's what this feature is and how to use
it", both exposed to userspace. In security/ there is a mix of LSM
highlevel descriptions and basic usage, key API documentation, and the
one sort of design goal document ("self-protection.txt").
I think it'd make sense to keep Security Documentation as a top-level
index for now, and create LSM and keys subsections for those items,
and then move prctl/* under security:
deleted:Documentation/security/00-INDEX
deleted:Documentation/security/conf.py
renamed:Documentation/security/IMA-templates.txt ->
Documentation/security/IMA-templates.rst
renamed:Documentation/security/credentials.txt ->
Documentation/security/credentials.rst
renamed:Documentation/security/keys-ecryptfs.txt ->
Documentation/security/keys/ecryptfs.rst
renamed:Documentation/security/keys.txt ->
Documentation/security/keys/index.rst
renamed:Documentation/security/keys-request-key.txt ->
Documentation/security/keys/request-key.rst
renamed:Documentation/security/keys-trusted-encrypted.txt
-> Documentation/security/keys/trusted-encrypted.rst
renamed:Documentation/security/LoadPin.txt ->
Documentation/security/lsm/LoadPin.rst
renamed:Documentation/security/SELinux.txt ->
Documentation/security/lsm/SELinux.rst
renamed:Documentation/security/Smack.txt ->
Documentation/security/lsm/Smack.rst
renamed:Documentation/security/Yama.txt ->
Documentation/security/lsm/Yama.rst
renamed:Documentation/security/apparmor.txt ->
Documentation/security/lsm/apparmor.rst
renamed:Documentation/security/LSM.txt ->
Documentation/security/lsm/index.rst
renamed:Documentation/security/tomoyo.txt ->
Documentation/security/lsm/tomoyo.rst
renamed:Documentation/prctl/no_new_privs.txt ->
Documentation/security/no_new_privs.rst
renamed:Documentation/prctl/seccomp_filter.txt ->
Documentation/security/seccomp_filter.rst
renamed:Documentation/security/self-protection.txt ->
Documentation/security/self-protection.rst
modified: Documentation/security/index.rst
This is just renames and an update to security/index.rst to include
the two new subdirs. This doesn't have any formatting updates. (What
is preferred, organizational changes first or .rst formatting first?)
Does this looks sensible?
What do LSM authors think of this? (e.g. various questions: should
LSM.txt become lsm/index.rst and keys.txt become keys/index.rst? Maybe
key "LSM" case for the new subdirectory? Should IMA be under LSM?)
Thanks!
-Kees
--
Kees Cook
Pixel Security
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 0/3] dm: boot a mapped device without an initramfs
On Tue, Apr 18, 2017 at 9:42 AM, Enric Balletbo i Serra <enric.balle...@collabora.com> wrote: > Hello, > > Some of these patches were send few years back, I saw that first > version was send to this list in 2010, and after version 4 did not > land [1]. Some days ago I resend the patches [2] and few hours later I > noticed that one year ago was send a v5 version [3] and I was not aware. > > There was some discussion about v5 and during the discussion Mike Snitzer > proposed that at least a change of the syntax is required, we're really > interested on see this upstream as is extensively used in ChromeOS based > devices so I'm wondering if we can restart the discussion and hopefully > we will be able to do the modifications needed. > > So my first question is, apart of the change of the syntax, what more > should be changed? AFAIK, this was the main change needed. Change the syntax and plumb into the ioctl interface. The discussion ended with Mike being open to the idea, and for me to go work on it. I haven't had time to work on it, though, so it has continued to be a locally carried patch: https://www.redhat.com/archives/dm-devel/2016-February/msg00199.html More recently David Zeuthen has been poking at this code, so I've included him on CC here, in case there are new developments. -Kees > > Thanks for your help, > Enric > > [1] Patchwork links: > https://patchwork.kernel.org/patch/104857/ > https://patchwork.kernel.org/patch/104856/ > https://patchwork.kernel.org/patch/104858/ > > [2] https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1375276.html > > [3] https://www.redhat.com/archives/dm-devel/2016-February/msg00112.html > > > Brian Norris (1): > dm: make some mapped_device functions available > > Will Drewry (2): > dm: export a table+mapped device to the ioctl interface > init: add support to directly boot to a mapped device > > Documentation/admin-guide/kernel-parameters.rst | 1 + > Documentation/admin-guide/kernel-parameters.txt | 3 + > Documentation/device-mapper/boot.txt| 65 > drivers/md/dm-ioctl.c | 36 ++ > drivers/md/dm.h | 8 - > include/linux/device-mapper.h | 19 + > init/Makefile | 1 + > init/do_mounts.c| 1 + > init/do_mounts.h| 10 + > init/do_mounts_dm.c | 448 > > 10 files changed, 584 insertions(+), 8 deletions(-) > create mode 100644 Documentation/device-mapper/boot.txt > create mode 100644 init/do_mounts_dm.c > > -- > 2.9.3 > -- Kees Cook Pixel Security -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] hibernation: on 32-bit x86, disabled in favor of KASLR
On Sat, Mar 25, 2017 at 7:54 AM, Evgenii Shatokhin
<eugene.shatok...@yandex.ru> wrote:
> On 23.03.2017 18:30, Rafael J. Wysocki wrote:
>>
>> On Thu, Mar 23, 2017 at 2:23 PM, Evgenii Shatokhin
>> <eugene.shatok...@yandex.ru> wrote:
>>>
>>> On 23.03.2017 03:27, Kees Cook wrote:
>>>>
>>>>
>>>> This is a modified revert of commit 65fe935dd238 ("x86/KASLR, x86/power:
>>>> Remove x86 hibernation restrictions"), since it appears that 32-bit
>>>> hibernation still can't support KASLR. 64-bit is fine. Since people have
>>>> been running with KASLR by default on 32-bit since v4.8, this disables
>>>> hibernation (with a warning). Booting with "nokaslr" will disable KASLR
>>>> and enable hibernation.
>>>>
>>>> Reported-by: Evgenii Shatokhin <eugene.shatok...@yandex.ru>
>>>> Signed-off-by: Kees Cook <keesc...@chromium.org>
>>>> Cc: sta...@vger.kernel.org # v4.8+
>>>
>>>
>>>
>>> The patch does not work as intended on my system, unfortunately.
>>>
>>> I tried the mainline kernel v4.11-rc3 and added this patch. With
>>> "nokaslr"
>>> in the kernel command line, the system fails to hibernate. It complains
>>> this
>>> way in the log:
>>>
>>> <...>
>>> kernel: PM: writing image.
>>> kernel: PM: Cannot find swap device, try swapon -a.
>>> kernel: PM: Cannot get swap writer
>>> kernel: PM: Basic memory bitmaps freed
>>> kernel: Restarting tasks ... done.
>>> systemd[1]: Time has been changed
>>> systemd[3948]: Time has been changed
>>> systemd[14825]: Time has been changed
>>> systemd[1]: systemd-hibernate.service: main process exited, code=exited,
>>> status=1/FAILURE
>>> systemd[1]: Failed to start Hibernate.
>>> <...>
>>>
>>> The swap device (swap file, actually) is available, however:
>>> -
>>> # swapon -s
>>> Filename Type SizeUsed Priority
>>> /swap file 6297596 0 -1
>>> -
>>>
>>> I built the same kernel without this patch then, added "nokaslr" in the
>>> kernel command line again, and the system hibernates and resumes fine.
>>
>>
>> With the patch applied and "nokaslr" in the kernel command line, what
>> shows up when you do
>>
>> $ cat /sys/power/state
>>
>> ?
>
>
> freeze standby mem disk
>
> However, I think now that the patch itself is OK.
>
> I experimented with the patched kernel a bit more and found that hibernate
> does work when I place "nokaslr" before "resume=xxx resume_offset=xxx" in
> the kernel command line and does not work when I place "nokaslr" after these
> options. So I guess there is an issue with parsing of the kernel command
> line somewhere (dracut scripts? systemd? I do not know). If resume= or
> resume_offset= were corrupted, that might have been the reason why the
> system could not find the swap file when hibernating.
>
> Anyway, that issue is clearly unrelated to this patch and the patch itself
> works OK for me.
>
> Thanks a lot!
>
> Tested-by: Evgenii Shatokhin <eugene.shatok...@yandex.ru>
Ah, right. Hm, that is kind of the fault of the patch (and the prior
disabling too). Let me see if I can find a better solution...
-Kees
--
Kees Cook
Pixel Security
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] hibernation: on 32-bit x86, disabled in favor of KASLR
This is a modified revert of commit 65fe935dd238 ("x86/KASLR, x86/power:
Remove x86 hibernation restrictions"), since it appears that 32-bit
hibernation still can't support KASLR. 64-bit is fine. Since people have
been running with KASLR by default on 32-bit since v4.8, this disables
hibernation (with a warning). Booting with "nokaslr" will disable KASLR
and enable hibernation.
Reported-by: Evgenii Shatokhin <eugene.shatok...@yandex.ru>
Signed-off-by: Kees Cook <keesc...@chromium.org>
Cc: sta...@vger.kernel.org # v4.8+
---
Documentation/admin-guide/kernel-parameters.txt | 5 +
arch/x86/boot/compressed/kaslr.c| 3 +++
kernel/power/hibernate.c| 18 +-
3 files changed, 25 insertions(+), 1 deletion(-)
diff --git a/Documentation/admin-guide/kernel-parameters.txt
b/Documentation/admin-guide/kernel-parameters.txt
index 2ba45caabada..6f899c7f587d 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -1725,6 +1725,11 @@
kernel and module base offset ASLR (Address Space
Layout Randomization).
+ On 32-bit x86 with CONFIG_HIBERNATION, hibernation
+ is disabled if KASLR is enabled. If "nokaslr" is
+ specified, KASLR will be diabled and hibernation
+ will be enabled.
+
keepinitrd [HW,ARM]
kernelcore= [KNL,X86,IA-64,PPC]
diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
index 8b7c9e75edcb..b694af45f1e0 100644
--- a/arch/x86/boot/compressed/kaslr.c
+++ b/arch/x86/boot/compressed/kaslr.c
@@ -572,6 +572,9 @@ void choose_random_location(unsigned long input,
return;
}
+ if (IS_ENABLED(CONFIG_X86_32) && IS_ENABLED(CONFIG_HIBERNATION))
+ warn("KASLR active: hibernation disabled on 32-bit x86.");
+
boot_params->hdr.loadflags |= KASLR_FLAG;
/* Prepare to add new identity pagetables on demand. */
diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
index a8b978c35a6a..1d8f1fe1b7f4 100644
--- a/kernel/power/hibernate.c
+++ b/kernel/power/hibernate.c
@@ -37,9 +37,14 @@
#include "power.h"
-static int nocompress;
+#if defined(CONFIG_X86_32) && defined(CONFIG_RANDOMIZE_BASE)
+static int noresume = 1;
+static int nohibernate = 1;
+#else
static int noresume;
static int nohibernate;
+#endif
+static int nocompress;
static int resume_wait;
static unsigned int resume_delay;
static char resume_file[256] = CONFIG_PM_STD_PARTITION;
@@ -1194,3 +1199,14 @@ __setup("hibernate=", hibernate_setup);
__setup("resumewait", resumewait_setup);
__setup("resumedelay=", resumedelay_setup);
__setup("nohibernate", nohibernate_setup);
+
+/* Allow hibernation to be disabled in favor of KASLR on 32-bit x86. */
+#if defined(CONFIG_X86_32) && defined(CONFIG_RANDOMIZE_BASE)
+static int __init nokaslr_hibernate_setup(char *str)
+{
+ noresume = 0;
+ nohibernate = 0;
+ return 1;
+}
+__setup("nokaslr", nokaslr_hibernate_setup);
+#endif
--
2.7.4
--
Kees Cook
Pixel Security
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] gcc-plugins: update architecture list in documentation
On Mon, Mar 20, 2017 at 1:39 AM, Michael Ellerman <m...@ellerman.id.au> wrote:
> Andrew Donnellan <andrew.donnel...@au1.ibm.com> writes:
>
>> Commit 65c059bcaa73 ("powerpc: Enable support for GCC plugins") enabled GCC
>> plugins on powerpc, but neglected to update the architecture list in the
>> docs. Rectify this.
>>
>> Fixes: 65c059bcaa73 ("powerpc: Enable support for GCC plugins")
>> Signed-off-by: Andrew Donnellan <andrew.donnel...@au1.ibm.com>
>> ---
>> Documentation/gcc-plugins.txt | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> It would be nice to merge this for v4.11, so the docs are up to do date
> with the code for the release.
>
> I'm not sure who owns it though, should it go via Jon as docs
> maintainer (added to CC), or via Kees tree or mine?
If you have other changes queued for v4.11, please take it via your
tree. Otherwise, perhaps the docs tree or mine? (I don't currently
have any fixes queued; I'm just trying to minimize pull requests going
to Linus...)
-Kees
--
Kees Cook
Pixel Security
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 03/18] pstore: Avoid race in module unloading
On Tue, Mar 7, 2017 at 8:16 AM, Namhyung Kim <namhy...@gmail.com> wrote:
> Hi Kees,
>
> On Tue, Mar 7, 2017 at 6:55 AM, Kees Cook <keesc...@chromium.org> wrote:
>> Technically, it might be possible for struct pstore_info to go out of
>> scope after the module_put(), so report the backend name first.
>
> But in that case, using pstore will crash the kernel anyway, right?
> If so, why pstore doesn't keep a reference until unregister?
> Do I miss something?
I could be wrong with this, since the backend can't call unregister
until register has finished... I'll drop this patch.
-Kees
>
> Thanks,
> Namhyung
>
>
>>
>> Signed-off-by: Kees Cook <keesc...@chromium.org>
>> ---
>> fs/pstore/platform.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
>> index 074fe85a2078..d69ef8a840b9 100644
>> --- a/fs/pstore/platform.c
>> +++ b/fs/pstore/platform.c
>> @@ -722,10 +722,10 @@ int pstore_register(struct pstore_info *psi)
>> */
>> backend = psi->name;
>>
>> - module_put(owner);
>> -
>> pr_info("Registered %s as persistent store backend\n", psi->name);
>>
>> + module_put(owner);
>> +
>> return 0;
>> }
>> EXPORT_SYMBOL_GPL(pstore_register);
>> --
>> 2.7.4
>>
--
Kees Cook
Pixel Security
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 06/18] pstore: Extract common arguments into structure
On Tue, Mar 7, 2017 at 8:22 AM, Namhyung Kim <namhy...@gmail.com> wrote:
> On Tue, Mar 7, 2017 at 6:55 AM, Kees Cook <keesc...@chromium.org> wrote:
>> The read/mkfile pair pass the same arguments and should be cleared
>> between calls. Move to a structure and wipe it after every loop.
>>
>> Signed-off-by: Kees Cook <keesc...@chromium.org>
>> ---
>> fs/pstore/platform.c | 55
>> +++---
>> include/linux/pstore.h | 28 -
>> 2 files changed, 57 insertions(+), 26 deletions(-)
>>
>> diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
>> index 320a673ecb5b..3fa1575a6e36 100644
>> --- a/fs/pstore/platform.c
>> +++ b/fs/pstore/platform.c
>> @@ -766,16 +766,9 @@ EXPORT_SYMBOL_GPL(pstore_unregister);
>> void pstore_get_records(int quiet)
>> {
>> struct pstore_info *psi = psinfo;
>> - char*buf = NULL;
>> - ssize_t size;
>> - u64 id;
>> - int count;
>> - enum pstore_type_id type;
>> - struct timespec time;
>> + struct pstore_recordrecord = { .psi = psi, };
>> int failed = 0, rc;
>> - boolcompressed;
>> int unzipped_len = -1;
>> - ssize_t ecc_notice_size = 0;
>>
>> if (!psi)
>> return;
>> @@ -784,39 +777,51 @@ void pstore_get_records(int quiet)
>> if (psi->open && psi->open(psi))
>> goto out;
>>
>> - while ((size = psi->read(, , , , ,
>> ,
>> -_notice_size, psi)) > 0) {
>> - if (compressed && (type == PSTORE_TYPE_DMESG)) {
>> + while ((record.size = psi->read(, ,
>> +, ,
>> +, ,
>> +_notice_size,
>> +record.psi)) > 0) {
>> + if (record.compressed &&
>> + record.type == PSTORE_TYPE_DMESG) {
>> if (big_oops_buf)
>> - unzipped_len = pstore_decompress(buf,
>> - big_oops_buf, size,
>> + unzipped_len = pstore_decompress(
>> + record.buf,
>> + big_oops_buf,
>> + record.size,
>> big_oops_buf_sz);
>>
>> if (unzipped_len > 0) {
>> - if (ecc_notice_size)
>> + if (record.ecc_notice_size)
>> memcpy(big_oops_buf + unzipped_len,
>> - buf + size, ecc_notice_size);
>> - kfree(buf);
>> - buf = big_oops_buf;
>> - size = unzipped_len;
>> - compressed = false;
>> + record.buf + recorrecord.size,
>
> A typo on record.size.
Thanks! Yeah, 0-day noticed this too. I've refreshed the patches in my
tree with the correction now.
-Kees
--
Kees Cook
Pixel Security
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] docs: Clarify details for reporting security bugs
On Mon, Mar 6, 2017 at 11:27 PM, Jonathan Corbet <cor...@lwn.net> wrote: > On Mon, 6 Mar 2017 11:13:51 -0800 > Kees Cook <keesc...@chromium.org> wrote: > >> The kernel security team is regularly asked to provide CVE identifiers, >> which we don't normally do. This updates the documentation to mention >> this and adds some more details about coordination and patch handling >> that come up regularly. Based on an earlier draft by Willy Tarreau. >> >> Signed-off-by: Kees Cook <keesc...@chromium.org> >> Acked-by: Willy Tarreau <w...@1wt.eu> > > Seems good, applied to the docs tree, thanks. Thanks! > >> Related question: shouldn't security-bugs.rst and submitting-patches.rst live >> in /process/ rather than /admin-guide/ ? > > The former should maybe be there, depending on just who we think it > should be aimed at, I guess. submitting-patches.rst is already in > process/, though, so I'm not quite sure I understand that question? Ah, maybe I was looking at the wrong thing. Regardless, it seems like security-bugs.rst is the same "type" as "submitting-patches.rst", so perhaps it should move? -Kees -- Kees Cook Pixel Security -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 03/18] pstore: Avoid race in module unloading
Technically, it might be possible for struct pstore_info to go out of
scope after the module_put(), so report the backend name first.
Signed-off-by: Kees Cook <keesc...@chromium.org>
---
fs/pstore/platform.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
index 074fe85a2078..d69ef8a840b9 100644
--- a/fs/pstore/platform.c
+++ b/fs/pstore/platform.c
@@ -722,10 +722,10 @@ int pstore_register(struct pstore_info *psi)
*/
backend = psi->name;
- module_put(owner);
-
pr_info("Registered %s as persistent store backend\n", psi->name);
+ module_put(owner);
+
return 0;
}
EXPORT_SYMBOL_GPL(pstore_register);
--
2.7.4
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 02/18] pstore: Shut down worker when unregistering
When built as a module and running with update_ms >= 0, pstore will Oops
during module unload since the work timer is still running. This makes sure
the worker is stopped before unloading.
Signed-off-by: Kees Cook <keesc...@chromium.org>
Cc: sta...@vger.kernel.org
---
fs/pstore/platform.c | 10 +-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
index cfc1abd264d9..074fe85a2078 100644
--- a/fs/pstore/platform.c
+++ b/fs/pstore/platform.c
@@ -709,6 +709,7 @@ int pstore_register(struct pstore_info *psi)
if (psi->flags & PSTORE_FLAGS_PMSG)
pstore_register_pmsg();
+ /* Start watching for new records, if desired. */
if (pstore_update_ms >= 0) {
pstore_timer.expires = jiffies +
msecs_to_jiffies(pstore_update_ms);
@@ -731,6 +732,11 @@ EXPORT_SYMBOL_GPL(pstore_register);
void pstore_unregister(struct pstore_info *psi)
{
+ /* Stop timer and make sure all work has finished. */
+ pstore_update_ms = -1;
+ del_timer_sync(_timer);
+ flush_work(_work);
+
if (psi->flags & PSTORE_FLAGS_PMSG)
pstore_unregister_pmsg();
if (psi->flags & PSTORE_FLAGS_FTRACE)
@@ -830,7 +836,9 @@ static void pstore_timefunc(unsigned long dummy)
schedule_work(_work);
}
- mod_timer(_timer, jiffies + msecs_to_jiffies(pstore_update_ms));
+ if (pstore_update_ms >= 0)
+ mod_timer(_timer,
+ jiffies + msecs_to_jiffies(pstore_update_ms));
}
module_param(backend, charp, 0444);
--
2.7.4
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 08/18] pstore: Switch pstore_mkfile to pass record
Instead of the long list of arguments, just pass the new record struct.
Signed-off-by: Kees Cook <keesc...@chromium.org>
---
fs/pstore/inode.c| 57 +---
fs/pstore/internal.h | 5 +
fs/pstore/platform.c | 6 +-
3 files changed, 34 insertions(+), 34 deletions(-)
diff --git a/fs/pstore/inode.c b/fs/pstore/inode.c
index 57c0646479f5..a98787bab3e6 100644
--- a/fs/pstore/inode.c
+++ b/fs/pstore/inode.c
@@ -302,9 +302,7 @@ bool pstore_is_mounted(void)
* Load it up with "size" bytes of data from "buf".
* Set the mtime & ctime to the date that this record was originally stored.
*/
-int pstore_mkfile(enum pstore_type_id type, char *psname, u64 id, int count,
- char *data, bool compressed, size_t size,
- struct timespec time, struct pstore_info *psi)
+int pstore_mkfile(struct pstore_record *record)
{
struct dentry *root = pstore_sb->s_root;
struct dentry *dentry;
@@ -313,12 +311,13 @@ int pstore_mkfile(enum pstore_type_id type, char *psname,
u64 id, int count,
charname[PSTORE_NAMELEN];
struct pstore_private *private, *pos;
unsigned long flags;
+ size_t size = record->size + record->ecc_notice_size;
spin_lock_irqsave(_lock, flags);
list_for_each_entry(pos, , list) {
- if (pos->type == type &&
- pos->id == id &&
- pos->psi == psi) {
+ if (pos->type == record->type &&
+ pos->id == record->id &&
+ pos->psi == record->psi) {
rc = -EEXIST;
break;
}
@@ -336,48 +335,56 @@ int pstore_mkfile(enum pstore_type_id type, char *psname,
u64 id, int count,
private = kmalloc(sizeof *private + size, GFP_KERNEL);
if (!private)
goto fail_alloc;
- private->type = type;
- private->id = id;
- private->count = count;
- private->psi = psi;
+ private->type = record->type;
+ private->id = record->id;
+ private->count = record->count;
+ private->psi = record->psi;
- switch (type) {
+ switch (record->type) {
case PSTORE_TYPE_DMESG:
scnprintf(name, sizeof(name), "dmesg-%s-%lld%s",
- psname, id, compressed ? ".enc.z" : "");
+ record->psi->name, record->id,
+ record->compressed ? ".enc.z" : "");
break;
case PSTORE_TYPE_CONSOLE:
- scnprintf(name, sizeof(name), "console-%s-%lld", psname, id);
+ scnprintf(name, sizeof(name), "console-%s-%lld",
+ record->psi->name, record->id);
break;
case PSTORE_TYPE_FTRACE:
- scnprintf(name, sizeof(name), "ftrace-%s-%lld", psname, id);
+ scnprintf(name, sizeof(name), "ftrace-%s-%lld",
+ record->psi->name, record->id);
break;
case PSTORE_TYPE_MCE:
- scnprintf(name, sizeof(name), "mce-%s-%lld", psname, id);
+ scnprintf(name, sizeof(name), "mce-%s-%lld",
+ record->psi->name, record->id);
break;
case PSTORE_TYPE_PPC_RTAS:
- scnprintf(name, sizeof(name), "rtas-%s-%lld", psname, id);
+ scnprintf(name, sizeof(name), "rtas-%s-%lld",
+ record->psi->name, record->id);
break;
case PSTORE_TYPE_PPC_OF:
scnprintf(name, sizeof(name), "powerpc-ofw-%s-%lld",
- psname, id);
+ record->psi->name, record->id);
break;
case PSTORE_TYPE_PPC_COMMON:
scnprintf(name, sizeof(name), "powerpc-common-%s-%lld",
- psname, id);
+ record->psi->name, record->id);
break;
case PSTORE_TYPE_PMSG:
- scnprintf(name, sizeof(name), "pmsg-%s-%lld", psname, id);
+ scnprintf(name, sizeof(name), "pmsg-%s-%lld",
+ record->psi->name, record->id);
break;
case PSTORE_TYPE_PPC_OPAL:
- sprintf(name, "powerpc-opal-%s-%lld", psname, id);
+ scnprintf(name, sizeof(name), "powerpc-opal-%s-%lld",
+ record->psi->name, record->id);
break;
case PSTORE
[PATCH 07/18] pstore: Move record decompression to function
This moves the record decompression logic out to a separate function
to avoid the deep indentation.
Signed-off-by: Kees Cook <keesc...@chromium.org>
---
fs/pstore/platform.c | 67 +---
1 file changed, 37 insertions(+), 30 deletions(-)
diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
index 3fa1575a6e36..0503380704de 100644
--- a/fs/pstore/platform.c
+++ b/fs/pstore/platform.c
@@ -757,6 +757,37 @@ void pstore_unregister(struct pstore_info *psi)
}
EXPORT_SYMBOL_GPL(pstore_unregister);
+static void decompress_record(struct pstore_record *record)
+{
+ int unzipped_len;
+
+ /* Only PSTORE_TYPE_DMESG support compression. */
+ if (!record->compressed || record->type != PSTORE_TYPE_DMESG) {
+ pr_warn("ignored compressed record type %d\n", record->type);
+ return;
+ }
+
+ /* No compression method has created the common buffer. */
+ if (!big_oops_buf) {
+ pr_warn("no decompression buffer allocated\n");
+ return;
+ }
+
+ unzipped_len = pstore_decompress(record->buf, big_oops_buf,
+record->size, big_oops_buf_sz);
+ if (unzipped_len > 0) {
+ if (record->ecc_notice_size)
+ memcpy(big_oops_buf + unzipped_len,
+ record->buf + record->size,
+ record->ecc_notice_size);
+ kfree(record->buf);
+ record->buf = big_oops_buf;
+ record->size = unzipped_len;
+ record->compressed = false;
+ } else
+ pr_err("decompression failed: %d\n", unzipped_len);
+}
+
/*
* Read all the records from the persistent store. Create
* files in our filesystem. Don't warn about -EEXIST errors
@@ -768,7 +799,6 @@ void pstore_get_records(int quiet)
struct pstore_info *psi = psinfo;
struct pstore_recordrecord = { .psi = psi, };
int failed = 0, rc;
- int unzipped_len = -1;
if (!psi)
return;
@@ -782,41 +812,18 @@ void pstore_get_records(int quiet)
, ,
_notice_size,
record.psi)) > 0) {
- if (record.compressed &&
- record.type == PSTORE_TYPE_DMESG) {
- if (big_oops_buf)
- unzipped_len = pstore_decompress(
- record.buf,
- big_oops_buf,
- record.size,
- big_oops_buf_sz);
-
- if (unzipped_len > 0) {
- if (record.ecc_notice_size)
- memcpy(big_oops_buf + unzipped_len,
- record.buf + recorrecord.size,
- record.ecc_notice_size);
- kfree(record.buf);
- record.buf = big_oops_buf;
- record.size = unzipped_len;
- record.compressed = false;
- } else {
- pr_err("decompression failed;returned %d\n",
- unzipped_len);
- record.compressed = true;
- }
- }
+
+ decompress_record();
rc = pstore_mkfile(record.type, psi->name, record.id,
record.count, record.buf,
record.compressed,
record.size + record.ecc_notice_size,
record.time, record.psi);
- if (unzipped_len < 0) {
- /* Free buffer other than big oops */
+
+ /* Free buffer other than big oops */
+ if (record.buf != big_oops_buf)
kfree(record.buf);
- record.buf = NULL;
- } else
- unzipped_len = -1;
+
if (rc && (rc != -EEXIST || !quiet))
failed++;
--
2.7.4
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 14/18] pstore: Do not duplicate record metadata
This switches the inode-private data from carrying duplicate metadata to
keeping the record passed in during pstore_mkfile().
Signed-off-by: Kees Cook <keesc...@chromium.org>
---
fs/pstore/inode.c| 57 ++--
fs/pstore/platform.c | 6 ++
2 files changed, 30 insertions(+), 33 deletions(-)
diff --git a/fs/pstore/inode.c b/fs/pstore/inode.c
index 3d1f047e4f41..0ea281b457fa 100644
--- a/fs/pstore/inode.c
+++ b/fs/pstore/inode.c
@@ -47,12 +47,8 @@ static LIST_HEAD(allpstore);
struct pstore_private {
struct list_head list;
- struct pstore_info *psi;
- enum pstore_type_id type;
- u64 id;
- int count;
- ssize_t size;
- char*buf;
+ struct pstore_record *record;
+ size_t total_size;
};
struct pstore_ftrace_seq_data {
@@ -67,7 +63,10 @@ static void free_pstore_private(struct pstore_private
*private)
{
if (!private)
return;
- kfree(private->buf);
+ if (private->record) {
+ kfree(private->record->buf);
+ kfree(private->record);
+ }
kfree(private);
}
@@ -80,9 +79,9 @@ static void *pstore_ftrace_seq_start(struct seq_file *s,
loff_t *pos)
if (!data)
return NULL;
- data->off = ps->size % REC_SIZE;
+ data->off = ps->total_size % REC_SIZE;
data->off += *pos * REC_SIZE;
- if (data->off + REC_SIZE > ps->size) {
+ if (data->off + REC_SIZE > ps->total_size) {
kfree(data);
return NULL;
}
@@ -102,7 +101,7 @@ static void *pstore_ftrace_seq_next(struct seq_file *s,
void *v, loff_t *pos)
struct pstore_ftrace_seq_data *data = v;
data->off += REC_SIZE;
- if (data->off + REC_SIZE > ps->size)
+ if (data->off + REC_SIZE > ps->total_size)
return NULL;
(*pos)++;
@@ -113,7 +112,9 @@ static int pstore_ftrace_seq_show(struct seq_file *s, void
*v)
{
struct pstore_private *ps = s->private;
struct pstore_ftrace_seq_data *data = v;
- struct pstore_ftrace_record *rec = (void *)(ps->buf + data->off);
+ struct pstore_ftrace_record *rec;
+
+ rec = (struct pstore_ftrace_record *)(ps->record->buf + data->off);
seq_printf(s, "CPU:%d ts:%llu %08lx %08lx %pf <- %pF\n",
pstore_ftrace_decode_cpu(rec),
@@ -133,7 +134,7 @@ static const struct seq_operations pstore_ftrace_seq_ops = {
static int pstore_check_syslog_permissions(struct pstore_private *ps)
{
- switch (ps->type) {
+ switch (ps->record->type) {
case PSTORE_TYPE_DMESG:
case PSTORE_TYPE_CONSOLE:
return check_syslog_permissions(SYSLOG_ACTION_READ_ALL,
@@ -149,9 +150,10 @@ static ssize_t pstore_file_read(struct file *file, char
__user *userbuf,
struct seq_file *sf = file->private_data;
struct pstore_private *ps = sf->private;
- if (ps->type == PSTORE_TYPE_FTRACE)
+ if (ps->record->type == PSTORE_TYPE_FTRACE)
return seq_read(file, userbuf, count, ppos);
- return simple_read_from_buffer(userbuf, count, ppos, ps->buf, ps->size);
+ return simple_read_from_buffer(userbuf, count, ppos,
+ ps->record->buf, ps->total_size);
}
static int pstore_file_open(struct inode *inode, struct file *file)
@@ -165,7 +167,7 @@ static int pstore_file_open(struct inode *inode, struct
file *file)
if (err)
return err;
- if (ps->type == PSTORE_TYPE_FTRACE)
+ if (ps->record->type == PSTORE_TYPE_FTRACE)
sops = _ftrace_seq_ops;
err = seq_open(file, sops);
@@ -201,17 +203,18 @@ static const struct file_operations
pstore_file_operations = {
static int pstore_unlink(struct inode *dir, struct dentry *dentry)
{
struct pstore_private *p = d_inode(dentry)->i_private;
+ struct pstore_record *record = p->record;
int err;
err = pstore_check_syslog_permissions(p);
if (err)
return err;
- if (p->psi->erase) {
- mutex_lock(>psi->read_mutex);
- p->psi->erase(p->type, p->id, p->count,
- d_inode(dentry)->i_ctime, p->psi);
- mutex_unlock(>psi->read_mutex);
+ if (record->psi->erase) {
+ mutex_lock(>psi->read_mutex);
+ record->psi->erase(record->type, record->id, record->count,
+ d_inode(dentry)->i_ctime, record->psi);
+ mutex_unlock(>psi->read_mutex);
} else {
return -EPERM;
}
@@ -323,9 +326,9 @@ int pstore_mkfile(struct pstore_r
[PATCH 00/18] pstore: refactor internal APIs
For a long time I've been bothered by the complexity of argument passing in the pstore internals, which makes understanding things and changing things extremely fragile. With the proposal of a new backend (EPI capsules), and my attempts to reorganize things for the proposed multiple-pmsg frontend, I've spent some time refactoring the internal pstore API to pass a record structure instead of lots of arguments. This hugely simplifies things, and lets me document the API better, reduce memory copies, etc. Included in the series are a few bug fixes as well. If pstore backend authors could review the changes I've made in their drivers, I'd appreciate it. Hopefully I got it all right. :) And unless there are any objections, I'll put this into -next in a couple days. I intend to continue working on this to fix up the "ecc_notification_size" field and make it more generalized, and get multiple pmsg support added too, but I'd like to get this first series out for review first, since it's rather long already. Thanks! -Kees -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 15/18] pstore: Replace arguments for erase() API
This removes the argument list for the erase() callback and replaces it
with a pointer to the backend record details to be removed.
Signed-off-by: Kees Cook <keesc...@chromium.org>
---
drivers/acpi/apei/erst.c | 8 +++-
drivers/firmware/efi/efi-pstore.c | 26 +++---
fs/pstore/inode.c | 12 +---
fs/pstore/ram.c | 15 +++
include/linux/pstore.h| 16 +---
5 files changed, 31 insertions(+), 46 deletions(-)
diff --git a/drivers/acpi/apei/erst.c b/drivers/acpi/apei/erst.c
index 440588d189e7..7207e5fc9d3d 100644
--- a/drivers/acpi/apei/erst.c
+++ b/drivers/acpi/apei/erst.c
@@ -927,8 +927,7 @@ static int erst_open_pstore(struct pstore_info *psi);
static int erst_close_pstore(struct pstore_info *psi);
static ssize_t erst_reader(struct pstore_record *record);
static int erst_writer(struct pstore_record *record);
-static int erst_clearer(enum pstore_type_id type, u64 id, int count,
- struct timespec time, struct pstore_info *psi);
+static int erst_clearer(struct pstore_record *record);
static struct pstore_info erst_info = {
.owner = THIS_MODULE,
@@ -1100,10 +1099,9 @@ static int erst_writer(struct pstore_record *record)
return ret;
}
-static int erst_clearer(enum pstore_type_id type, u64 id, int count,
- struct timespec time, struct pstore_info *psi)
+static int erst_clearer(struct pstore_record *record)
{
- return erst_clear(id);
+ return erst_clear(record->id);
}
static int __init erst_init(void)
diff --git a/drivers/firmware/efi/efi-pstore.c
b/drivers/firmware/efi/efi-pstore.c
index f81e3ec6f1c0..93d8cdbe7ef4 100644
--- a/drivers/firmware/efi/efi-pstore.c
+++ b/drivers/firmware/efi/efi-pstore.c
@@ -266,10 +266,7 @@ static int efi_pstore_write(struct pstore_record *record)
};
struct pstore_erase_data {
- u64 id;
- enum pstore_type_id type;
- int count;
- struct timespec time;
+ struct pstore_record *record;
efi_char16_t *name;
};
@@ -295,8 +292,9 @@ static int efi_pstore_erase_func(struct efivar_entry
*entry, void *data)
* Check if an old format, which doesn't support
* holding multiple logs, remains.
*/
- sprintf(name_old, "dump-type%u-%u-%lu", ed->type,
- (unsigned int)ed->id, ed->time.tv_sec);
+ snprintf(name_old, sizeof(name_old), "dump-type%u-%u-%lu",
+ ed->record->type, (unsigned int)ed->record->id,
+ ed->record->time.tv_sec);
for (i = 0; i < DUMP_NAME_LEN; i++)
efi_name_old[i] = name_old[i];
@@ -321,8 +319,7 @@ static int efi_pstore_erase_func(struct efivar_entry
*entry, void *data)
return 1;
}
-static int efi_pstore_erase(enum pstore_type_id type, u64 id, int count,
- struct timespec time, struct pstore_info *psi)
+static int efi_pstore_erase(struct pstore_record *record)
{
struct pstore_erase_data edata;
struct efivar_entry *entry = NULL;
@@ -331,17 +328,16 @@ static int efi_pstore_erase(enum pstore_type_id type, u64
id, int count,
int found, i;
unsigned int part;
- do_div(id, 1000);
- part = do_div(id, 100);
- sprintf(name, "dump-type%u-%u-%d-%lu", type, part, count, time.tv_sec);
+ do_div(record->id, 1000);
+ part = do_div(record->id, 100);
+ snprintf(name, sizeof(name), "dump-type%u-%u-%d-%lu",
+record->type, record->part, record->count,
+record->time.tv_sec);
for (i = 0; i < DUMP_NAME_LEN; i++)
efi_name[i] = name[i];
- edata.id = part;
- edata.type = type;
- edata.count = count;
- edata.time = time;
+ edata.record = record;
edata.name = efi_name;
if (efivar_entry_iter_begin())
diff --git a/fs/pstore/inode.c b/fs/pstore/inode.c
index 0ea281b457fa..06504b69575b 100644
--- a/fs/pstore/inode.c
+++ b/fs/pstore/inode.c
@@ -210,14 +210,12 @@ static int pstore_unlink(struct inode *dir, struct dentry
*dentry)
if (err)
return err;
- if (record->psi->erase) {
- mutex_lock(>psi->read_mutex);
- record->psi->erase(record->type, record->id, record->count,
- d_inode(dentry)->i_ctime, record->psi);
- mutex_unlock(>psi->read_mutex);
- } else {
+ if (!record->psi->erase)
return -EPERM;
- }
+
+ mutex_lock(>psi->read_mutex);
+ record->psi->erase(record);
+ mutex_unlock(>psi->read_mutex);
return simple_unlink(dir, dentry);
}
diff --g
[PATCH 13/18] pstore: Allocate records on heap instead of stack
In preparation for handling records off to pstore_mkfile(), allocate the
record instead of reusing stack. This still always frees the record,
though, since pstore_mkfile() isn't yet keeping it.
Signed-off-by: Kees Cook <keesc...@chromium.org>
---
fs/pstore/platform.c | 30 ++
1 file changed, 22 insertions(+), 8 deletions(-)
diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
index d897e2f11b6a..072326625629 100644
--- a/fs/pstore/platform.c
+++ b/fs/pstore/platform.c
@@ -818,8 +818,7 @@ static void decompress_record(struct pstore_record *record)
void pstore_get_records(int quiet)
{
struct pstore_info *psi = psinfo;
- struct pstore_recordrecord = { .psi = psi, };
- int failed = 0, rc;
+ int failed = 0;
if (!psi)
return;
@@ -833,19 +832,34 @@ void pstore_get_records(int quiet)
* may reallocate record.buf. On success, pstore_mkfile() will keep
* the record.buf, so free it only on failure.
*/
- while ((record.size = psi->read()) > 0) {
- decompress_record();
- rc = pstore_mkfile();
+ for (;;) {
+ struct pstore_record *record;
+ int rc;
+
+ record = kzalloc(sizeof(*record), GFP_KERNEL);
+ if (!record) {
+ pr_err("out of memory creating record\n");
+ break;
+ }
+ record->psi = psi;
+
+ record->size = psi->read(record);
+
+ /* No more records left in backend? */
+ if (record->size <= 0)
+ break;
+
+ decompress_record(record);
+ rc = pstore_mkfile(record);
if (rc) {
/* pstore_mkfile() did not take buf, so free it. */
- kfree(record.buf);
+ kfree(record->buf);
if (rc != -EEXIST || !quiet)
failed++;
}
/* Reset for next record. */
- memset(, 0, sizeof(record));
- record.psi = psi;
+ kfree(record);
}
if (psi->close)
psi->close(psi);
--
2.7.4
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 17/18] pstore: Replace arguments for write_buf_user() API
Removes argument list in favor of pstore record, though the user buffer
remains passed separately since it must carry the __user annotation.
Signed-off-by: Kees Cook <keesc...@chromium.org>
---
fs/pstore/platform.c | 35 ---
fs/pstore/pmsg.c | 9 ++---
fs/pstore/ram.c| 14 +-
include/linux/pstore.h | 23 +++
4 files changed, 30 insertions(+), 51 deletions(-)
diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
index 5eecf9012459..1e6642a2063e 100644
--- a/fs/pstore/platform.c
+++ b/fs/pstore/platform.c
@@ -639,47 +639,36 @@ static int pstore_write_compat(struct pstore_record
*record)
return record->psi->write_buf(record);
}
-static int pstore_write_buf_user_compat(enum pstore_type_id type,
- enum kmsg_dump_reason reason,
- u64 *id, unsigned int part,
- const char __user *buf,
- bool compressed, size_t size,
- struct pstore_info *psi)
+static int pstore_write_buf_user_compat(struct pstore_record *record,
+ const char __user *buf)
{
unsigned long flags = 0;
- size_t i, bufsize = size;
+ size_t i, bufsize, total_size = record->size;
long ret = 0;
- if (unlikely(!access_ok(VERIFY_READ, buf, size)))
+ if (unlikely(!access_ok(VERIFY_READ, buf, total_size)))
return -EFAULT;
+ bufsize = total_size;
if (bufsize > psinfo->bufsize)
bufsize = psinfo->bufsize;
+ record->buf = psinfo->buf;
spin_lock_irqsave(>buf_lock, flags);
- for (i = 0; i < size; ) {
- struct pstore_record record = {
- .type = type,
- .reason = reason,
- .id = id,
- .part = part,
- .buf = psinfo->buf,
- .compressed = compressed,
- .psi = psi,
- };
- size_t c = min(size - i, bufsize);
+ for (i = 0; i < total_size; ) {
+ size_t c = min(total_size - i, bufsize);
- ret = __copy_from_user(psinfo->buf, buf + i, c);
+ ret = __copy_from_user(record->buf, buf + i, c);
if (unlikely(ret != 0)) {
ret = -EFAULT;
break;
}
- record.size = c;
- ret = psi->write_buf();
+ record->size = c;
+ ret = record->psi->write_buf(record);
if (unlikely(ret < 0))
break;
i += c;
}
spin_unlock_irqrestore(>buf_lock, flags);
- return unlikely(ret < 0) ? ret : size;
+ return unlikely(ret < 0) ? ret : total_size;
}
/*
diff --git a/fs/pstore/pmsg.c b/fs/pstore/pmsg.c
index 78f6176c020f..ce35907602de 100644
--- a/fs/pstore/pmsg.c
+++ b/fs/pstore/pmsg.c
@@ -23,7 +23,11 @@ static DEFINE_MUTEX(pmsg_lock);
static ssize_t write_pmsg(struct file *file, const char __user *buf,
size_t count, loff_t *ppos)
{
- u64 id;
+ struct pstore_record record = {
+ .type = PSTORE_TYPE_PMSG,
+ .size = count,
+ .psi = psinfo,
+ };
int ret;
if (!count)
@@ -34,8 +38,7 @@ static ssize_t write_pmsg(struct file *file, const char
__user *buf,
return -EFAULT;
mutex_lock(_lock);
- ret = psinfo->write_buf_user(PSTORE_TYPE_PMSG, 0, , 0, buf, 0, count,
-psinfo);
+ ret = psinfo->write_buf_user(, buf);
mutex_unlock(_lock);
return ret ? ret : count;
}
diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index a7cdde60b1f9..d85e1adae1b6 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -451,19 +451,15 @@ static int notrace ramoops_pstore_write_buf(struct
pstore_record *record)
return 0;
}
-static int notrace ramoops_pstore_write_buf_user(enum pstore_type_id type,
-enum kmsg_dump_reason reason,
-u64 *id, unsigned int part,
-const char __user *buf,
-bool compressed, size_t size,
-struct pstore_info *psi)
+static int notrace ramoops_pstore_write_buf_user(struct pstore_record *record,
+const char __user *buf)
{
- if (type == PSTORE_TYPE_PMSG) {
- struct ramoops_context *cxt = psi->data;
+ if (record->type == PSTORE_TYPE_PMSG) {
+ struct ramoops_context *cxt = record-
[PATCH 04/18] pstore: Improve register_pstore() error reporting
Uncommon errors are better to get reported to dmesg so developers can
more easily figure out why pstore is unhappy with a backend attempting
to register.
Signed-off-by: Kees Cook <keesc...@chromium.org>
---
fs/pstore/platform.c | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
index d69ef8a840b9..320a673ecb5b 100644
--- a/fs/pstore/platform.c
+++ b/fs/pstore/platform.c
@@ -673,11 +673,15 @@ int pstore_register(struct pstore_info *psi)
{
struct module *owner = psi->owner;
- if (backend && strcmp(backend, psi->name))
+ if (backend && strcmp(backend, psi->name)) {
+ pr_warn("ignoring unexpected backend '%s'\n", psi->name);
return -EPERM;
+ }
spin_lock(_lock);
if (psinfo) {
+ pr_warn("backend '%s' already loaded: ignoring '%s'\n",
+ psinfo->name, psi->name);
spin_unlock(_lock);
return -EBUSY;
}
--
2.7.4
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 09/18] pstore: Replace arguments for read() API
The argument list for the pstore_read() interface is unwieldy. This changes
passes the new struct pstore_record instead. The erst backend was already
doing something similar internally.
Signed-off-by: Kees Cook <keesc...@chromium.org>
---
arch/powerpc/kernel/nvram_64.c| 61 +++---
drivers/acpi/apei/erst.c | 38 ++
drivers/firmware/efi/efi-pstore.c | 104 --
fs/pstore/platform.c | 7 +--
fs/pstore/ram.c | 53 ++-
include/linux/pstore.h| 20 +++-
6 files changed, 124 insertions(+), 159 deletions(-)
diff --git a/arch/powerpc/kernel/nvram_64.c b/arch/powerpc/kernel/nvram_64.c
index d5e2b8309939..7f192001d09a 100644
--- a/arch/powerpc/kernel/nvram_64.c
+++ b/arch/powerpc/kernel/nvram_64.c
@@ -442,10 +442,7 @@ static int nvram_pstore_write(enum pstore_type_id type,
* Returns the length of the data we read from each partition.
* Returns 0 if we've been called before.
*/
-static ssize_t nvram_pstore_read(u64 *id, enum pstore_type_id *type,
- int *count, struct timespec *time, char **buf,
- bool *compressed, ssize_t *ecc_notice_size,
- struct pstore_info *psi)
+static ssize_t nvram_pstore_read(struct pstore_record *record)
{
struct oops_log_info *oops_hdr;
unsigned int err_type, id_no, size = 0;
@@ -459,40 +456,40 @@ static ssize_t nvram_pstore_read(u64 *id, enum
pstore_type_id *type,
switch (nvram_type_ids[read_type]) {
case PSTORE_TYPE_DMESG:
part = _log_partition;
- *type = PSTORE_TYPE_DMESG;
+ record->type = PSTORE_TYPE_DMESG;
break;
case PSTORE_TYPE_PPC_COMMON:
sig = NVRAM_SIG_SYS;
part = _partition;
- *type = PSTORE_TYPE_PPC_COMMON;
- *id = PSTORE_TYPE_PPC_COMMON;
- time->tv_sec = 0;
- time->tv_nsec = 0;
+ record->type = PSTORE_TYPE_PPC_COMMON;
+ record->id = PSTORE_TYPE_PPC_COMMON;
+ record->time.tv_sec = 0;
+ record->time.tv_nsec = 0;
break;
#ifdef CONFIG_PPC_PSERIES
case PSTORE_TYPE_PPC_RTAS:
part = _log_partition;
- *type = PSTORE_TYPE_PPC_RTAS;
- time->tv_sec = last_rtas_event;
- time->tv_nsec = 0;
+ record->type = PSTORE_TYPE_PPC_RTAS;
+ record->time.tv_sec = last_rtas_event;
+ record->time.tv_nsec = 0;
break;
case PSTORE_TYPE_PPC_OF:
sig = NVRAM_SIG_OF;
part = _config_partition;
- *type = PSTORE_TYPE_PPC_OF;
- *id = PSTORE_TYPE_PPC_OF;
- time->tv_sec = 0;
- time->tv_nsec = 0;
+ record->type = PSTORE_TYPE_PPC_OF;
+ record->id = PSTORE_TYPE_PPC_OF;
+ record->time.tv_sec = 0;
+ record->time.tv_nsec = 0;
break;
#endif
#ifdef CONFIG_PPC_POWERNV
case PSTORE_TYPE_PPC_OPAL:
sig = NVRAM_SIG_FW;
part = _partition;
- *type = PSTORE_TYPE_PPC_OPAL;
- *id = PSTORE_TYPE_PPC_OPAL;
- time->tv_sec = 0;
- time->tv_nsec = 0;
+ record->type = PSTORE_TYPE_PPC_OPAL;
+ record->id = PSTORE_TYPE_PPC_OPAL;
+ record->time.tv_sec = 0;
+ record->time.tv_nsec = 0;
break;
#endif
default:
@@ -520,10 +517,10 @@ static ssize_t nvram_pstore_read(u64 *id, enum
pstore_type_id *type,
return 0;
}
- *count = 0;
+ record->count = 0;
if (part->os_partition)
- *id = id_no;
+ record->id = id_no;
if (nvram_type_ids[read_type] == PSTORE_TYPE_DMESG) {
size_t length, hdr_size;
@@ -533,28 +530,28 @@ static ssize_t nvram_pstore_read(u64 *id, enum
pstore_type_id *type,
/* Old format oops header had 2-byte record size */
hdr_size = sizeof(u16);
length = be16_to_cpu(oops_hdr->version);
- time->tv_sec = 0;
- time->tv_nsec = 0;
+ record->time.tv_sec = 0;
+ record->time.tv_nsec = 0;
} else {
hdr_size = sizeof(*oops_hdr);
length = be16_to_cpu(oops_hdr->report_length);
- time->tv_sec = be64_to_cpu(oops_hdr->timestamp);
- time->tv_nsec = 0;
+ record->time.tv_sec = be64_to_cpu(oops_hdr->timest
[PATCH 11/18] pstore: Always allocate buffer for decompression
Currently, pstore_mkfile() performs a memcpy() of the record contents,
so it can live anywhere. However, this is needlessly wasteful. In
preparation of pstore_mkfile() keeping the record contents, always
allocate a buffer for the contents.
Signed-off-by: Kees Cook <keesc...@chromium.org>
---
fs/pstore/platform.c | 38 --
1 file changed, 24 insertions(+), 14 deletions(-)
diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
index 879658b4c679..c0d401e732e6 100644
--- a/fs/pstore/platform.c
+++ b/fs/pstore/platform.c
@@ -768,6 +768,7 @@ EXPORT_SYMBOL_GPL(pstore_unregister);
static void decompress_record(struct pstore_record *record)
{
int unzipped_len;
+ char *decompressed;
/* Only PSTORE_TYPE_DMESG support compression. */
if (!record->compressed || record->type != PSTORE_TYPE_DMESG) {
@@ -783,17 +784,29 @@ static void decompress_record(struct pstore_record
*record)
unzipped_len = pstore_decompress(record->buf, big_oops_buf,
record->size, big_oops_buf_sz);
- if (unzipped_len > 0) {
- if (record->ecc_notice_size)
- memcpy(big_oops_buf + unzipped_len,
- record->buf + record->size,
- record->ecc_notice_size);
- kfree(record->buf);
- record->buf = big_oops_buf;
- record->size = unzipped_len;
- record->compressed = false;
- } else
+ if (unzipped_len <= 0) {
pr_err("decompression failed: %d\n", unzipped_len);
+ return;
+ }
+
+ /* Build new buffer for decompressed contents. */
+ decompressed = kmalloc(unzipped_len + record->ecc_notice_size,
+ GFP_KERNEL);
+ if (!decompressed) {
+ pr_err("decompression ran out of memory\n");
+ return;
+ }
+ memcpy(decompressed, big_oops_buf, unzipped_len);
+
+ /* Append ECC notice to decompressed buffer. */
+ memcpy(decompressed + unzipped_len, record->buf + record->size,
+ record->ecc_notice_size);
+
+ /* Swap out compresed contents with decompressed contents. */
+ kfree(record->buf);
+ record->buf = decompressed;
+ record->size = unzipped_len;
+ record->compressed = false;
}
/*
@@ -819,13 +832,10 @@ void pstore_get_records(int quiet)
decompress_record();
rc = pstore_mkfile();
- /* Free buffer other than big oops */
- if (record.buf != big_oops_buf)
- kfree(record.buf);
-
if (rc && (rc != -EEXIST || !quiet))
failed++;
+ kfree(record.buf);
memset(, 0, sizeof(record));
record.psi = psi;
}
--
2.7.4
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
