From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mout01.posteo.de (mout01.posteo.de [185.67.36.65]) by mx.groups.io with SMTP id smtpd.web10.36558.1675068494169208505 for ; Mon, 30 Jan 2023 00:48:14 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@posteo.de header.s=2017 header.b=HQzud+Qv; spf=pass (domain: posteo.de, ip: 185.67.36.65, mailfrom: mhaeuser@posteo.de) Received: from submission (posteo.de [185.67.36.169]) by mout01.posteo.de (Postfix) with ESMTPS id 874EB240139 for ; Mon, 30 Jan 2023 09:48:11 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=posteo.de; s=2017; t=1675068491; bh=hsWLSNsdou71hwmmKFbg8qx+dk02vV/O2fhbU7nXbC0=; h=From:Subject:Date:Cc:To:From; b=HQzud+QvX195eatQlh1eVebGFVPvhMkxfQBwySJGEI0VRbDUwJfw4ivftx+KuXnEf zWqorq3p93n8+EssYTAtMs+4OQNbd7I+ey86yv6WyGNT1L72Ky7XIVzfLsq0GqabUF iETNI2OtiTutqGarRlwfi+Vsy2nXpRGvkOE6sdBeMDVbTrf8t25ToKx5udMj75cVTW xia0SEQ8hwfLlvySacRa6hwPIUlw7Vy4TJkVrBViEnJyOSxmxfo3JPbB/+8mPL2Wfz GyyWCdPOLYJCSvpnQWW1h/GTYxmHoJt70S1mtRFcDEF/a0/3slsOK4O6hMvigqyftn 9TmoxwDaHujPg== Received: from customer (localhost [127.0.0.1]) by submission (posteo.de) with ESMTPSA id 4P51zZ2hGfz9rxM; Mon, 30 Jan 2023 09:48:10 +0100 (CET) From: =?UTF-8?B?TWFydmluIEjDpHVzZXI=?= Mime-Version: 1.0 (1.0) Subject: Re: [edk2-platforms][PATCH v3 11/11] Ext4Pkg: Filter out directory entry names containing \0 as invalid Date: Mon, 30 Jan 2023 08:48:09 +0000 Message-Id: <85F3AF0A-D39C-496F-BA9D-B8D7DA3D846A@posteo.de> References: Cc: Savva Mitrofanov , edk2-devel-groups-io , Vitaly Cheptsov In-Reply-To: To: Pedro Falcato Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable As Pedro agreed, forgot to add Reviewed-by: Marvin H=C3=A4user @Pedro feel free to change to UINT8 on push nevertheless. :) > On 27. Jan 2023, at 15:14, Marvin H=C3=A4user wrote: >=20 > =EF=BB=BF >=20 >> On 27. Jan 2023, at 15:09, Pedro Falcato wrote:= >>=20 >>> On Fri, Jan 27, 2023 at 10:04 AM Marvin H=C3=A4user = wrote: >>>=20 >>> On 27. Jan 2023, at 10:29, Savva Mitrofanov wrote: >>>>=20 >>>> The directory entry name conventions forbid having null-terminator >>>> symbols in its body and can lead to undefined behavior conditions >>>> and crashes >>>>=20 >>>> Cc: Marvin H=C3=A4user >>>> Cc: Pedro Falcato >>>> Cc: Vitaly Cheptsov >>>> Fixes: 89b2bb0db263 ("Ext4Pkg: Fix and clarify handling regarding non-u= tf8 dir entries") >>>> Signed-off-by: Savva Mitrofanov >>>> --- >>>> Features/Ext4Pkg/Ext4Dxe/Directory.c | 9 ++++++++- >>>> 1 file changed, 8 insertions(+), 1 deletion(-) >>>>=20 >>>> diff --git a/Features/Ext4Pkg/Ext4Dxe/Directory.c b/Features/Ext4Pkg/Ex= t4Dxe/Directory.c >>>> index 0753a20b5377..465749c9b51d 100644 >>>> --- a/Features/Ext4Pkg/Ext4Dxe/Directory.c >>>> +++ b/Features/Ext4Pkg/Ext4Dxe/Directory.c >>>> @@ -28,9 +28,16 @@ Ext4GetUcs2DirentName ( >>>> { >>>> CHAR8 Utf8NameBuf[EXT4_NAME_MAX + 1]; >>>> UINT16 *Str; >>>> + UINTN Index; >>>=20 >>> I *really* do not like UINTN in code that does not deal with buffer addr= esses and sizes. I'd change it to UINT8, but I'll leave it up to Pedro. >> Considering this is a size (length of name) and it gets explicitly >=20 > Well, being a "size" is not sufficient, I explicitly referred to buffer si= zes. Say, you get an arbitrarily long buffer from a caller, its size paramet= er should probably be UINTN, to be able to optimally leverage the platform m= emory and be flexible. This is not a buffer size, this is a bounded index of= an architecture-agnostic data structure. I simply do not want behaviour tha= t depends on the host architecture in architecture-agnostic code for no reas= on. We actually had multiple prior examples of bugs due to this, though this= is obviously fine (thus not demanding it's changed). >=20 >> bounded by name_len (a UINT8) I'm okay with it. But if for some reason >> you need to submit a v4, please change. >>=20 >> Reviewed-by: Pedro Falcato >>=20 >> --=20 >> Pedro >=20