linux-kernel - Re: [PATCH] amd_iommu: Fix leak in free

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: <1371757608.30572.18.camel@ul30vt.home>
Date:	Thu, 20 Jun 2013 13:46:48 -0600
From:	Alex Williamson <alex.williamson@...hat.com>
To:	Joerg Roedel <joro@...tes.org>
Cc:	iommu@...ts.linux-foundation.org, ddutile@...hat.com,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] amd_iommu: Fix leak in free_pagetable()

On Thu, 2013-06-20 at 21:26 +0200, Joerg Roedel wrote:
> On Thu, Jun 20, 2013 at 01:08:00PM -0600, Alex Williamson wrote:
> > On Thu, 2013-06-20 at 20:28 +0200, Joerg Roedel wrote:
> > > Hmm, actually a recursive version would make more sense here. But since
> > > recursion is a bad idea in the kernel, how about this approach instead:
> > 
> > It's a fixed maximum depth of recursion though, is it really that taboo?
> 
> Well, a recursive version would be better here because it has lower icache
> impact than my patch. But the risk is if there is a bug in recursive
> code and it goes on forever it will overwrite random kernel memory, with
> undefined, potentially very bad, results (like data corruption, killed
> filesystems and so on).
> 
> So in my opinion it is better to not take that risk whenever possible.

But that's true of a bug in any kernel code.  I think the only danger
unique to recursion is using too much stack space, but I doubt that's
really an issue for a tiny function with a fixed depth like this.  In
case you didn't notice, I did send a recursive version along with the
flat version, but I stupidly used the same subject for both.  It's a
little less code and a tiny bit easier to understand than the macro
version.  Up to you though.  Thanks,

Alex

> > > The IOMMU pagetables can have up to 3 levels, but the code
> > 
> > s/3/6/
> 
> Right, thanks.
> 
> > Seems like it should do the right thing
> 
> Ok, I also gave it some testing (started a vm, assigned and de-assigned
> a device), so it should work as expected.
> 
> > Reviewed-by: Alex Williamson <alex.williamson@...hat.com>
> 
> Thanks,
> 
> 	Joerg
> 
> 



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