From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.90_1) id 1p5sqh-0001Tj-Kj for mharc-grub-devel@gnu.org; Thu, 15 Dec 2022 13:20:54 -0500 Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1p5sqY-0001Ow-MV for grub-devel@gnu.org; Thu, 15 Dec 2022 13:20:48 -0500 Received: from mout.gmx.net ([212.227.17.22]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1p5sqO-0008Lf-HL for grub-devel@gnu.org; Thu, 15 Dec 2022 13:20:34 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=gmx.net; s=s31663417; t=1671128407; bh=v82JA0xV+KVN+9NJ32jdUgoBAbgb8GX5uKxV/p9Qocs=; h=X-UI-Sender-Class:Date:From:To:Subject:Cc:References:In-Reply-To; b=bGK/eu5P4AV/A8K8SyA5YcnAZZ7BALUPPtoyeU1BrQ3SWSpKouVaf9cyzwvYR3iem IAauRgt6iiLKwfeFGqlYFAeMWX8tc+VsTikzDIGGEBLWsEADDkqEzrAKXLVnMtxs8n LDUpsedR/ifk7P7M4UbKhfRV10y4wu0iH4OXp11/KuCVbUiFWR7W5ofvWrhQcxp9uv J44aFyp/vYw9wNV9AWQ7IecpIn5iCAhcr8SOmxqLqFRHRL9bkBfUkYiktB565leLcz /obC+sMI4PBFVUsdzt9ExzaqR3siqz4PsqBTafIL4iUQWLjGjP4O6fSmRi5LB/Z0oL 5cSnaFIwyy9DA== X-UI-Sender-Class: 724b4f7f-cbec-4199-ad4e-598c01a50d3a Received: from scdbackup.webframe.org ([84.179.236.73]) by mail.gmx.net (mrgmx105 [212.227.17.168]) with ESMTPSA (Nemesis) id 1M9Fjb-1p1lxo0xoF-006M5N; Thu, 15 Dec 2022 19:20:07 +0100 Date: Thu, 15 Dec 2022 19:20:49 +0100 From: "Thomas Schmitt" To: grub-devel@gnu.org Subject: Re: [PATCH 4/4] fs/iso9660: Incorrect check for entry boudary Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable Cc: lidong.chen@oracle.com, fengtao40@huawei.com, yanan@huawei.com, daniel.kiper@oracle.com, lichenca2005@gmail.com References: In-Reply-To: Message-Id: <13890389959357580538@scdbackup.webframe.org> X-Provags-ID: V03:K1:0d45sU2OpwmytWGKmHnmy43NZ6W5PTRozN+pPirmPxaxZ3ashVA LmIIg7Vo+0EybD4xTIzUrnwUlNiWyfv1OP51TosQiltOTGgWDtgyTnHMNAY7Exf6N1PmB9Y Yscg8vm7v020wgEJ6d7PBQG+JwDyhYhmrlftisKyN8355DYymI04jztjcKGQJK9eAdcgxz6 YSlcODHYPoLawyOi9X/qg== UI-OutboundReport: notjunk:1;M01:P0:P20kwHzjk9k=;xbw7jMv4wUT8Xmh9h4l+Qp7FSgR qiP5kRN1Q7zGHrdPGFKq5SmY5Su9gOqrFf0GCktDai8tOCEThMyWWot/2DuIK0BTc0Nw35AzK PuNQOwmy8L8Z/nbNbYZ9S7Cdrk7AiEM1BOjFDxuybzLY0QsbjEM/xXJHA2vZb/vHcBoM415Q3 57IYiMMFRiXfydGhgzaxk/MRHfbIM6neXeRsni9GMsue/J6xAGKqI4winf05Ir9AahQbbJPj1 9XDZ1EAuPegHbjqRIE68Ht/xBYgq8KPqCskZz51R79F4RVzFDZj9tkDt6cOAHt9xZZzUcY55o jEUDn1b3XeYGv9F+M0irbOtZ6lJ6tLBv9leOgB5kpp5SxUODaKb7jax7vkbRoSD2QWj+/B+q3 7rbEOjZmlM+Us5fwJlNZVYSB5Gl0gexYD+QwMqXutUcGtWKu+q13fenCQ4WZdSl9kQGSJD/5u pyTFK1TrILi68DiZLmnLuwAB8b+MH8mlZvHjtnDv4R57HczAIXo4lcxY80ZgBjDa9UxhVO16q nA4ts//lbVDOGryxZ58svEM15aqGZhTk5gqw89i66ZLwiYEgbN60pf36+wvfFtI57HtrHo7iv 80r2A3hf6K5tfwfiK+oJcsIDMHgAETvN7r5Gqy5liMidUoMG5MhCNOpmDtY++1c+786f+qJh9 uLIa65i+yjTIjaxTSj7bTZ3gGAhYbheuw6sEwhq25ZGzHfTv5hw3k1H5uLPQhEo5XVvQUO7XB 9L0Bn8nqonMlKOXoGjjmuJzZRnF+UW6VtVC46/bXfVIDzVNMo47Y7ID4p3TRxzd8+w2iu3Suj hHUXVqLVwAENBPnt33hpbYzNut25SD+Eca8LROunf5LTfbl6rTIBDPAfjstcGehpXtU7XIzEN RfApGKwwBr9JVVEPT074jHuJGtQElUUh8FQD9VtpnyMAEBtpBIOKvhe0o25NN69jdibUIdlF5 Ect/jKjSdz8ZmUeFQfWWo+rQUpM= Received-SPF: pass client-ip=212.227.17.22; envelope-from=scdbackup@gmx.net; helo=mout.gmx.net X-Spam_score_int: -27 X-Spam_score: -2.8 X-Spam_bar: -- X-Spam_report: (-2.8 / 5.0 requ) BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, FREEMAIL_FROM=0.001, RCVD_IN_DNSWL_LOW=-0.7, RCVD_IN_MSPIKE_H2=-0.001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: grub-devel@gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: The development of GNU GRUB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 15 Dec 2022 18:20:48 -0000 Hi, On Wed, 14 Dec 2022 18:55:05 +0000 Lidong Chen wr= ote: > [PATCH 4/4] fs/iso9660: Incorrect check for entry boudary s/boudary/boundary/ > An entry consists of the entry info and the component area. Component area is specific to the RRIP SL entry. So: s/An entry/An SL entry/ > The entry info should take up 5 bytes instead of sizeof (*entry). > The area after the first 5 bytes is the component area. The code > uses the sizeof (*entry) to check the boundary which is incorrect. > Also, an entry may not have component record. Added a check for > for the component length before reading the component record. > > Signed-off-by: Lidong Chen > --- > grub-core/fs/iso9660.c | 23 +++++++++++++++++++---- > 1 file changed, 19 insertions(+), 4 deletions(-) > > diff --git a/grub-core/fs/iso9660.c b/grub-core/fs/iso9660.c > index 67aa8451c..af432ee82 100644 > --- a/grub-core/fs/iso9660.c > +++ b/grub-core/fs/iso9660.c > @@ -662,10 +662,22 @@ susp_iterate_dir (struct grub_iso9660_susp_entry *= entry, > else if (grub_strncmp ("SL", (char *) entry->sig, 2) =3D=3D 0) > { > unsigned int pos =3D 1; > + unsigned int csize; > > - /* The symlink is not stored as a POSIX symlink, translate it. *= / > - while (pos + sizeof (*entry) < entry->len) > + /* The symlink is not stored as a POSIX symlink, translate it. */ > + while ((pos + GRUB_ISO9660_SUSP_HEADER_SZ) < entry->len) This loop is iterating over component records, not SUSP entries. Minimum size of a component record is 2. So i think the appropriate condition is: while (pos + 1 < entry->len) Component records have no struct representation in GRUB. So no sizeof will tell the correct value. C-wise decisive is this line: add_part (ctx, (char *) &entry->data[pos + 2], entry->data[pos + 1]); which lower in this patch changes to if (entry->data[pos + 1] > 0) { add_part (ctx, (char *) &entry->data[pos + 2], entry->data[pos + 1]); } This change will alter the program behavior in respect to empty link target path components. The validity of entry->data[pos + 1] is guaranteed by my test proposal. If entry->data[pos + 1] is 0, then &entry->data[pos + 2] can be an illegal address, but 0 bytes from that address will be requested to be added. (A 0-byte will be added by add_part() as separator between link target pat= h components.) I am undecided whether add_part() should be skipped if (pos + 2 =3D=3D entry->len) which indicates an empty path component. If add_part() shall be performed with length 0, then we should skip in it the call of grub_memcpy (ctx->symlink + size, part, len2); in case of (len2 =3D=3D 0). > { > + /* > + * entry->len is GRUB_ISO9660_SUSP_HEADER_SZ plus the > + * length of the 'Component Record'. The length of the With GRUB_ISO9660_SUSP_HEADER_SZ =3D=3D 4 it will be GRUB_ISO9660_SUSP_HEADER_SZ + 1 + length of the Component Area. The "+ 1" stands for the "FLAGS" byte. (RRIP-1.12, 4.1.3) It should be 'Component Area' or plural 'Component Records' because 'Component Record' is a single path component. > + * record is 2 (pos and pos + 1) plus the actual record > + * starting at pos + 2. pos stores the 'Component Flags', > + * pos + 1 specifies the length of actual record. > + */ This describes a single component record, of which there are as many as needed to represent the link target path. entry->data[pos + 1] gives the length of the component, not of the compone= nt record, of which the length is entry->data[pos + 1] + 2. > + csize =3D entry->data[pos + 1] + 2; > + if (csize + GRUB_ISO9660_SUSP_HEADER_SZ > entry->len) With GRUB_ISO9660_SUSP_HEADER_SZ =3D=3D 4 it will be if (csize + GRUB_ISO9660_SUSP_HEADER_SZ + 1 > entry->len) > + break; > + > /* The current position is the `Component Flag'. */ > switch (entry->data[pos] & 30) > { > @@ -681,8 +693,11 @@ susp_iterate_dir (struct grub_iso9660_susp_entry *e= ntry, > return grub_errno; > } > > - add_part (ctx, (char *) &entry->data[pos + 2], > - entry->data[pos + 1]); > + if (entry->data[pos + 1] > 0) > + { > + add_part (ctx, (char *) &entry->data[pos + 2], > + entry->data[pos + 1]); > + } As written above, this changes program behavior if a link target path contains an empty component. > ctx->was_continue =3D (entry->data[pos] & 1); > break; > } > -- > 2.35.1 > So no "Reviewed-by:" yet. Open issues: The commit message should mention that it is about SL and not about genera= l SUSP or RRIP entries. The while-condition of the component loop should neither use sizeof (*entr= y) nor GRUB_ISO9660_SUSP_HEADER_SZ, but plain 1, because that's the minimum size of an SL component record minus 1. Probably this should be a separate bug-fix patch to be applied before this patch [4/4]. Remaining mentioning of GRUB_ISO9660_SUSP_HEADER_SZ needs to become GRUB_ISO9660_SUSP_HEADER_SZ + 1 when/if GRUB_ISO9660_SUSP_HEADER_SZ gets defined to 4. The change of program behavior by ignoring empty link target path componen= ts should be mentioned in the commit message. =2D-----------------------------------------------------------------------= - ... whew. All four reviews done. Probably i made some mistakes in them. Please check all my statements before basing code changes on them. Inform me if you find such mistakes. Now i'm looking forward to the next round of review, decisions by Daniel Kiper, and comments from bystanders. Have a nice day :) Thomas