linux-kernel - Re: [bug] block subsystem related crash with latest -git

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: <alpine.LFD.0.999.0710171022340.26902@woody.linux-foundation.org>
Date:	Wed, 17 Oct 2007 10:52:13 -0700 (PDT)
From:	Linus Torvalds <torvalds@...ux-foundation.org>
To:	Jens Axboe <jens.axboe@...cle.com>
cc:	Ingo Molnar <mingo@...e.hu>, linux-kernel@...r.kernel.org,
	Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [bug] block subsystem related crash with latest -git

On Wed, 17 Oct 2007, Jens Axboe wrote:
> 
> OK, the below should actually be safe, I don't know why I talked myself
> into the next_sg stuff in the beginning. It's always safe to zero sg,
> since it's a valid entry - nothing to save in ->page. Ingo, does this
> work for you?

I really don't think this should work.

Doing "sg_next()" on a valid sg is *always* ok. So if the old code didn't 
work, then "sg" wasn't valid to start with (and the code *after* the 
sg_next() would have oopsed even if you try to avoid using sg_next.

So avoiding the "sg_next()" on the last entry is pointless. 

Also, your patch makes the code almost totally unreadable, with that 
subtle issue of the "if (bvprv && cluster)" case not triggering on the 
first case, so the NULL initial sg is "safe".

So at a guess, I think the *real* problem is simply that the passed-in 
sglist was just too small. What guarantees that the sg list allocation 
(apparently done by scsi_alloc_sgtable()) is big enough? 

If I read things right, scsi_alloc_sgtable() will allocate "cmd->use_sg" 
SG enties, no? But I also notice that it does not seem to initialize the 
SG allocation, so those SG entries contain random crap - including, 
perhaps, a random - and bogus - chain pointer in sg->page..

Yes, we set sh->page *if* we create a chain, but if we don't chain, we 
leave the old random contents around which in turn may include old and 
stale chain pointers. Or am I missing something?

So when you added that "memset(sg, 0, sizeof(*sg))" into blk_rq_map_sg(), 
you did it way too late - it needs to be done when the sg chain is 
allocated, and for every entry (and then the "link" entry needs to be 
linked in separately)

I think.

		Linus

-
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/