linux-kernel - Re: [PATCH] SPARC64: fix iommu sg chaining

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: <20071017182401X.fujita.tomonori@lab.ntt.co.jp>
Date:	Wed, 17 Oct 2007 18:24:01 +0900
From:	FUJITA Tomonori <fujita.tomonori@....ntt.co.jp>
To:	jens.axboe@...cle.com
Cc:	davem@...emloft.net, fujita.tomonori@....ntt.co.jp,
	linux-kernel@...r.kernel.org, linux-scsi@...r.kernel.org
Subject: Re: [PATCH] SPARC64: fix iommu sg chaining

On Wed, 17 Oct 2007 11:16:29 +0200
Jens Axboe <jens.axboe@...cle.com> wrote:

> On Wed, Oct 17 2007, David Miller wrote:
> > From: Jens Axboe <jens.axboe@...cle.com>
> > Date: Wed, 17 Oct 2007 10:45:28 +0200
> > 
> > > Righto, it's invalid to call sg_next() on the last entry!
> > 
> > Unfortunately, that's what the sparc64 code wanted to do, this
> > transformation in the sparc64 sg chaining patch is not equilavent:
> > 
> > -	struct scatterlist *sg_end = sg + nelems;
> > +	struct scatterlist *sg_end = sg_last(sg, nelems);
> >  ...
> > -			while (sg < sg_end &&
> > +			while (sg != sg_end &&
> 
> Auch indeed. That'd probably be better as a
> 
>         do {
>                 ...
>         } while (sg != sg_end);
> 
> > No, that's not what the code was doing.  The while loop
> > has to process the last entry in the list,
> > 
> > We really needed "sg_end" to be "one past the last element",
> > rather than "the last element".
> > 
> > Since you say that sg_next() on the last entry is illegal,
> > and that's what this code would have done to try and reach
> > loop termination (it doesn't actually derefrence that
> > "end plus one" scatterlist entry) I'll try to code this up
> > some other way.
> > 
> > Besides, sg_last() is so absurdly expensive, it has to walk the entire
> > chain in the chaining case.  So better to implement this without it.
> 
> It is, sg_last() should really not be used a lot since it'll leaf
> through the entire sg list. People should either keep count of the
> number of entries so that they know when they are dealing with the last
> valid entry. Or use the for_each_sg() loop helper, if possible.
> 
> Drivers are usually very simple, the iommu code does more sg tricks and
> thus is more complex to audit.

Can we just remove sg_last?


> > I would suggest that other sg_last() uses be audited for the same bug.
> 
> Agree.

Only libata, I think.
-
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/