Date: Tue, 20 Dec 2022 19:32:04 +0000 From: Andrew Turner <andrew@fubar.geek.nz> To: Doug Moore <dougm@freebsd.org> Cc: "src-committers@freebsd.org" <src-committers@FreeBSD.org>, "dev-commits-src-all@freebsd.org" <dev-commits-src-all@FreeBSD.org>, "dev-commits-src-main@freebsd.org" <dev-commits-src-main@FreeBSD.org> Subject: Re: git: e0e8d0c8d694 - main - iommu_gas: consolidate find_space helpers Message-ID: <5325DB40-8B13-4B12-8C0E-86352003132E@fubar.geek.nz> In-Reply-To: <202207101939.26AJdeGp023355@gitrepo.freebsd.org> References: <202207101939.26AJdeGp023355@gitrepo.freebsd.org>
index | next in thread | previous in thread | raw e-mail
[-- Attachment #1 --] > On 10 Jul 2022, at 20:39, Doug Moore <dougm@freebsd.org> wrote: > > The branch main has been updated by dougm: > > URL: https://cgit.FreeBSD.org/src/commit/?id=e0e8d0c8d69459c7128e6fd4fb537892445ce710 > > commit e0e8d0c8d69459c7128e6fd4fb537892445ce710 > Author: Doug Moore <dougm@FreeBSD.org> > AuthorDate: 2022-07-10 19:24:23 +0000 > Commit: Doug Moore <dougm@FreeBSD.org> > CommitDate: 2022-07-10 19:24:23 +0000 > > iommu_gas: consolidate find_space helpers > > Merge lowermatch and uppermatch into find_space. Eliminate uppermatch > recursion. Merge match_insert into match_one and eliminate some > redundant calculation. Move some initialization out of find_space and > into map (and out from under a lock). > This commit introduced an integer overflow that breaks the iommu on arm64. In iommu_gas_find_space it adds "addr = a->common->lowaddr + 1;”, however when lowaddr is (bus_addr_t)-1 it will overflow making addr 0. We then use this to set the bounds in iommu_gas_match_one, however this will fail as the bounds are 0, 0. When this first loops fails it then searches for address space above highaddr, however as this is above the maximum address this loop is never run. As far as I can tell it works on amd64 because of another integer overflow in the loop to find memory above highaddr where, due to it also overflowing, it incorrectly uses 0 and domain->end as the bounds. It can get into this case as curr->last == (bus_addr_t)-1 so the RB_PARENT loop will exit with a non-NULL curr pointer. D37756 works around this issue by making arm64 behave in the same way as amd64, however I don’t think we should be entering the second loop with a highaddr of (bus_addr_t)-1 as it may lead to an invalid address being allocated, e.g. If the first loop failed because it is out of usable address space. Andrew [-- Attachment #2 --] <html><head><meta http-equiv="Content-Type" content="text/html; charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class=""><br class=""><div><blockquote type="cite" class=""><div class="">On 10 Jul 2022, at 20:39, Doug Moore <<a href="mailto:dougm@freebsd.org" class="">dougm@freebsd.org</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div class="">The branch main has been updated by dougm:<br class=""><br class="">URL: <a href="https://cgit.FreeBSD.org/src/commit/?id=e0e8d0c8d69459c7128e6fd4fb537892445ce710" class="">https://cgit.FreeBSD.org/src/commit/?id=e0e8d0c8d69459c7128e6fd4fb537892445ce710</a><br class=""><br class="">commit e0e8d0c8d69459c7128e6fd4fb537892445ce710<br class="">Author: Doug Moore <<a href="mailto:dougm@FreeBSD.org" class="">dougm@FreeBSD.org</a>><br class="">AuthorDate: 2022-07-10 19:24:23 +0000<br class="">Commit: Doug Moore <<a href="mailto:dougm@FreeBSD.org" class="">dougm@FreeBSD.org</a>><br class="">CommitDate: 2022-07-10 19:24:23 +0000<br class=""><br class=""> iommu_gas: consolidate find_space helpers<br class=""><br class=""> Merge lowermatch and uppermatch into find_space. Eliminate uppermatch<br class=""> recursion. Merge match_insert into match_one and eliminate some<br class=""> redundant calculation. Move some initialization out of find_space and<br class=""> into map (and out from under a lock).<br class=""><br class=""></div></div></blockquote><div><br class=""></div><div>This commit introduced an integer overflow that breaks the iommu on arm64.</div><div><br class=""></div><div>In iommu_gas_find_space it adds "addr = a->common->lowaddr + 1;”, however when lowaddr is (bus_addr_t)-1 it will overflow making addr 0. We then use this to set the bounds in iommu_gas_match_one, however this will fail as the bounds are 0, 0.</div><div><br class=""></div><div>When this first loops fails it then searches for address space above highaddr, however as this is above the maximum address this loop is never run.</div><div><br class=""></div><div>As far as I can tell it works on amd64 because of another integer overflow in the loop to find memory above highaddr where, due to it also overflowing, it incorrectly uses 0 and domain->end as the bounds. It can get into this case as curr->last == (bus_addr_t)-1 so the RB_PARENT loop will exit with a non-NULL curr pointer.</div><div><span style="color: rgba(0, 0, 0, 0.85); font-family: "Helvetica Neue";" class=""><br class=""></span></div><div><font face="Helvetica Neue" class=""><span style="color: rgba(0, 0, 0, 0.85);" class="">D37756 works around this issue by </span><span style="caret-color: rgba(0, 0, 0, 0.85); color: rgba(0, 0, 0, 0.85);" class="">making</span><span style="color: rgba(0, 0, 0, 0.85);" class=""> arm64 behave in the same </span><span style="caret-color: rgba(0, 0, 0, 0.85); color: rgba(0, 0, 0, 0.85);" class="">way as amd64</span><span style="color: rgba(0, 0, 0, 0.85);" class="">, however I don</span><span style="caret-color: rgba(0, 0, 0, 0.85); color: rgba(0, 0, 0, 0.85);" class="">’</span><span style="color: rgba(0, 0, 0, 0.85);" class="">t think we should be entering the second loop with a highaddr of </span></font>(bus_addr_t)-1 as it may lead to an invalid address being allocated, e.g. If the first loop failed because it is out of usable address space.</div><div><br class=""></div><div>Andrew</div><div><br class=""></div></div></body></html>help
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?5325DB40-8B13-4B12-8C0E-86352003132E>