The recursive_loop() function is intended as a diagnostic to ensure that
exhausting the stack is caught and mitigated. Currently, it uses
pr_info() to ensure that the function has side effects that the compiler
cannot simply optimize away, so that the stack footprint does not get
reduced inadvertently.
The typical mitigation for stack overflow is to kill the task, and this
overflow may occur inside the call to pr_info(), which means it could be
holding the console lock when this happens. This means that the console
lock is never going to be released again, preventing the diagnostic
prints related to the stack overflow handling from being visible on the
console.
So let's replace the call to pr_info() with a call to
memzero_explicit(), which is not a 'magic' function name like memset()
or memcpy(), which the compiler may replace with plain loads and stores.
To ensure that the stack frames are nested rather than tail-called, put
the call to memzero_explicit() after the recursive call.
Signed-off-by: Ard Biesheuvel <[email protected]>
---
drivers/misc/lkdtm/bugs.c | 16 +++++++++-------
1 file changed, 9 insertions(+), 7 deletions(-)
diff --git a/drivers/misc/lkdtm/bugs.c b/drivers/misc/lkdtm/bugs.c
index 4282b625200f..41fa558675c4 100644
--- a/drivers/misc/lkdtm/bugs.c
+++ b/drivers/misc/lkdtm/bugs.c
@@ -41,20 +41,22 @@ static DEFINE_SPINLOCK(lock_me_up);
* Make sure compiler does not optimize this function or stack frame away:
* - function marked noinline
* - stack variables are marked volatile
- * - stack variables are written (memset()) and read (pr_info())
- * - function has external effects (pr_info())
- * */
+ * - stack variables are written (memset()) and read (buf[..] passed as arg)
+ * - function may have external effects (memzero_explicit())
+ * - no tail recursion possible
+ */
static int noinline recursive_loop(int remaining)
{
volatile char buf[REC_STACK_SIZE];
+ volatile int ret;
memset((void *)buf, remaining & 0xFF, sizeof(buf));
- pr_info("loop %d/%d ...\n", (int)buf[remaining % sizeof(buf)],
- recur_count);
if (!remaining)
- return 0;
+ ret = 0;
else
- return recursive_loop(remaining - 1);
+ ret = recursive_loop((int)buf[remaining % sizeof(buf)] - 1);
+ memzero_explicit((void *)buf, sizeof(buf));
+ return ret;
}
/* If the depth is negative, use the default, otherwise keep parameter. */
--
2.30.2
On Thu, 7 Oct 2021 10:12:35 +0200, Ard Biesheuvel wrote:
> The recursive_loop() function is intended as a diagnostic to ensure that
> exhausting the stack is caught and mitigated. Currently, it uses
> pr_info() to ensure that the function has side effects that the compiler
> cannot simply optimize away, so that the stack footprint does not get
> reduced inadvertently.
>
> The typical mitigation for stack overflow is to kill the task, and this
> overflow may occur inside the call to pr_info(), which means it could be
> holding the console lock when this happens. This means that the console
> lock is never going to be released again, preventing the diagnostic
> prints related to the stack overflow handling from being visible on the
> console.
>
> [...]
Applied to for-next/lkdtm, thanks!
[1/1] lkdtm: avoid printk() in recursive_loop()
https://git.kernel.org/kees/c/700fa7d22233
--
Kees Cook
On Thu, 7 Oct 2021 at 18:59, Kees Cook <[email protected]> wrote:
>
> On Thu, 7 Oct 2021 10:12:35 +0200, Ard Biesheuvel wrote:
> > The recursive_loop() function is intended as a diagnostic to ensure that
> > exhausting the stack is caught and mitigated. Currently, it uses
> > pr_info() to ensure that the function has side effects that the compiler
> > cannot simply optimize away, so that the stack footprint does not get
> > reduced inadvertently.
> >
> > The typical mitigation for stack overflow is to kill the task, and this
> > overflow may occur inside the call to pr_info(), which means it could be
> > holding the console lock when this happens. This means that the console
> > lock is never going to be released again, preventing the diagnostic
> > prints related to the stack overflow handling from being visible on the
> > console.
> >
> > [...]
>
> Applied to for-next/lkdtm, thanks!
>
> [1/1] lkdtm: avoid printk() in recursive_loop()
> https://git.kernel.org/kees/c/700fa7d22233
>
Ping?
On Thu, Dec 16, 2021 at 12:16:19PM +0100, Ard Biesheuvel wrote:
> On Thu, 7 Oct 2021 at 18:59, Kees Cook <[email protected]> wrote:
> >
> > On Thu, 7 Oct 2021 10:12:35 +0200, Ard Biesheuvel wrote:
> > > The recursive_loop() function is intended as a diagnostic to ensure that
> > > exhausting the stack is caught and mitigated. Currently, it uses
> > > pr_info() to ensure that the function has side effects that the compiler
> > > cannot simply optimize away, so that the stack footprint does not get
> > > reduced inadvertently.
> > >
> > > The typical mitigation for stack overflow is to kill the task, and this
> > > overflow may occur inside the call to pr_info(), which means it could be
> > > holding the console lock when this happens. This means that the console
> > > lock is never going to be released again, preventing the diagnostic
> > > prints related to the stack overflow handling from being visible on the
> > > console.
> > >
> > > [...]
> >
> > Applied to for-next/lkdtm, thanks!
> >
> > [1/1] lkdtm: avoid printk() in recursive_loop()
> > https://git.kernel.org/kees/c/700fa7d22233
> >
>
> Ping?
Eek, I didn't send my -next pull to Greg yet. Done now; thanks for the
ping! :)
-Kees
--
Kees Cook