linux-block.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Kees Cook <keescook@chromium.org>
To: Arnd Bergmann <arnd@arndb.de>
Cc: Denis Efremov <efremov@linux.com>,
	Dan Carpenter <dan.carpenter@oracle.com>,
	Peilin Ye <yepeilin.cs@gmail.com>, Jens Axboe <axboe@kernel.dk>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	linux-block <linux-block@vger.kernel.org>,
	linux-kernel-mentees@lists.linuxfoundation.org
Subject: Re: [Linux-kernel-mentees] [PATCH v2] block/floppy: Prevent kernel-infoleak in raw_cmd_copyout()
Date: Thu, 30 Jul 2020 11:10:36 -0700	[thread overview]
Message-ID: <202007301056.D3BD1805B0@keescook> (raw)
In-Reply-To: <CAK8P3a20SEoYCrp3jOK32oZc9OkiPv+1KTjNZ2GxLbHpY4WexQ@mail.gmail.com>

On Thu, Jul 30, 2020 at 10:11:07AM +0200, Arnd Bergmann wrote:
> > On Wed, Jul 29, 2020 at 3:22 PM Denis Efremov <efremov@linux.com> wrote:
> 
> > And checked for leaks on x86_64 with the script test.sh
> > $ cat test.sh
> > #!/bin/bash
> >
> > for i in 4.8 5 6 7 8 9 10
> > do
> > ./run_container.sh gcc-$i $(pwd)/src $(pwd)/out bash -c 'gcc test.c; ./a.out'
> > ./run_container.sh gcc-$i $(pwd)/src $(pwd)/out bash -c 'gcc -O2 test.c; ./a.out'
> > ./run_container.sh gcc-$i $(pwd)/src $(pwd)/out bash -c 'gcc -O3 test.c; ./a.out'
> > done
> >
> > No leaks reported. Is it really possible this this kind of init, i.e. cmd = *ptr?
> 
> The problem is that the behavior is dependent not just on the compiler
> version but
> also optimization flags, target architecture and specific structure
> layouts. Most
> of the time, both gcc and clang will initialize the whole structure
> rather than just
> the individual members, but you still can't be sure that this is true
> for all configurations
> that this code runs on, except by using CONFIG_GCC_PLUGIN_STRUCTLEAK.
> 
> Kees pointed me to the lib/test_stackinit.c file in the kernel in which he has
> collected a number of combinations that are known to trigger the problem.
> 
> What I see there though are only cases of struct initializers like
> 
>   struct test_big_hole var = { .one = arg->one, .two=arg->two, .three
> = arg->three, .four = arg->four };

test_stackinit.c intended to use six cases (where "full" is in the sense
of "all members are named", this is intentionally testing the behavior
of padding hole initialization):

full static initialization:

          = { .one = 0,
              .two = 0,
              .three = 0,
              .four = 0,
          };

partial static init:

          = { .two = 0, };

full dynamic init:

          = { .one = arg->one,
              .two = arg->two,
              .three = arg->three,
              .four = arg->four,
          };

partial dynamic init:

          = { .two = arg->two, };

full runtime init:

          var.one = 0;
          var.two = 0;
          var.three = 0;
          memset(&var.four, 0, sizeof(var.four));

partial runtime init:

          var.two = 0;

(It seems in refactoring I botched the "full static initialization"
case, which I'll go fix separately.)

> but not the syntax used in the floppy driver:
> 
>    struct test_big_hole var = *arg;

So this one is a "whole structure copy" which I didn't have any tests
for, since I'd (perhaps inappropriately) assumed would be accomplished
with memcpy() internally, which means the incoming "*arg"'s padding holes
would be copied as-is. If the compiler is actually doing per-member copies
and leaving holes in "var" untouched, that's unexpected, so clearly that
needs to be added to test_stackinit.c! :)

> or the a constructor like
> 
>   struct test_big_hole var;
>   var = (struct test_big_hole){ .one = arg->one, .two=arg->two, .three
> = arg->three, .four = arg->four };
> 
> Kees, do you know whether those two would behave differently?
> Would it make sense to also check for those, or am I perhaps
> misreading your code and it already gets checked?

I *think* the above constructor would be covered under "full runtime
init", but it does also seem likely it would be handled similarly to
the "whole structure copy" in the previous example. I will go add more
tests...

-- 
Kees Cook

  reply	other threads:[~2020-07-30 18:10 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-07-28 14:19 [Linux-kernel-mentees] [PATCH] block/floppy: Prevent kernel-infoleak in raw_cmd_copyout() Peilin Ye
2020-07-29  9:07 ` Denis Efremov
2020-07-29  9:18 ` Denis Efremov
2020-07-29  9:46   ` Peilin Ye
2020-07-29 11:51 ` [Linux-kernel-mentees] [PATCH v2] " Peilin Ye
2020-07-29 12:58   ` Dan Carpenter
2020-07-29 13:22     ` Denis Efremov
2020-07-29 13:42       ` Dan Carpenter
2020-07-30  8:11       ` Arnd Bergmann
2020-07-30 18:10         ` Kees Cook [this message]
2020-07-30 20:45           ` Arnd Bergmann
2021-07-23 22:22             ` 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=202007301056.D3BD1805B0@keescook \
    --to=keescook@chromium.org \
    --cc=arnd@arndb.de \
    --cc=axboe@kernel.dk \
    --cc=dan.carpenter@oracle.com \
    --cc=efremov@linux.com \
    --cc=linux-block@vger.kernel.org \
    --cc=linux-kernel-mentees@lists.linuxfoundation.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=yepeilin.cs@gmail.com \
    /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: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).