Re: [PATCH v2 0/5] fs/iso9660: Fix out-of-bounds read

[Lidong Chen]

Hi,

> On Jan 18, 2023, at 8:31 AM, Thomas Schmitt <scdbackup@gmx.net> wrote:
> 
> Hi,
> 
> On Wed, 18 Jan 2023 08:23:53 +0000 Lidong Chen <lidong.chen@oracle.com> wrote:
>> Thomas also pointed out the issue of the potential endless
>> loops by CE. Since the sugguested fix requires a bit more
>> investigation, and as Thomas pointed out that it should be
>> handled in a separate patch, the fix is not included in this
>> this v2 patches set. Because I am not an expert, it would
>> be better that someone else can work on it. For the background
>> info and the comments, please see this email. The bottom half
>> of the email addressed the endless loop issue:
>> 
>> https://urldefense.com/v3/__https://www.mail-archive.com/grub-devel@gnu.org/msg35785.html__;!!ACWV5N9M2RV99hQ!PnDkQnGTDLOfxl9IryXHlAoUSWaeNHsXVtkVb_R8FghhG18JFoFQGCllHBnL9fpX3MJUFMN2vjoj6pcyPqE$
>>  
> 
> I had a look at Linux fs/isofs/rock.c about the handling of CE loops.
> 
> It works with a hop counter like my current untested proposal for GRUB,
> which i made in above mail.
> But Linux is very restrictive:
> 
>  /* Maximum number of Rock Ridge continuation entries */
>  #define RR_MAX_CE_ENTRIES 32
>  ...
>                ret = -EIO;
>                if (++rs->cont_loops >= RR_MAX_CE_ENTRIES)
>                        goto out;
> 
> So probably my proposed limit of a million is just oversized. But i think
> that 32 would be too low.
> 
> The Linux limit is bad news for people who want to put larger data blobs
> into SUSP controlled blocks, because the size of a continuation area is
> also restricted by Linux to a single block of 2048 bytes.
> The EIO in case of too much CE entries does not show up in the system log
> or on the terminal. Just the file is not listed and not accessible:
> 
>  $ wc /mnt/iso/x
>  wc: /mnt/iso/x: No such file or directory
> 
> I understand from source code and experiments that the actual maximum
> number of CE hops in Linux is 31 rather than 32.
> 
> libisofs and xorriso are in the process of getting an adjustable curb to
> prevent the production of ISO filesystems with files which would not show
> up in Linux. I decided for 100,000 hops as hard limit but set the default
> to 31.
I am not sure I understand the hard limit vs the default in terms of checking 
the number of hops.
Will checking against the hard limit be enough?  Since the limit of CE hops has 
been decided,
if the previous proposed fix still stands, I can create a patch for it. The 
tough part for me is the
testing. 

Regards,
Lidong
 

> (SUSP prescribes that entries of unknown type be "ignored and skipped",
> which Linux normally does fine. Here it could do better.)
> 
> 
> Have a nice day :)
> 
> Thomas
>