Re: [PATCH 1/4] fs/iso9660: Add check to prevent infinite loop

[Lidong Chen]

> On Dec 15, 2022, at 9:52 AM, Thomas Schmitt <scdbackup@gmx.net> wrote:
> 
> Hi,
> 
> On Wed, 14 Dec 2022 18:55:02 +0000 Lidong Chen <lidong.chen@oracle.com> 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 <lidong.chen@oracle.com>
>> ---
>> 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 <scdbackup@gmx.net>
> 
> 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
>