linux-kernel - Re: [S390] cio: kernel stack overflow.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 30 Aug 2006 14:52:33 -0300
From:	"Julio Auto" <mindvortex@...il.com>
To:	"David Wagner" <daw-usenet@...erner.cs.berkeley.edu>,
	schwidefsky@...ibm.com
Cc:	linux-kernel@...r.kernel.org
Subject: Re: [S390] cio: kernel stack overflow.

On 8/30/06, David Wagner <daw@...berkeley.edu> wrote:
> Have you checked that in all cases all fields of the struct have
> been overwritten?  For instance, look at this:
>
> Martin Schwidefsky  wrote:
> >-      chp->dev = (struct device) {
> >-              .parent  = &css[0]->device,
> >-              .release = chp_release,
> >-      };
> >+      chp->dev.parent = &css[0]->device;
> >+      chp->dev.release = chp_release;
>
> Doesn't this leave chp->dev.bus still holding whatever old value it
> had laying around before?

You're correct. While this eliminates the possibility of getting
random values from the stack, it still leaves space for letting
previous unwanted values unchanged.
Unless, of course, the structure in question is kcalloc()'d (which is
not the case of gdev in the beginning of the patch - I haven't had the
time to check the other cases). But we surely can't rely on that.

>Unless I'm missing something, it looks to
> me like this diff causes a change in the semantics of the code.

I can't see the semantic change.

>
> Perhaps it would be better to memset() the entire struct (chp->dev, in
> this case) to zero, before assigning to individual fields, so there is
> no possibility of old remnant data still being left laying around?

Yes. Since the code can't be depend on the caller passing a zeroed
structure, it's definitely more safe to memset to 0 (or use kcalloc(),
instead of kmalloc(), when the allocation is the routine's own
responsability).

Changing subjects a little bit: where's the stack overflow in the
code? Either I'm too stupid to find it myself by looking at the patch
or the e-mail is mistitled.

Cheers,

    Julio Auto
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/