usr.sbin/makefs/cd9660.c CID 976312: Explicit null dereferenced (FORWARD_NULL)i 4. var_deref_op: Dereferencing null pointer conversion_function. 1740 return (*conversion_function)(oldname, newname, is_file); --------------- Source analysis: The function pointer is non-NULL only if diskStructure.isoLevel is 1 or 2. A snippet in cd9660_parse_opts() indicates that the value can indeed be 3: option_t cd9660_options[] = { { "l", &diskStructure.isoLevel, 1, 3, "ISO Level" }, { "isolevel", &diskStructure.isoLevel, 1, 3, "ISO Level" }, ... The line 1740 is in function cd9660_convert_filename(). ISO 9660 level 3 allows the same file names as level 2. The use of ISO level 3 is not announced anywhere in the ISO image but rather becomes visible only if a file is large enough to need more than one extent. Extents can have 4 GiB - 1 byte of size. The offer of ISO level 3 is questionable because neither FreeBSD nor NetBSD can read files with multiple extents from ISO 9660. http://lists.freebsd.org/pipermail/freebsd-hackers/2012-April/038552.html Surely nobody has ever created a level 3 ISO with makefs. Not only would it SIGSEGV, but also the struct stat.st_size value is assigned to int64_t cd9660node.fileDataLength without any check in cd9660.c line 871: newnode->fileDataLength = node->inode->st.st_size; Later it is handed over to cd9660_bothendian_dword() which expects uint32_t (see cd9660/cd9660_conversion.c) cd9660.c line 834: cd9660_bothendian_dword(newnode->fileDataLength, newnode->isoDirRecord->size); So only one extent will be produced with a size as given by the low 4 bytes of the disk file size. --------------- Remedy proposal: Restrict isoLevel to 1 and 2. - { "l", &diskStructure.isoLevel, 1, 3, "ISO Level" }, - { "isolevel", &diskStructure.isoLevel, 1, 3, "ISO Level" }, + { "l", &diskStructure.isoLevel, 1, 2, "ISO Level" }, + { "isolevel", &diskStructure.isoLevel, 1, 2, "ISO Level" }, Restrict data file size to 4 GiB - 1 byte. /* Set the size */ - if (!(S_ISDIR(node->type))) + if (!(S_ISDIR(node->type))) { + if (node->inode->st.st_size > (off_t) 4096 * 1024 * 1024 - 1) { + + >>> error out in appropriate way <<< + + } newnode->fileDataLength = node->inode->st.st_size; + } To appease checkers use the same conversion function for all levels above 1: - else if (diskStructure.isoLevel == 2) + else conversion_function = &cd9660_level2_convert_filename;
Also present in NetBSD. http://cvsweb.netbsd.org/bsdweb.cgi/src/usr.sbin/makefs/cd9660.c?annotate=1.49 280: OPT_NUM('l', "isolevel", isoLevel, 281: 1, 3, "ISO Level"), 879: if (!(S_ISDIR(node->type))) 880: newnode->fileDataLength = node->inode->st.st_size; 1772: else if (diskStructure->isoLevel == 2) 1773: conversion_function = &cd9660_level2_convert_filename; 1774: return (*conversion_function)(diskStructure, oldname, newname, is_file);
A commit references this bug: Author: ngie Date: Mon Nov 2 08:43:15 UTC 2015 New revision: 290264 URL: https://svnweb.freebsd.org/changeset/base/290264 Log: Limit isoLevel to 1 and 2 to avoid segfaulting when isoLevel is set to 3 by dereferencing a NULL function pointer Add some asserts to ensure that isolevel is always either 1 or 2. PR: 203645 Reported by: Thomas Schmitt <scdbackup@gmx.net> MFC after: 1 week Sponsored by: EMC / Isilon Storage Division Changes: head/usr.sbin/makefs/cd9660.c
A commit references this bug: Author: ngie Date: Mon Nov 2 09:16:51 UTC 2015 New revision: 290265 URL: https://svnweb.freebsd.org/changeset/base/290265 Log: Add testcases for -t cd9660 -o isolevel=[1-3] -- -o isolevel=1 currently fails because of path comparison issues, so mark it as an expected failure. -- -o isolevel=3 is not implemented, so expect it to fail as an out of bounds value [*]. PR: 203645 MFC after: 1 week X-MFC with: r290264 Sponsored by: EMC / Isilon Storage Division Changes: head/usr.sbin/makefs/tests/makefs_cd9660_tests.sh
The patch does not care for oversized data files, which currently get misrepresented silently. Such a data file should rather be excluded or taken as reason for abort.
A commit references this bug: Author: ngie Date: Mon Nov 9 09:20:01 UTC 2015 New revision: 290594 URL: https://svnweb.freebsd.org/changeset/base/290594 Log: MFC r290265,r290267,r290270: r290265: Add testcases for -t cd9660 -o isolevel=[1-3] -- -o isolevel=1 currently fails because of path comparison issues, so mark it as an expected failure. -- -o isolevel=3 is not implemented, so expect it to fail as an out of bounds value [*]. PR: 203645 Sponsored by: EMC / Isilon Storage Division r290267: Clean up mtree keyword support a slight bit and add a few more default keywords - Parameterize the mtree keywords as $DEFAULT_MTREE_KEYWORDS - Test with the extra mtree keywords, `mode,gid,uid`. - Add a note about mtrees with time support not working with makefs right now Sponsored by: EMC / Isilon Storage Division r290270: Add testcases for -t ffs -o version=[12] Verify the filesystem type using dumpfs. Add preliminary support for NetBSD (needs to be validated) Sponsored by: EMC / Isilon Storage Division Changes: _U stable/10/ stable/10/usr.sbin/makefs/tests/makefs_cd9660_tests.sh stable/10/usr.sbin/makefs/tests/makefs_ffs_tests.sh stable/10/usr.sbin/makefs/tests/makefs_tests_common.sh
A commit references this bug: Author: ngie Date: Mon Nov 9 09:22:12 UTC 2015 New revision: 290595 URL: https://svnweb.freebsd.org/changeset/base/290595 Log: MFC r290264: Limit isoLevel to 1 and 2 to avoid segfaulting when isoLevel is set to 3 by dereferencing a NULL function pointer Add some asserts to ensure that isolevel is always either 1 or 2. PR: 203645 Reported by: Thomas Schmitt <scdbackup@gmx.net> Sponsored by: EMC / Isilon Storage Division Changes: _U stable/10/ stable/10/usr.sbin/makefs/cd9660.c
A commit references this bug: Author: ngie Date: Mon Nov 9 09:23:13 UTC 2015 New revision: 290596 URL: https://svnweb.freebsd.org/changeset/base/290596 Log: MFstable/10 r290595: MFC r290264: Limit isoLevel to 1 and 2 to avoid segfaulting when isoLevel is set to 3 by dereferencing a NULL function pointer Add some asserts to ensure that isolevel is always either 1 or 2. PR: 203645 Reported by: Thomas Schmitt <scdbackup@gmx.net> Sponsored by: EMC / Isilon Storage Division Changes: _U stable/9/ _U stable/9/usr.sbin/ _U stable/9/usr.sbin/makefs/ stable/9/usr.sbin/makefs/cd9660.c
(In reply to scdbackup from comment #4) > The patch does not care for oversized data files, > which currently get misrepresented silently. > Such a data file should rather be excluded or > taken as reason for abort. ISO-9660 level 1/2 both need to be fixed and ISO-9660 level 3 needs to be completely implemented. Feel free to post follow up bugs. I just wanted to make sure the segfault was corrected as implementing level 3 seemed like a larger task to perform for this bug.
I agree that it is most important to put a cap on that SIGSEGV. But implementing multi-extent files (which effectively constitute ISO 9660 level 3) would be a substantial change. Further it will not be enough to enhance makefs alone, because the readonly filesystem implementation in the FreeBSD kernel is unable to properly represent such files. So for now - and only to complete the safety cap - makefs should refuse to record data files which must be represented as multi-extent files. I.e. files with more than 4 GiB -1 bytes of content. This test is needed anyways to make sure that chosen level 1 and 2 do not exceed the limits which are specified in ECMA-119 chapter 10 "Levels of interchange": "10.1 Level 1. - each file shall consist of only one File Section; [...] 10.2 Level 2 - each file shall consist of only one File Section; [...] " File Section is what other filesystems name "Extent". The byte counter of a File Section is an unsigned 32 bit number (written doubly in both endiannesses, as usual in ISO 9660). So if the --isolevel option is given, then user may expect to be protected from shooting the own foot by oversized data files.
(In reply to scdbackup from comment #9) Closing bug. Will talk with emaste about implementing the enhancements discussed in comment # 9 and will CC you on the bug. Please don't reopen this bug.