> On Dec 14, 2022, at 1:42 PM, Thomas Schmitt <scdbac...@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 > _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel