linux-kernel - Re: [dm-devel] [PATCH v3 01/26] block: Fix a buffer overrun in bio_integrity

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]

Message-ID: <20121002220744.GW26488@google.com>
Date:	Tue, 2 Oct 2012 15:07:47 -0700
From:	Kent Overstreet <koverstreet@...gle.com>
To:	Vivek Goyal <vgoyal@...hat.com>
Cc:	linux-bcache@...r.kernel.org, linux-kernel@...r.kernel.org,
	dm-devel@...hat.com, axboe@...nel.dk,
	"Martin K. Petersen" <martin.petersen@...cle.com>, tj@...nel.org
Subject: Re: [dm-devel] [PATCH v3 01/26] block: Fix a buffer overrun in
 bio_integrity_split()

On Tue, Oct 02, 2012 at 05:58:45PM -0400, Vivek Goyal wrote:
> On Tue, Oct 02, 2012 at 02:01:43PM -0700, Kent Overstreet wrote:
> > I'm honestly not sure what your complaint about my bugfix patch was -
> > it's small and complete, it does fix the bug. I don't follow why you
> > think we need to introduce the bip->bio_vec pointer early...
> 
> I think having iv1 and iv2 and then not even accessing these using 
> bp->iv1 and bp->iv2 is a bad idea even for bugfix.
> 
> I have never seen a code which says, hey I have defined two fields in a
> struct but, don't access those fields directly(as there might be padding
> issues). These fields are just there for blocking a chunk of memory but are
> never meant to be accessed directly. I think, that's what my issue is. It
> is bad programming (does not matter whether it is bug fix or not).
> 
> For your series it probably is still fine as you will overide it pretty
> soon but what about stable. Anybody looking at that code might want
> to say, hey why not directly initialize bp->iv1 instead of trying to
> do *bp->bip1.bip_vec. And everybody will say, yes looks fine and boom
> a bug is introduced because we did bad programming.

Ok. It's definitely a bit weird and unusual, and if I wasn't getting rid
of it in the next patch it would definitely merit a comment.

For stable... wtf would they be making that kind of change for, and
without reading the relevant code?

Eh, maybe I will stick in that comment and take it out in the next
patch.
--
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/