Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20200728141946.426245-1-yepeilin.cs@gmail.com>
 <20200729115157.8519-1-yepeilin.cs@gmail.com> <20200729125820.GB1840@kadam>
 <f2cf6137-987a-ab41-d88a-6828d46c255f@linux.com> <CAK8P3a20SEoYCrp3jOK32oZc9OkiPv+1KTjNZ2GxLbHpY4WexQ@mail.gmail.com>
 <202007301056.D3BD1805B0@keescook>
In-Reply-To: <202007301056.D3BD1805B0@keescook>
From:   Arnd Bergmann <arnd@arndb.de>
Date:   Thu, 30 Jul 2020 22:45:02 +0200
Message-ID: <CAK8P3a2oUgdaYicdHwWvCY-HqjrcBAEzYA5yc5Gw14RLLoLdug@mail.gmail.com>
Subject: Re: [Linux-kernel-mentees] [PATCH v2] block/floppy: Prevent
 kernel-infoleak in raw_cmd_copyout()
To:     Kees Cook <keescook@chromium.org>
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
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Thu, Jul 30, 2020 at 8:10 PM Kees Cook <keescook@chromium.org> wrote:
> On Thu, Jul 30, 2020 at 10:11:07AM +0200, Arnd Bergmann wrote:
>
> 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):

Ok, so I read that correctly, thanks for confirming.

> >
> >    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! :)

For some reason I remembered this not turning into a memcpy()
somewhere, but I can't reproduce it in any of my recent attempts,
just like what Denis found.

> > 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 would assume that at least with C99 it is more like the
"whole structure copy", based on the standard language of

  "The value of the compound literal is that of an unnamed
  object initialized by the initializer list. If the compound literal
  occurs outside the body of a function, the object has static
  storage duration; otherwise, it has automatic storage duration
  associated with the enclosing block."

> I will go add more tests...

Thanks!

         Arnd