From: Alexander Popov <alex.popov@linux.com> To: Mark Rutland <mark.rutland@arm.com>, Kees Cook <keescook@chromium.org> Cc: linux-arm-kernel@lists.infradead.org, akpm@linux-foundation.org, catalin.marinas@arm.com, linux-kernel@vger.kernel.org, luto@kernel.org, will@kernel.org Subject: Re: [PATCH v2 03/13] stackleak: remove redundant check Date: Sun, 15 May 2022 19:17:16 +0300 [thread overview] Message-ID: <8d8061c4-2a3e-cb3a-00c9-677fa8899058@linux.com> (raw) In-Reply-To: <YnzQDWTw1qdtVJMJ@FVFF77S0Q05N> On 12.05.2022 12:14, Mark Rutland wrote: > On Wed, May 11, 2022 at 07:44:41AM -0700, Kees Cook wrote: >> >> >> On May 11, 2022 1:02:45 AM PDT, Mark Rutland <mark.rutland@arm.com> wrote: >>> On Tue, May 10, 2022 at 08:00:38PM -0700, Kees Cook wrote: >>>> On Tue, May 10, 2022 at 12:46:48PM +0100, Mark Rutland wrote: >>>>> On Sun, May 08, 2022 at 09:17:01PM +0300, Alexander Popov wrote: >>>>>> On 27.04.2022 20:31, Mark Rutland wrote: >>>>>>> In __stackleak_erase() we check that the `erase_low` value derived from >>>>>>> `current->lowest_stack` is above the lowest legitimate stack pointer >>>>>>> value, but this is already enforced by stackleak_track_stack() when >>>>>>> recording the lowest stack value. >>>>>>> >>>>>>> Remove the redundant check. >>>>>>> >>>>>>> There should be no functional change as a result of this patch. >>>>>> >>>>>> Mark, I can't agree here. I think this check is important. >>>>>> The performance profit from dropping it is less than the confidence decrease :) >>>>>> >>>>>> With this check, if the 'lowest_stack' value is corrupted, stackleak doesn't >>>>>> overwrite some wrong kernel memory, but simply clears the whole thread >>>>>> stack, which is safe behavior. >>>>> >>>>> If you feel strongly about it, I can restore the check, but I struggle to >>>>> believe that it's worthwhile. The `lowest_stack` value lives in the >>>>> task_struct, and if you have the power to corrupt that you have the power to do >>>>> much more interesting things. >>>>> >>>>> If we do restore it, I'd like to add a big fat comment explaining the >>>>> rationale (i.e. that it only matter if someone could corrupt >>>>> `current->lowest_stack`, as otherwise that's guarnateed to be within bounds). >>>> >>>> Yeah, let's restore it and add the comment. While I do agree it's likely >>>> that such an corruption would likely mean an attacker had significant >>>> control over kernel memory already, it is not uncommon that an attack >>>> only has a limited index from a given address, etc. Or some manipulation >>>> is possible via weird gadgets, etc. It's unlikely, but not impossible, >>>> and a bounds-check for that value is cheap compared to the rest of the >>>> work happening. :) >>> >>> Fair enough; I can go spin a patch restoring this. I'm somewhat unhappy with >>> silently fixing that up, though -- IMO it'd be better to BUG() or similar in >>> that case. >> >> I share your desires, and this was exactly what Alexander originally proposed, but Linus rejected it violently. :( >> https://lore.kernel.org/lkml/CA+55aFy6jNLsywVYdGp83AMrXBo_P-pkjkphPGrO=82SPKCpLQ@mail.gmail.com/ > > I see. :/ > > Thinking about this some more, if we assume someone can corrupt *some* word of > memory, then we need to consider that instead of corrupting > task_struct::lowest_stack, they could corrupt task_struct::stack (or x86's > cpu_current_top_of_stack prior to this series). > > With that in mind, if we detect that task_struct::lowest_stack is > out-of-bounds, we have no idea whether it has been corrupted or the other bound > values have been corrupted, and so we can't do the erase safely anyway. :) IMO, even if a kernel thread stack is moved somewhere for any weird reason, stackleak must erase it at the end of syscall and do its job. > So AFAICT we must *avoid* erasing when that goes wrong. Maybe we could WARN() > instead of BUG()? Mark, I think security features must not go out of service. The 'lowest_stack' value is for making stackleak faster. I believe if the 'lowest_stack' value is invalid, stackleak must not skip its main job and should erase the whole kernel thread stack. When I developed 'stackleak_erase()' I tried adding 'WARN_ON()', but it didn't work properly there, as I remember. Warning handling code is very complex. So I dropped that fragile part. Best regards, Alexander
WARNING: multiple messages have this Message-ID (diff)
From: Alexander Popov <alex.popov@linux.com> To: Mark Rutland <mark.rutland@arm.com>, Kees Cook <keescook@chromium.org> Cc: linux-arm-kernel@lists.infradead.org, akpm@linux-foundation.org, catalin.marinas@arm.com, linux-kernel@vger.kernel.org, luto@kernel.org, will@kernel.org Subject: Re: [PATCH v2 03/13] stackleak: remove redundant check Date: Sun, 15 May 2022 19:17:16 +0300 [thread overview] Message-ID: <8d8061c4-2a3e-cb3a-00c9-677fa8899058@linux.com> (raw) In-Reply-To: <YnzQDWTw1qdtVJMJ@FVFF77S0Q05N> On 12.05.2022 12:14, Mark Rutland wrote: > On Wed, May 11, 2022 at 07:44:41AM -0700, Kees Cook wrote: >> >> >> On May 11, 2022 1:02:45 AM PDT, Mark Rutland <mark.rutland@arm.com> wrote: >>> On Tue, May 10, 2022 at 08:00:38PM -0700, Kees Cook wrote: >>>> On Tue, May 10, 2022 at 12:46:48PM +0100, Mark Rutland wrote: >>>>> On Sun, May 08, 2022 at 09:17:01PM +0300, Alexander Popov wrote: >>>>>> On 27.04.2022 20:31, Mark Rutland wrote: >>>>>>> In __stackleak_erase() we check that the `erase_low` value derived from >>>>>>> `current->lowest_stack` is above the lowest legitimate stack pointer >>>>>>> value, but this is already enforced by stackleak_track_stack() when >>>>>>> recording the lowest stack value. >>>>>>> >>>>>>> Remove the redundant check. >>>>>>> >>>>>>> There should be no functional change as a result of this patch. >>>>>> >>>>>> Mark, I can't agree here. I think this check is important. >>>>>> The performance profit from dropping it is less than the confidence decrease :) >>>>>> >>>>>> With this check, if the 'lowest_stack' value is corrupted, stackleak doesn't >>>>>> overwrite some wrong kernel memory, but simply clears the whole thread >>>>>> stack, which is safe behavior. >>>>> >>>>> If you feel strongly about it, I can restore the check, but I struggle to >>>>> believe that it's worthwhile. The `lowest_stack` value lives in the >>>>> task_struct, and if you have the power to corrupt that you have the power to do >>>>> much more interesting things. >>>>> >>>>> If we do restore it, I'd like to add a big fat comment explaining the >>>>> rationale (i.e. that it only matter if someone could corrupt >>>>> `current->lowest_stack`, as otherwise that's guarnateed to be within bounds). >>>> >>>> Yeah, let's restore it and add the comment. While I do agree it's likely >>>> that such an corruption would likely mean an attacker had significant >>>> control over kernel memory already, it is not uncommon that an attack >>>> only has a limited index from a given address, etc. Or some manipulation >>>> is possible via weird gadgets, etc. It's unlikely, but not impossible, >>>> and a bounds-check for that value is cheap compared to the rest of the >>>> work happening. :) >>> >>> Fair enough; I can go spin a patch restoring this. I'm somewhat unhappy with >>> silently fixing that up, though -- IMO it'd be better to BUG() or similar in >>> that case. >> >> I share your desires, and this was exactly what Alexander originally proposed, but Linus rejected it violently. :( >> https://lore.kernel.org/lkml/CA+55aFy6jNLsywVYdGp83AMrXBo_P-pkjkphPGrO=82SPKCpLQ@mail.gmail.com/ > > I see. :/ > > Thinking about this some more, if we assume someone can corrupt *some* word of > memory, then we need to consider that instead of corrupting > task_struct::lowest_stack, they could corrupt task_struct::stack (or x86's > cpu_current_top_of_stack prior to this series). > > With that in mind, if we detect that task_struct::lowest_stack is > out-of-bounds, we have no idea whether it has been corrupted or the other bound > values have been corrupted, and so we can't do the erase safely anyway. :) IMO, even if a kernel thread stack is moved somewhere for any weird reason, stackleak must erase it at the end of syscall and do its job. > So AFAICT we must *avoid* erasing when that goes wrong. Maybe we could WARN() > instead of BUG()? Mark, I think security features must not go out of service. The 'lowest_stack' value is for making stackleak faster. I believe if the 'lowest_stack' value is invalid, stackleak must not skip its main job and should erase the whole kernel thread stack. When I developed 'stackleak_erase()' I tried adding 'WARN_ON()', but it didn't work properly there, as I remember. Warning handling code is very complex. So I dropped that fragile part. Best regards, Alexander _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2022-05-15 16:17 UTC|newest] Thread overview: 94+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-04-27 17:31 [PATCH v2 00/13] stackleak: fixes and rework Mark Rutland 2022-04-27 17:31 ` Mark Rutland 2022-04-27 17:31 ` [PATCH v2 01/13] arm64: stackleak: fix current_top_of_stack() Mark Rutland 2022-04-27 17:31 ` Mark Rutland 2022-05-04 16:41 ` Catalin Marinas 2022-05-04 16:41 ` Catalin Marinas 2022-05-04 19:01 ` Kees Cook 2022-05-04 19:01 ` Kees Cook 2022-05-04 19:55 ` Catalin Marinas 2022-05-04 19:55 ` Catalin Marinas 2022-05-05 8:25 ` Will Deacon 2022-05-05 8:25 ` Will Deacon 2022-05-08 17:24 ` Alexander Popov 2022-05-08 17:24 ` Alexander Popov 2022-05-10 11:36 ` Mark Rutland 2022-05-10 11:36 ` Mark Rutland 2022-04-27 17:31 ` [PATCH v2 02/13] stackleak: move skip_erasing() check earlier Mark Rutland 2022-04-27 17:31 ` Mark Rutland 2022-05-08 17:44 ` Alexander Popov 2022-05-08 17:44 ` Alexander Popov 2022-05-10 11:40 ` Mark Rutland 2022-05-10 11:40 ` Mark Rutland 2022-04-27 17:31 ` [PATCH v2 03/13] stackleak: remove redundant check Mark Rutland 2022-04-27 17:31 ` Mark Rutland 2022-05-08 18:17 ` Alexander Popov 2022-05-08 18:17 ` Alexander Popov 2022-05-10 11:46 ` Mark Rutland 2022-05-10 11:46 ` Mark Rutland 2022-05-11 3:00 ` Kees Cook 2022-05-11 3:00 ` Kees Cook 2022-05-11 8:02 ` Mark Rutland 2022-05-11 8:02 ` Mark Rutland 2022-05-11 14:44 ` Kees Cook 2022-05-11 14:44 ` Kees Cook 2022-05-12 9:14 ` Mark Rutland 2022-05-12 9:14 ` Mark Rutland 2022-05-15 16:17 ` Alexander Popov [this message] 2022-05-15 16:17 ` Alexander Popov 2022-05-24 10:03 ` Mark Rutland 2022-05-24 10:03 ` Mark Rutland 2022-05-26 22:09 ` Alexander Popov 2022-05-26 22:09 ` Alexander Popov 2022-04-27 17:31 ` [PATCH v2 04/13] stackleak: rework stack low bound handling Mark Rutland 2022-04-27 17:31 ` Mark Rutland 2022-04-27 17:31 ` [PATCH v2 05/13] stackleak: clarify variable names Mark Rutland 2022-04-27 17:31 ` Mark Rutland 2022-05-08 20:49 ` Alexander Popov 2022-05-08 20:49 ` Alexander Popov 2022-05-10 13:01 ` Mark Rutland 2022-05-10 13:01 ` Mark Rutland 2022-05-11 3:05 ` Kees Cook 2022-05-11 3:05 ` Kees Cook 2022-04-27 17:31 ` [PATCH v2 06/13] stackleak: rework stack high bound handling Mark Rutland 2022-04-27 17:31 ` Mark Rutland 2022-05-08 21:27 ` Alexander Popov 2022-05-08 21:27 ` Alexander Popov 2022-05-10 11:22 ` Mark Rutland 2022-05-10 11:22 ` Mark Rutland 2022-05-15 16:32 ` Alexander Popov 2022-05-15 16:32 ` Alexander Popov 2022-04-27 17:31 ` [PATCH v2 07/13] stackleak: rework poison scanning Mark Rutland 2022-04-27 17:31 ` Mark Rutland 2022-05-09 13:51 ` Alexander Popov 2022-05-09 13:51 ` Alexander Popov 2022-05-10 13:13 ` Mark Rutland 2022-05-10 13:13 ` Mark Rutland 2022-05-15 17:33 ` Alexander Popov 2022-05-15 17:33 ` Alexander Popov 2022-05-24 13:31 ` Mark Rutland 2022-05-24 13:31 ` Mark Rutland 2022-05-26 23:25 ` Alexander Popov 2022-05-26 23:25 ` Alexander Popov 2022-05-31 18:13 ` Kees Cook 2022-05-31 18:13 ` Kees Cook 2022-06-03 16:55 ` Alexander Popov 2022-06-03 16:55 ` Alexander Popov 2022-04-27 17:31 ` [PATCH v2 08/13] lkdtm/stackleak: avoid spurious failure Mark Rutland 2022-04-27 17:31 ` Mark Rutland 2022-04-27 17:31 ` [PATCH v2 09/13] lkdtm/stackleak: rework boundary management Mark Rutland 2022-04-27 17:31 ` Mark Rutland 2022-05-04 19:07 ` Kees Cook 2022-05-04 19:07 ` Kees Cook 2022-04-27 17:31 ` [PATCH v2 10/13] lkdtm/stackleak: prevent unexpected stack usage Mark Rutland 2022-04-27 17:31 ` Mark Rutland 2022-04-27 17:31 ` [PATCH v2 11/13] lkdtm/stackleak: check stack boundaries Mark Rutland 2022-04-27 17:31 ` Mark Rutland 2022-04-27 17:31 ` [PATCH v2 12/13] stackleak: add on/off stack variants Mark Rutland 2022-04-27 17:31 ` Mark Rutland 2022-04-27 17:31 ` [PATCH v2 13/13] arm64: entry: use stackleak_erase_on_task_stack() Mark Rutland 2022-04-27 17:31 ` Mark Rutland 2022-05-04 16:42 ` Catalin Marinas 2022-05-04 16:42 ` Catalin Marinas 2022-05-04 19:16 ` [PATCH v2 00/13] stackleak: fixes and rework Kees Cook 2022-05-04 19:16 ` Kees Cook
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=8d8061c4-2a3e-cb3a-00c9-677fa8899058@linux.com \ --to=alex.popov@linux.com \ --cc=akpm@linux-foundation.org \ --cc=catalin.marinas@arm.com \ --cc=keescook@chromium.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=luto@kernel.org \ --cc=mark.rutland@arm.com \ --cc=will@kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.