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

[Lidong Chen]

> On Dec 14, 2022, at 1:42 PM, Thomas Schmitt <scdbackup@gmx.net> 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
>