Re: [PATCH 0/4] fs/iso9660: Fix out-of-bounds read
> On Dec 14, 2022, at 1:42 PM, Thomas Schmitt wrote:
>
> Hi,
>
> i will review the patches hopefully tomorrow.
>
> But in [PATCH 2/4] and [4/4] i stumble over the statement that a SUSP
> entry has 5 bytes of size. This is not true. The minimum size is 4.
> In SUSP there are "Padding" PD (if its byte LEN_PD is 0) and "Stop" ST,
> which have 4 bytes. In RRIP there is "Relocated" RE.
> Other SUSP-compliant protocols could define 4-byte entries, too.
Thank you for the clarification about the SUSP entry size. I was confused with
’NM’ entry.
I will fix it.
>
> I will have to analyze the patches whether the assumption of 5 bytes
> minimum can cause real harm.
>
> But i see at least two inappropriate applications of the minimum size:
>
> In [2/4] a RRIP NM entry is processed:
> - csize = entry->len - 5;
> + csize = entry->len - GRUB_ISO9660_SUSP_HEADER_SZ;
> The number 5 is meant to skip over the 4 bytes of SUSP header and the one
> byte of "FLAGS" to reach to the "NAME CONTENT" bytes. This is specific to
> NM (version 1, to be exacting), not to SUSP in general.
> I propose to leave the 5 as is.
I will revert this change and restore the plain 5 as it is.
Thanks,
Lidong
>
> In [4/4] a RRIP SL entry is processed:
> - /* 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)
> At least in a quick test in GNU/Linux userspace
> struct grub_iso9660_susp_entry {
>uint8_t sig[2];
>uint8_t len;
>uint8_t version;
>uint8_t data[0];
> };
> has sizeof 4, not 5.
> So i see the risk that this change alters program behavior in situations
> where we don't perceive a problem yet.
>
> It is too late in the evening to analyze what sizeof(*entry) has to do
> with reading the component records of SL. The component records are the
> components of the link target path with 2 bytes header plus the name
> bytes. So a size of 3 is plausible like in .../b/... Even a size of 2
> is not totally illegal, as Linux allows paths like ...//
>
>
> Have a nice day :)
>
> Thomas
>
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 1/4] fs/iso9660: Add check to prevent infinite loop
> On Dec 15, 2022, at 9:52 AM, Thomas Schmitt wrote:
>
> Hi,
>
> On Wed, 14 Dec 2022 18:55:02 + Lidong Chen wrote:
>> There is no check for the end of block When reading
>
> s/When/when/
>
>> directory extents. It resulted in read_node() always
>> read from the same offset in the while loop, thus
>> caused infinite loop. The fix added a check for the
>> end of the block and ensure the read is within directory
>> boundary.
>>
>> Signed-off-by: Lidong Chen
>> ---
>> grub-core/fs/iso9660.c | 21 +
>> 1 file changed, 21 insertions(+)
>>
>> diff --git a/grub-core/fs/iso9660.c b/grub-core/fs/iso9660.c
>> index 91817ec1f..4f4cd6165 100644
>> --- a/grub-core/fs/iso9660.c
>> +++ b/grub-core/fs/iso9660.c
>> @@ -795,6 +795,15 @@ grub_iso9660_iterate_dir (grub_fshelp_node_t dir,
>> while (dirent.flags & FLAG_MORE_EXTENTS)
>>{
>> offset += dirent.len;
>> +
>> +/* offset should within the dir's len. */
>> +if (offset > len)
>> + {
>> +if (ctx.filename_alloc)
>> + grub_free (ctx.filename);
>> +return 0;
>> + }
>> +
>> if (read_node (dir, offset, sizeof (dirent), (char *) &dirent))
>>{
>> if (ctx.filename_alloc)
>> @@ -802,6 +811,18 @@ grub_iso9660_iterate_dir (grub_fshelp_node_t dir,
>> grub_free (node);
>> return 0;
>>}
>> +
>> +/*
>> + * It is either the end of block or zero-padded sector,
>> + * skip to the next block.
>> + */
>> +if (!dirent.len)
>> + {
>> +offset = (offset / GRUB_ISO9660_BLKSZ + 1) * GRUB_ISO9660_BLKSZ;
>> +dirent.flags |= FLAG_MORE_EXTENTS;
>> +continue;
>> + }
>> +
>> if (node->have_dirents >= node->alloc_dirents)
>>{
>> struct grub_fshelp_node *new_node;
>> --
>> 2.35.1
>>
>
> Reviewed-by: Thomas Schmitt
>
> The second hunk will become very necessary when more initrds >= 4 GiB will
> be around. Then GRUB might more probably encounter directory records of a
> large file which are not stored in the same block.
>
> (Are we aware of the file size limit of 32 GiB - 14 KiB - 1 imposed by
> struct grub_fshelp_node { ... struct grub_iso9660_dir dirents[8]; ... }
> ? )
>
I am not familiar with this file size limit. Do we need to add a check
somewhere?
Thanks,
Lidong
>
> Have a nice day :)
>
> Thomas
>
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 2/4] fs/iso9660: Prevent read past the end of system use area
> On Dec 15, 2022, at 10:00 AM, Thomas Schmitt wrote:
>
> Hi,
>
> On Wed, 14 Dec 2022 18:55:03 + Lidong Chen wrote:
>> In the code, the for loop advanced the entry pointer to the
>> next entry before checking if the next entry is within the
>> system use area boundary. Another issue in the code was that
>> there is no check for the size of system use area. For a
>> corrupted system, the size of system use area can be less than
>> the size of SUSP entry size (5 bytes).
>
> The minimum size of a SUSP entry is 4, not 5. Examples of 4-byte entries
> are ST in SUSP and RE in RRIP. (Ending before ST would be harmless, because
> ST marks the end of the SUSP entry chain. But RE may appear before the end.)
>
I will fix it.
>
>> These can cause buffer
>> overrun. The fixes added the checks to ensure the read is
>> valid and within the boundary.
>>
>> Signed-off-by: Lidong Chen
>> ---
>> grub-core/fs/iso9660.c | 31 +++
>> 1 file changed, 27 insertions(+), 4 deletions(-)
>>
>> diff --git a/grub-core/fs/iso9660.c b/grub-core/fs/iso9660.c
>> index 4f4cd6165..9170fa820 100644
>> --- a/grub-core/fs/iso9660.c
>> +++ b/grub-core/fs/iso9660.c
>> @@ -49,6 +49,8 @@ GRUB_MOD_LICENSE ("GPLv3+");
>> #define GRUB_ISO9660_VOLDESC_PART3
>> #define GRUB_ISO9660_VOLDESC_END 255
>>
>> +#define GRUB_ISO9660_SUSP_HEADER_SZ 5
>
> So i think this should be 4, not 5.
> If we find reason why it should be 5, then the name SUSP_HEADER_SZ is not
> appropriate. The SUSP header size is surely 4.
You are right. SUSP-1.12 stated:
“If the remaining allocated space following the last recorded System Use
Entry in a System Use field or Continuation Area is less than 4 bytes long,
It cannot contain a System Use Entry and should be ignored”
It implied the minimum SUSP Entry size is 4. I will fix it.
>
>
>> +
>> /* The head of a volume descriptor. */
>> struct grub_iso9660_voldesc
>> {
>> @@ -272,6 +274,9 @@ grub_iso9660_susp_iterate (grub_fshelp_node_t node,
>> grub_off_t off,
>> if (sua_size <= 0)
>> return GRUB_ERR_NONE;
>>
>> + if (sua_size < GRUB_ISO9660_SUSP_HEADER_SZ)
>> +return grub_error (GRUB_ERR_BAD_FS, "invalid susp entry size");
>> +
>
> Here we need 4.
>
>
>> sua = grub_malloc (sua_size);
>> if (!sua)
>> return grub_errno;
>> @@ -281,10 +286,14 @@ grub_iso9660_susp_iterate (grub_fshelp_node_t node,
>> grub_off_t off,
>> if (err)
>> return err;
>>
>> - for (entry = (struct grub_iso9660_susp_entry *) sua; (char *) entry <
>> (char *) sua + sua_size - 1 && entry->len > 0;
>> - entry = (struct grub_iso9660_susp_entry *)
>> - ((char *) entry + entry->len))
>> + entry = (struct grub_iso9660_susp_entry *) sua;
>> +
>> + while (entry->len > 0)
>> {
>> + /* Ensure the entry is within System Use Area */
>> + if ((char *) entry + entry->len > (sua + sua_size))
>> +break;
>> +
>> /* The last entry. */
>> if (grub_strncmp ((char *) entry->sig, "ST", 2) == 0)
>> break;
>> @@ -300,6 +309,15 @@ grub_iso9660_susp_iterate (grub_fshelp_node_t node,
>> grub_off_t off,
>>off = grub_le_to_cpu32 (ce->off);
>>ce_block = grub_le_to_cpu32 (ce->blk) << GRUB_ISO9660_LOG2_BLKSZ;
>>
>> + if (sua_size <= 0)
>> +break;
>> +
>> + if (sua_size < GRUB_ISO9660_SUSP_HEADER_SZ)
>
> 4 would be appropriate here.
>
>
>> +{
>> + grub_free (sua);
>> + return grub_error (GRUB_ERR_BAD_FS, "invalid CE entry size");
>> +}
>> +
>>grub_free (sua);
>>sua = grub_malloc (sua_size);
>>if (!sua)
>> @@ -319,6 +337,11 @@ grub_iso9660_susp_iterate (grub_fshelp_node_t node,
>> grub_off_t off,
>>grub_free (sua);
>>return 0;
>> }
>> +
>> + entry = (struct grub_iso9660_susp_entry *) ((char *) entry +
>> entry->len);
>
> This is equivalent to the third statement in the "for" which was
> replaced by a while-loop. So far so good.
>
> But i believe to see an old bug. This advancing of entry breaks the
> chain of CE if the first entry of the Continuation Area is again a CE.
>
> err = grub_disk_read (node->data->disk, ce_block, off,
>sua_size, sua);
> ...
> entry = (struct grub_iso9660_susp_entry *) sua;
>}
>
> if (hook (entry, hook_arg))
>{
> grub_free (sua);
> return 0;
>}
>
> entry = (struct grub_iso9660_susp_entry *) ((char *) entry + entry->len);
>
> hook() will not interpret CE (and has no means to advance "entry").
> entry = entry + entry->len; will then skip over the entry so that the loop
> will not interpret it either.
> The SUSP area will be perceived as having ended, although more SUSP entries
> are present for the file in question.
>
> I hereby ask for more opinions about this. Maybe i misinterpret the old loop.
>
> Background:
> CE points to the block and offset where the chain of SUSP entries goes on.
>
Re: [PATCH 3/4] fs/iso9660: Avoid reading past the entry boundary
> On Dec 15, 2022, at 10:08 AM, Thomas Schmitt wrote:
>
> Hi,
>
> On Wed, 14 Dec 2022 18:55:04 + Lidong Chen wrote:
>> Added a check for the SP entry data boundary before reading it.
>>
>> Signed-off-by: Lidong Chen
>> ---
>> grub-core/fs/iso9660.c | 16 ++--
>> 1 file changed, 14 insertions(+), 2 deletions(-)
>>
>> diff --git a/grub-core/fs/iso9660.c b/grub-core/fs/iso9660.c
>> index 9170fa820..67aa8451c 100644
>> --- a/grub-core/fs/iso9660.c
>> +++ b/grub-core/fs/iso9660.c
>> @@ -408,6 +408,9 @@ set_rockridge (struct grub_iso9660_data *data)
>> if (!sua_size)
>> return GRUB_ERR_NONE;
>>
>> + if (sua_size < GRUB_ISO9660_SUSP_HEADER_SZ)
>
> As mentioned in my review of patch [2/4], GRUB_ISO9660_SUSP_HEADER_SZ should
> be defined as 4, rather than as 5. Else entry RE could trigger this error:
>
>> +return grub_error (GRUB_ERR_BAD_FS, "invalid rock ridge entry size");
>> +
>> sua = grub_malloc (sua_size);
>> if (! sua)
>> return grub_errno;
>> @@ -434,8 +437,17 @@ set_rockridge (struct grub_iso9660_data *data)
>> rootnode.have_symlink = 0;
>> rootnode.dirents[0] = data->voldesc.rootdir;
>>
>> - /* The 2nd data byte stored how many bytes are skipped every time
>> - to get to the SUA (System Usage Area). */
>> + /*
>> + * The 2nd data byte stored how many bytes are skipped every time
>> + * to get to the SUA (System Usage Area).
>> + */
>> + if (sua_size < GRUB_ISO9660_SUSP_HEADER_SZ + 2 ||
>> + entry->len < GRUB_ISO9660_SUSP_HEADER_SZ + 2)
>
> This interprets an SP entry, which is specified to have 7 bytes.
> So if GRUB_ISO9660_SUSP_HEADER_SZ gets corrected to 4, then the size demand
> will have to be (GRUB_ISO9660_SUSP_HEADER_SZ + 3).
>
> Like with the NM interpretation i would rather prefer a plain "7", maybe with
> a comment which says that this is the fixed size of SP (version 1).
>
>
>> +{
>> + grub_free (sua);
>> + return grub_error (GRUB_ERR_BAD_FS, "corrupted rock ridge entry");
>> +}
>> +
>> data->susp_skip = entry->data[2];
>> entry = (struct grub_iso9660_susp_entry *) ((char *) entry +
>> entry->len);
>>
>> --
>> 2.35.1
>>
>
> Reviewed-by: Thomas Schmitt
>
> But the expression (GRUB_ISO9660_SUSP_HEADER_SZ + 2) will need correction if
> my wish for
> #define GRUB_ISO9660_SUSP_HEADER_SZ 4
> gets fulfilled. As said, i'd prefer a plain "7".
>
Ok, I will change it to a plain ‘7’ and add a comment for it.
Thanks,
Lidong
>
> Have a nice day :)
>
> Thomas
>
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 1/4] fs/iso9660: Add check to prevent infinite loop
Hi,
i wrote:
> > (Are we aware of the file size limit of 32 GiB - 14 KiB - 1 imposed by
> > struct grub_fshelp_node { ... struct grub_iso9660_dir dirents[8]; ... }
> > ? )
Lidong Chen wrote:
> I am not familiar with this file size limit. Do we need to add a check
> somewhere?
Good question. The answer probably disproves my statement because the
struct definition seems not to match exactly its usage:
Assessment happens in grub_iso9660_iterate_dir():
while (dirent.flags & FLAG_MORE_EXTENTS)
{
...
if (node->have_dirents >= node->alloc_dirents)
{
At this point an overflow of currently allocated .dirents[] was detected.
struct grub_fshelp_node *new_node;
grub_size_t sz;
if (grub_mul (node->alloc_dirents, 2, &node->alloc_dirents) ||
grub_sub (node->alloc_dirents, ARRAY_SIZE (node->dirents),
&sz) ||
grub_mul (sz, sizeof (node->dirents[0]), &sz) ||
grub_add (sz, sizeof (struct grub_fshelp_node), &sz))
goto fail_0;
new_node = grub_realloc (node, sz);
I understand the computations in the if-clause as:
- The number of allocated dirents is doubled.
- The new_node size is the size of the new number of .dirents minus 8
.dirent sizes for the eight .dirents which are part of the
grub_fshelp_node definition,
- plus the defined size of the grub_fshelp_node.
The new_node gets allocated with that size, which provides enough space
for the new dirent and many of its potential successors.
So i retract my statement. Data file size seems quite unlimited.
At some point grub_mul() or grub_realloc() will throw an error if the number
of .dirents is too high for grub_size_t or the machine's memory.
Have a nice day :)
Thomas
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
[RFC PATCH] kern/dl: Add module version check
This patch add version information to GRUB modules. Specifically,
PACKAGE_VERSION is embedded as null-terminated string in .modver
section. This string is checked at module loading time. That module will
be rejected if mismatch is found. This will prevent loading modules from
different versions of GRUB.
It should be pointed out that the version string is only consisted of
PACKAGE_VERSION. Any difference not reflected in PACKAGE_VERSION is not
noticed by version check (e.g. configure options).
Signed-off-by: Zhang Boyang
---
grub-core/Makefile.am | 2 +-
grub-core/genmod.sh.in| 28 +---
grub-core/kern/dl.c | 18 ++
util/grub-module-verifierXX.c | 10 ++
4 files changed, 46 insertions(+), 12 deletions(-)
diff --git a/grub-core/Makefile.am b/grub-core/Makefile.am
index 80e7a83ed..d76aa80f4 100644
--- a/grub-core/Makefile.am
+++ b/grub-core/Makefile.am
@@ -40,7 +40,7 @@ gentrigtables$(BUILD_EXEEXT): gentrigtables.c
CLEANFILES += gentrigtables$(BUILD_EXEEXT)
build-grub-module-verifier$(BUILD_EXEEXT):
$(top_srcdir)/util/grub-module-verifier.c
$(top_srcdir)/util/grub-module-verifier32.c
$(top_srcdir)/util/grub-module-verifier64.c
$(top_srcdir)/grub-core/kern/emu/misc.c $(top_srcdir)/util/misc.c
- $(BUILD_CC) -o $@ -I$(top_srcdir)/include $(BUILD_CFLAGS)
$(BUILD_CPPFLAGS) $(BUILD_LDFLAGS) -DGRUB_BUILD=1 -DGRUB_UTIL=1
-DGRUB_BUILD_PROGRAM_NAME=\"build-grub-module-verifier\" $^
+ $(BUILD_CC) -o $@ -I$(top_srcdir)/include $(BUILD_CFLAGS)
$(BUILD_CPPFLAGS) $(BUILD_LDFLAGS) -DGRUB_BUILD=1 -DGRUB_UTIL=1
-DGRUB_BUILD_PROGRAM_NAME=\"build-grub-module-verifier\"
-DPACKAGE_VERSION=\"$(PACKAGE_VERSION)\" $^
CLEANFILES += build-grub-module-verifier$(BUILD_EXEEXT)
# trigtables.c
diff --git a/grub-core/genmod.sh.in b/grub-core/genmod.sh.in
index e57c4d920..58a4cdcbe 100644
--- a/grub-core/genmod.sh.in
+++ b/grub-core/genmod.sh.in
@@ -36,22 +36,25 @@ deps=`grep ^$modname: $moddep | sed s@^.*:@@`
rm -f $tmpfile $outfile
if test x@TARGET_APPLE_LINKER@ != x1; then
-# stripout .modname and .moddeps sections from input module
-@TARGET_OBJCOPY@ -R .modname -R .moddeps $infile $tmpfile
+# stripout .modname and .moddeps and .modver sections from input module
+@TARGET_OBJCOPY@ -R .modname -R .moddeps -R .modver $infile $tmpfile
-# Attach .modname and .moddeps sections
+# Attach .modname and .moddeps and .modver sections
t1=`mktemp "${TMPDIR:-/tmp}/tmp.XX"` || exit 1
printf "$modname\0" >$t1
t2=`mktemp "${TMPDIR:-/tmp}/tmp.XX"` || exit 1
for dep in $deps; do printf "$dep\0" >> $t2; done
+t3=`mktemp "${TMPDIR:-/tmp}/tmp.XX"` || exit 1
+printf "@PACKAGE_VERSION@\0" >$t3
+
if test -n "$deps"; then
- @TARGET_OBJCOPY@ --add-section .modname=$t1 --add-section .moddeps=$t2
$tmpfile
+ @TARGET_OBJCOPY@ --add-section .modname=$t1 --add-section .moddeps=$t2
--add-section .modver=$t3 $tmpfile
else
- @TARGET_OBJCOPY@ --add-section .modname=$t1 $tmpfile
+ @TARGET_OBJCOPY@ --add-section .modname=$t1 --add-section .modver=$t3
$tmpfile
fi
-rm -f $t1 $t2
+rm -f $t1 $t2 $t3
if test x@platform@ != xemu; then
@TARGET_STRIP@ --strip-unneeded \
@@ -71,23 +74,26 @@ else
tmpfile2=${outfile}.tmp2
t1=${outfile}.t1.c
t2=${outfile}.t2.c
+t3=${outfile}.t3.c
# remove old files if any
-rm -f $t1 $t2
+rm -f $t1 $t2 $t3
cp $infile $tmpfile
-# Attach .modname and .moddeps sections
+# Attach .modname and .moddeps and .modver sections
echo "char modname[] __attribute__ ((section(\"_modname, _modname\"))) =
\"$modname\";" >$t1
for dep in $deps; do echo "char moddep_$dep[] __attribute__
((section(\"_moddeps, _moddeps\"))) = \"$dep\";" >>$t2; done
+echo "char modver[] __attribute__ ((section(\"_modver, _modver\"))) =
\"@PACKAGE_VERSION@\";" >$t3
+
if test -n "$deps"; then
- @TARGET_CC@ @TARGET_LDFLAGS@ -ffreestanding -nostdlib -o $tmpfile2 $t1
$t2 $tmpfile -Wl,-r
+ @TARGET_CC@ @TARGET_LDFLAGS@ -ffreestanding -nostdlib -o $tmpfile2 $t1
$t2 $t3 $tmpfile -Wl,-r
else
- @TARGET_CC@ @TARGET_LDFLAGS@ -ffreestanding -nostdlib -o $tmpfile2 $t1
$tmpfile -Wl,-r
+ @TARGET_CC@ @TARGET_LDFLAGS@ -ffreestanding -nostdlib -o $tmpfile2 $t1
$t3 $tmpfile -Wl,-r
fi
-rm -f $t1 $t2 $tmpfile
+rm -f $t1 $t2 $t3 $tmpfile
mv $tmpfile2 $tmpfile
cp $tmpfile $tmpfile.bin
diff --git a/grub-core/kern/dl.c b/grub-core/kern/dl.c
index e447fd0fa..5fbcfc147 100644
--- a/grub-core/kern/dl.c
+++ b/grub-core/kern/dl.c
@@ -475,6 +475,23 @@ grub_dl_check_license (grub_dl_t mod, Elf_Ehdr *e)
(char *) e + s->sh_offset);
}
+static grub_err_t
+grub_dl_check_version (grub_dl_t mod, Elf_Ehdr *e)
+{
+ Elf_Shdr *s = grub_dl_find_section (e, ".modver");
+
+ if (s == NULL)
+return grub_error (GRUB_E
Re: [PATCH 4/4] fs/iso9660: Incorrect check for entry boudary
> On Dec 15, 2022, at 10:20 AM, Thomas Schmitt wrote:
>
> Hi,
>
> On Wed, 14 Dec 2022 18:55:05 + Lidong Chen wrote:
>> [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) == 0)
>> {
>> unsigned int pos = 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)
My understanding is the entry->len is the size of the System Use Field. If that
is
correct, the while-loop condition is to make sure the component records are
within the System Use Field boundary. “Pos” was initially set to ‘1’ and
incremented
by the size of each component record. So, “Pos” is relative to the Component
Area,
not the System Use Field of “SL”. I think the fix should include the 5 bytes:
While (pos + 5 < 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.
My mistake. I will restore the original code.
>
> 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 path
> components.)
>
> I am undecided whether add_part() should be skipped if
> (pos + 2 == entry->len)
Do you mean this within the while-loop?:
If (pos + 2 == entry->len)
continue
> 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 == 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 == 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 component
> record, of which the length is entry->data[pos + 1] + 2.
>
>
>> + csize = entry->data[pos + 1] + 2;
>> + if (csize + GRUB_ISO9660_SUSP_HEADER_SZ > entry->len)
>
> With GRUB_ISO9660_SUSP_HEADER_SZ == 4 it will be
>
> if (csize + GRUB_ISO9660_SUSP_HEADER_SZ + 1 > entry->len)
I will add plain ‘5’ and add the comment for it.
>
>
>> +break;
>> +
>>/* The current position is the `Component Flag'. */
>>switch (entry->data[pos] & 30)
>> {
>> @@ -681,8 +693,11 @@ susp_iterate_dir (struct grub_iso9660_susp
