Date: Fri, 23 Jun 2023 11:49:42 -0700 From: Enji Cooper <yaneurabeya@gmail.com> To: Mark Johnston <markj@FreeBSD.org> Cc: src-committers@freebsd.org, dev-commits-src-all@freebsd.org, dev-commits-src-main@freebsd.org Subject: Re: git: fc915f1be145 - main - pseudofs: Fix a potential out-of-bounds access in pfs_lookup() Message-ID: <C69534AE-7470-4A4D-AEDF-41B6E64DD214@gmail.com> In-Reply-To: <ZJXAosyybBm10gKx@nuc> References: <202306231509.35NF9sAk037726@gitrepo.freebsd.org> <0BAC85B7-6A67-4F6E-87B8-97ABD2FF7075@gmail.com> <ZJXAosyybBm10gKx@nuc>
next in thread | previous in thread | raw e-mail | index | archive | help
--Apple-Mail=_F9A75F62-F1BB-43C2-8C19-EBC79BAE7801 Content-Type: multipart/alternative; boundary="Apple-Mail=_D51E7714-1DA8-491A-844D-DF0846B5FC1D" --Apple-Mail=_D51E7714-1DA8-491A-844D-DF0846B5FC1D Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 > On Jun 23, 2023, at 8:56 AM, Mark Johnston <markj@FreeBSD.org> wrote: =E2=80=A6 >> Naive question: should this be an && conditional or an || = conditional? >=20 > It should be &&. Using || here would reintroduce the original bug. > If strncmp(pname, pn->pn_name, namelen) =3D=3D 0, then > strlen(pn->pn_name) >=3D namelen, and pn->pn_name is nul-terminated, = so it > is safe to check pn->pn_name[namelen] =3D=3D '\0'. >=20 >> If the former, could this be simplified by using a direct NUL char = equality check instead of using strncmp? >=20 > I'm not sure what you mean by this. This code is simply checking > whether pname and pn->pn_name are the same string, without assuming = that > pname is nul-terminated. I completely misread the conditional when I sent out my email. = After you pointed out the obvious part dealing with namelen, it = doesn=E2=80=99t make sense for the conditionals to exist by themselves. Thanks for the explanation :)! -Enji --Apple-Mail=_D51E7714-1DA8-491A-844D-DF0846B5FC1D 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 = Jun 23, 2023, at 8:56 AM, Mark Johnston <<a = href=3D"mailto:markj@FreeBSD.org" class=3D"">markj@FreeBSD.org</a>> = wrote:</div></blockquote><div><br class=3D""></div>=E2=80=A6</div><div><br= class=3D""><blockquote type=3D"cite" class=3D""><div = class=3D""><blockquote type=3D"cite" style=3D"font-family: Helvetica; = font-size: 12px; font-style: normal; font-variant-caps: normal; = font-weight: 400; letter-spacing: normal; orphans: auto; text-align: = start; text-indent: 0px; text-transform: none; white-space: normal; = widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; = -webkit-text-stroke-width: 0px; text-decoration: none;" class=3D"">Naive = question: should this be an && conditional or an || = conditional?<br class=3D""></blockquote><br style=3D"caret-color: rgb(0, = 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; = font-variant-caps: normal; font-weight: 400; letter-spacing: normal; = text-align: start; text-indent: 0px; text-transform: none; white-space: = normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; = text-decoration: none;" class=3D""><span style=3D"caret-color: rgb(0, 0, = 0); font-family: Helvetica; font-size: 12px; font-style: normal; = font-variant-caps: normal; font-weight: 400; letter-spacing: normal; = text-align: start; text-indent: 0px; text-transform: none; white-space: = normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; = text-decoration: none; float: none; display: inline !important;" = class=3D"">It should be &&. Using || here would = reintroduce the original bug.</span><br style=3D"caret-color: rgb(0, 0, = 0); font-family: Helvetica; font-size: 12px; font-style: normal; = font-variant-caps: normal; font-weight: 400; letter-spacing: normal; = text-align: start; text-indent: 0px; text-transform: none; white-space: = normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; = text-decoration: none;" class=3D""><span style=3D"caret-color: rgb(0, 0, = 0); font-family: Helvetica; font-size: 12px; font-style: normal; = font-variant-caps: normal; font-weight: 400; letter-spacing: normal; = text-align: start; text-indent: 0px; text-transform: none; white-space: = normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; = text-decoration: none; float: none; display: inline !important;" = class=3D"">If strncmp(pname, pn->pn_name, namelen) =3D=3D 0, = then</span><br style=3D"caret-color: rgb(0, 0, 0); font-family: = Helvetica; font-size: 12px; font-style: normal; font-variant-caps: = normal; font-weight: 400; letter-spacing: normal; text-align: start; = text-indent: 0px; text-transform: none; white-space: normal; = word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: = none;" class=3D""><span style=3D"caret-color: rgb(0, 0, 0); font-family: = Helvetica; font-size: 12px; font-style: normal; font-variant-caps: = normal; font-weight: 400; letter-spacing: normal; text-align: start; = text-indent: 0px; text-transform: none; white-space: normal; = word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: = none; float: none; display: inline !important;" = class=3D"">strlen(pn->pn_name) >=3D namelen, and pn->pn_name is = nul-terminated, so it</span><br style=3D"caret-color: rgb(0, 0, 0); = font-family: Helvetica; font-size: 12px; font-style: normal; = font-variant-caps: normal; font-weight: 400; letter-spacing: normal; = text-align: start; text-indent: 0px; text-transform: none; white-space: = normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; = text-decoration: none;" class=3D""><span style=3D"caret-color: rgb(0, 0, = 0); font-family: Helvetica; font-size: 12px; font-style: normal; = font-variant-caps: normal; font-weight: 400; letter-spacing: normal; = text-align: start; text-indent: 0px; text-transform: none; white-space: = normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; = text-decoration: none; float: none; display: inline !important;" = class=3D"">is safe to check pn->pn_name[namelen] =3D=3D = '\0'.</span><br style=3D"caret-color: rgb(0, 0, 0); font-family: = Helvetica; font-size: 12px; font-style: normal; font-variant-caps: = normal; font-weight: 400; letter-spacing: normal; text-align: start; = text-indent: 0px; text-transform: none; white-space: normal; = word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: = none;" class=3D""><br style=3D"caret-color: rgb(0, 0, 0); font-family: = Helvetica; font-size: 12px; font-style: normal; font-variant-caps: = normal; font-weight: 400; letter-spacing: normal; text-align: start; = text-indent: 0px; text-transform: none; white-space: normal; = word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: = none;" class=3D""><blockquote type=3D"cite" style=3D"font-family: = Helvetica; font-size: 12px; font-style: normal; font-variant-caps: = normal; font-weight: 400; letter-spacing: normal; orphans: auto; = text-align: start; text-indent: 0px; text-transform: none; white-space: = normal; widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; = -webkit-text-stroke-width: 0px; text-decoration: none;" class=3D"">If = the former, could this be simplified by using a direct NUL char equality = check instead of using strncmp?<br class=3D""></blockquote><br = style=3D"caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: = 12px; font-style: normal; font-variant-caps: normal; font-weight: 400; = letter-spacing: normal; text-align: start; text-indent: 0px; = text-transform: none; white-space: normal; word-spacing: 0px; = -webkit-text-stroke-width: 0px; text-decoration: none;" class=3D""><span = style=3D"caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: = 12px; font-style: normal; font-variant-caps: normal; font-weight: 400; = letter-spacing: normal; text-align: start; text-indent: 0px; = text-transform: none; white-space: normal; word-spacing: 0px; = -webkit-text-stroke-width: 0px; text-decoration: none; float: none; = display: inline !important;" class=3D"">I'm not sure what you mean by = this. This code is simply checking</span><br style=3D"caret-color: = rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: = normal; font-variant-caps: normal; font-weight: 400; letter-spacing: = normal; text-align: start; text-indent: 0px; text-transform: none; = white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; = text-decoration: none;" class=3D""><span style=3D"caret-color: rgb(0, 0, = 0); font-family: Helvetica; font-size: 12px; font-style: normal; = font-variant-caps: normal; font-weight: 400; letter-spacing: normal; = text-align: start; text-indent: 0px; text-transform: none; white-space: = normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; = text-decoration: none; float: none; display: inline !important;" = class=3D"">whether pname and pn->pn_name are the same string, without = assuming that</span><br style=3D"caret-color: rgb(0, 0, 0); font-family: = Helvetica; font-size: 12px; font-style: normal; font-variant-caps: = normal; font-weight: 400; letter-spacing: normal; text-align: start; = text-indent: 0px; text-transform: none; white-space: normal; = word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: = none;" class=3D""><span style=3D"caret-color: rgb(0, 0, 0); font-family: = Helvetica; font-size: 12px; font-style: normal; font-variant-caps: = normal; font-weight: 400; letter-spacing: normal; text-align: start; = text-indent: 0px; text-transform: none; white-space: normal; = word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: = none; float: none; display: inline !important;" class=3D"">pname is = nul-terminated.</span></div></blockquote></div><br class=3D""><div = class=3D""><span class=3D"Apple-tab-span" style=3D"white-space:pre"> = </span>I completely misread the conditional when I sent out my email. = After you pointed out the obvious part dealing with namelen, it = doesn=E2=80=99t make sense for the conditionals to exist by = themselves.</div><div class=3D"">Thanks for the explanation = :)!</div><div class=3D"">-Enji</div></body></html>= --Apple-Mail=_D51E7714-1DA8-491A-844D-DF0846B5FC1D-- --Apple-Mail=_F9A75F62-F1BB-43C2-8C19-EBC79BAE7801 Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename=signature.asc Content-Type: application/pgp-signature; name=signature.asc Content-Description: Message signed with OpenPGP -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEtvtxN6kOllEF3nmX5JFNMZeDGN4FAmSV6UYACgkQ5JFNMZeD GN50Bg/+PM8qBKLJFGqnq9CpweSVddfEAjDYZ7R9ETaxddyytqqEkW11GR3aUDNy N8lmy5ux59y6l2pMeCutJ0I6vCRX+taXqZEwOPohlEMtX2bYw/om1vQBc92JVmdu XcVyng8l09ZnYZy/dJsiAG9/tMW5qn7DLkX0aYQcvKTzn/3z32tpyWUBxe7wVZ8X gWKkztSp/wvNGyoqLn9EIiMLBPweZzpKq6PIhmfKFsvxBPuPFhNUcpQ3LKAxmEIO 248lG7GNqDmy/e0uD7IY0B1jEdIkFdv3k9CcjZkSRyfLa+8+hHvy1ZdWOEnAoV5W LIJPsdWgZq5A0cCsgRmTFGzOmubgnXOI8yRikpJpSI/hUHIHysAat4dGwCJml28M 7/B1CHZ0IzS9HNWslA5pe8d3xpAT+5n+EF4wFIfYb/wpH/7dnW34xWaaiP8J9s1I CiWg2V99pUzhVBBjzqmj3EM9K7RQAyf6OUdMDvy+RLLASeOmvMNlcdLdt7IWcW7l 6FTBN5vPlhyvXfiDvX//5Rnhd0j1ZKuGZmX8OD80IbDRK6R5tky/bfx9ZD0U087V QBaFNSAyWK9kl8xRo7f1ilCmBDeMM4e/r4Dlq7HBOWhnJR+USfXRpv6WsBec46he bjzxGWmBnryDFL9xU+3pELy5rklwTQY7BsoB0/bpV17vB59Nbr0= =aJ/Z -----END PGP SIGNATURE----- --Apple-Mail=_F9A75F62-F1BB-43C2-8C19-EBC79BAE7801--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?C69534AE-7470-4A4D-AEDF-41B6E64DD214>