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>
next in thread | previous in thread | raw e-mail | index | archive | help
--Apple-Mail=_0E18B7F2-35E6-4162-B2D7-A559C3087A83 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 > On 10 Jul 2022, at 20:39, Doug Moore <dougm@freebsd.org> wrote: >=20 > The branch main has been updated by dougm: >=20 > URL: = https://cgit.FreeBSD.org/src/commit/?id=3De0e8d0c8d69459c7128e6fd4fb537892= 445ce710 >=20 > 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 >=20 > iommu_gas: consolidate find_space helpers >=20 > 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). >=20 This commit introduced an integer overflow that breaks the iommu on = arm64. In iommu_gas_find_space it adds "addr =3D a->common->lowaddr + 1;=E2=80=9D= , 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 =3D=3D (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=E2=80=99t 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 --Apple-Mail=_0E18B7F2-35E6-4162-B2D7-A559C3087A83 Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=utf-8 <html><head><meta http-equiv=3D"Content-Type" content=3D"text/html; = charset=3Dutf-8"></head><body style=3D"word-wrap: break-word; = -webkit-nbsp-mode: space; line-break: after-white-space;" class=3D""><br = class=3D""><div><blockquote type=3D"cite" class=3D""><div class=3D"">On = 10 Jul 2022, at 20:39, Doug Moore <<a href=3D"mailto:dougm@freebsd.org"= class=3D"">dougm@freebsd.org</a>> wrote:</div><br = class=3D"Apple-interchange-newline"><div class=3D""><div class=3D"">The = branch main has been updated by dougm:<br class=3D""><br class=3D"">URL: = <a = href=3D"https://cgit.FreeBSD.org/src/commit/?id=3De0e8d0c8d69459c7128e6fd4= fb537892445ce710" = class=3D"">https://cgit.FreeBSD.org/src/commit/?id=3De0e8d0c8d69459c7128e6= fd4fb537892445ce710</a><br class=3D""><br class=3D"">commit = e0e8d0c8d69459c7128e6fd4fb537892445ce710<br class=3D"">Author: = Doug Moore <<a = href=3D"mailto:dougm@FreeBSD.org" class=3D"">dougm@FreeBSD.org</a>><br = class=3D"">AuthorDate: 2022-07-10 19:24:23 +0000<br class=3D"">Commit: = Doug Moore <<a = href=3D"mailto:dougm@FreeBSD.org" class=3D"">dougm@FreeBSD.org</a>><br = class=3D"">CommitDate: 2022-07-10 19:24:23 +0000<br class=3D""><br = class=3D""> iommu_gas: consolidate find_space = helpers<br class=3D""><br class=3D""> Merge lowermatch = and uppermatch into find_space. Eliminate uppermatch<br class=3D""> = recursion. Merge match_insert into match_one and = eliminate some<br class=3D""> redundant calculation. = Move some initialization out of find_space and<br class=3D""> = into map (and out from under a lock).<br class=3D""><br = class=3D""></div></div></blockquote><div><br class=3D""></div><div>This = commit introduced an integer overflow that breaks the iommu on = arm64.</div><div><br class=3D""></div><div>In iommu_gas_find_space = it adds "addr =3D a->common->lowaddr + 1;=E2=80=9D, 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=3D""></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=3D""></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 =3D=3D (bus_addr_t)-1 so = the RB_PARENT loop will exit with a non-NULL curr = pointer.</div><div><span style=3D"color: rgba(0, 0, 0, 0.85); = font-family: "Helvetica Neue";" class=3D""><br = class=3D""></span></div><div><font face=3D"Helvetica Neue" = class=3D""><span style=3D"color: rgba(0, 0, 0, 0.85);" class=3D"">D37756 = works around this issue by </span><span style=3D"caret-color: = rgba(0, 0, 0, 0.85); color: rgba(0, 0, 0, 0.85);" = class=3D"">making</span><span style=3D"color: rgba(0, 0, 0, 0.85);" = class=3D""> arm64 behave in the same </span><span = style=3D"caret-color: rgba(0, 0, 0, 0.85); color: rgba(0, 0, 0, 0.85);" = class=3D"">way as amd64</span><span style=3D"color: rgba(0, 0, 0, = 0.85);" class=3D"">, however I don</span><span style=3D"caret-color: = rgba(0, 0, 0, 0.85); color: rgba(0, 0, 0, 0.85);" = class=3D"">=E2=80=99</span><span style=3D"color: rgba(0, 0, 0, 0.85);" = class=3D"">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=3D""></div><div>Andrew</div><div><br = class=3D""></div></div></body></html>= --Apple-Mail=_0E18B7F2-35E6-4162-B2D7-A559C3087A83--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?5325DB40-8B13-4B12-8C0E-86352003132E>