This small set fixes few bugs, improves gpt handling (under buggy conditions) and implements strict flag with more fine grained control which should fix issues with sanity checks against disk sizes. If this set is allright I'd want to do what I mentioned in older discussion with Ady - backport missing patches from 6.x to 5.x and 4.x so all versions have up to date chain version. Michal Soltys (6): chain/partiter: fix and improve gpt handling in buggy cases chain/partiter: fix possible non-NULL value returned by pi_begin() on error chain/partiter: adjust error reporting chain: add missing pi_del() in find*() functions chain: implement strict=<0|1|2> chain/partiter: correct gpt header checks com32/chain/chain.c | 64 ++++++------- com32/chain/mangle.c | 6 +- com32/chain/options.c | 30 +++++-- com32/chain/partiter.c | 234 ++++++++++++++++++++++++++++++------------------ com32/chain/partiter.h | 9 +- doc/chain.txt | 27 ++++-- 6 files changed, 222 insertions(+), 148 deletions(-) -- 1.7.10.4
Michal Soltys
2014-Jun-29 19:41 UTC
[syslinux] [PATCH 1/6] chain/partiter: fix and improve gpt handling in buggy cases
Previous version had some issues in case of error situations, among
those:
- backup gpt header was not read if reading of primary failed
- alternating nature of lba_cur and lba_alt was ignored
This patch fixes this and extends the gpt handling.
The current behavior is:
- try primary header; if unreadable or checksum fails (or sanity checks
fail unless relax flag is set) - try secondary header
- try main partition table referenced in the header that was read; if
it's unreadable or if its crc check fails - try alternative (so if we
managed to read primary header, table at the end would be considered
as alternative - if we managed to read secondary header, table at the
beginning would be treated as such)
diff --git a/com32/chain/partiter.c b/com32/chain/partiter.c
index 1eb5350..ee19e8b 100644
--- a/com32/chain/partiter.c
+++ b/com32/chain/partiter.c
@@ -412,34 +412,16 @@ static inline int valid_crc(uint32_t crc, const uint8_t
*buf, unsigned int siz)
return crc == crc32(crc32(0, NULL, 0), buf, siz);
}
-static int gpt_check_hdr_crc(const struct disk_info * const diskinfo, struct
disk_gpt_header **_gh)
+static int valid_crc_hdr(void *buf)
{
- struct disk_gpt_header *gh = *_gh;
- uint64_t lba_alt;
- uint32_t hold_crc32;
+ struct disk_gpt_header *gh = buf;
+ uint32_t crc = gh->chksum;
+ int valid;
- hold_crc32 = gh->chksum;
gh->chksum = 0;
- if (!valid_crc(hold_crc32, (const uint8_t *)gh, gh->hdr_size)) {
- warn("Primary GPT header checksum invalid.");
- /* retry with backup */
- lba_alt = gh->lba_alt;
- free(gh);
- if (!(gh = *_gh = disk_read_sectors(diskinfo, lba_alt, 1))) {
- error("Couldn't read backup GPT header.");
- return -1;
- }
- hold_crc32 = gh->chksum;
- gh->chksum = 0;
- if (!valid_crc(hold_crc32, (const uint8_t *)gh, gh->hdr_size)) {
- error("Secondary GPT header checksum invalid.");
- return -1;
- }
- }
- /* restore old checksum */
- gh->chksum = hold_crc32;
-
- return 0;
+ valid = crc == crc32(crc32(0, NULL, 0), buf, gh->hdr_size);
+ gh->chksum = crc;
+ return valid;
}
static int pi_next_(struct part_iter *iter)
@@ -546,10 +528,108 @@ void pi_del(struct part_iter **_iter)
*_iter = NULL;
}
+static void try_gpt_we(const char *str, int sec)
+{
+ if (sec)
+ error(str);
+ else
+ warn(str);
+}
+
+static struct disk_gpt_header *try_gpt_hdr(const struct disk_info *di, int sec)
+{
+ const char *desc = sec ? "backup" : "primary";
+ uint64_t gpt_cur = sec ? di->lbacnt - 1 : 1;
+ struct disk_gpt_header *gpth;
+ char errbuf[64];
+
+ gpth = disk_read_sectors(di, gpt_cur, 1);
+ if (!gpth) {
+ sprintf(errbuf, "Unable to read %s GPT header.", desc);
+ try_gpt_we(errbuf, sec);
+ return NULL;
+ }
+ if(!valid_crc_hdr(gpth)) {
+ sprintf(errbuf, "Invalid checksum of %s GPT header.", desc);
+ try_gpt_we(errbuf, sec);
+ free(gpth);
+ return NULL;
+ }
+ return gpth;
+}
+
+static struct disk_gpt_part_entry *try_gpt_list(const struct disk_info *di,
const struct disk_gpt_header *gpth, int alt)
+{
+ int pri = gpth->lba_cur < gpth->lba_alt;
+ const char *desc = alt ? "alternative" : "main";
+ struct disk_gpt_part_entry *gptl;
+ char errbuf[64];
+ uint64_t gpt_lsiz; /* size of GPT partition list in bytes */
+ uint64_t gpt_lcnt; /* size of GPT partition in sectors */
+ uint64_t gpt_loff; /* offset to GPT partition list in sectors */
+
+ gpt_lsiz = (uint64_t)gpth->part_size * gpth->part_count;
+ gpt_lcnt = (gpt_lsiz + di->bps - 1) / di->bps;
+ if (!alt) {
+ /* prefer header value for partition table if not asking for alternative */
+ gpt_loff = gpth->lba_table;
+ } else {
+ /* try to read alternative, we have to calculate its position */
+ if (!pri)
+ gpt_loff = gpth->lba_alt + 1;
+ else
+ gpt_loff = gpth->lba_alt - gpt_lcnt;
+ }
+
+ gptl = disk_read_sectors(di, gpt_loff, gpt_lcnt);
+ if (!gptl) {
+ sprintf(errbuf, "Unable to read %s GPT partition list.", desc);
+ try_gpt_we(errbuf, alt);
+ return NULL;
+ }
+ if (!valid_crc(gpth->table_chksum, (const uint8_t *)gptl, gpt_lsiz)) {
+ sprintf(errbuf, "Invalid checksum of %s GPT partition list.", desc);
+ try_gpt_we(errbuf, alt);
+ free(gptl);
+ return NULL;
+ }
+ return gptl;
+}
+
+static int notsane_gpt_hdr(const struct disk_info *di, const struct
disk_gpt_header *gpth, int flags)
+{
+ uint64_t gpt_loff; /* offset to GPT partition list in sectors */
+ uint64_t gpt_lsiz; /* size of GPT partition list in bytes */
+ uint64_t gpt_lcnt; /* size of GPT partition in sectors */
+
+ if (flags & PIF_RELAX)
+ return 0;
+
+ gpt_loff = gpth->lba_table;
+ gpt_lsiz = (uint64_t)gpth->part_size * gpth->part_count;
+ gpt_lcnt = (gpt_lsiz + di->bps - 1) / di->bps;
+
+ /*
+ * disk_read_sectors allows reading of max 255 sectors, so we use
+ * it as a sanity check base. EFI doesn't specify max (AFAIK).
+ */
+ if (gpt_loff < 2 || !gpt_lsiz || gpt_lcnt > 255u ||
+ gpth->lba_first_usable > gpth->lba_last_usable ||
+ !sane(gpt_loff, gpt_lcnt) ||
+ gpt_loff + gpt_lcnt > gpth->lba_first_usable ||
+ !sane(gpth->lba_last_usable, gpt_lcnt) ||
+ gpth->lba_last_usable + gpt_lcnt >= gpth->lba_alt ||
+ gpth->lba_alt >= di->lbacnt ||
+ gpth->part_size < sizeof(struct disk_gpt_part_entry))
+ return -1;
+
+ return 0;
+}
+
/* pi_begin() - validate and and get proper iterator for a disk described by di
*/
struct part_iter *pi_begin(const struct disk_info *di, int flags)
{
- int gptprot, ret = -1;
+ int isgpt = 0, ret = -1;
struct part_iter *iter;
struct disk_dos_mbr *mbr = NULL;
struct disk_gpt_header *gpth = NULL;
@@ -561,7 +641,7 @@ struct part_iter *pi_begin(const struct disk_info *di, int
flags)
/* Read MBR */
if (!(mbr = disk_read_sectors(di, 0, 1))) {
- error("Couldn't read the first disk sector.");
+ error("Unable to read the first disk sector.");
goto bail;
}
@@ -574,69 +654,41 @@ struct part_iter *pi_begin(const struct disk_info *di, int
flags)
}
/* Check for GPT protective MBR */
- gptprot = 0;
for (size_t i = 0; i < 4; i++)
- gptprot |= (mbr->table[i].ostype == 0xEE);
- if (gptprot && !(flags & PIF_PREFMBR)) {
- if (!(gpth = disk_read_sectors(di, 1, 1))) {
- error("Couldn't read potential GPT header.");
+ isgpt |= (mbr->table[i].ostype == 0xEE);
+ isgpt = isgpt && !(flags & PIF_PREFMBR);
+
+ /* Try to read GPT header */
+ if (isgpt) {
+ gpth = try_gpt_hdr(di, 0);
+ if (!gpth)
+ /*
+ * this read might fail if bios reports different disk size (different
vm/pc)
+ * not much we can do here to avoid it
+ */
+ gpth = try_gpt_hdr(di, 1);
+ if (!gpth)
goto bail;
- }
}
if (gpth && gpth->rev.uint32 == 0x00010000 &&
!memcmp(gpth->sig, disk_gpt_sig_magic, sizeof gpth->sig)) {
/* looks like GPT v1.0 */
- uint64_t gpt_loff; /* offset to GPT partition list in sectors */
- uint64_t gpt_lsiz; /* size of GPT partition list in bytes */
- uint64_t gpt_lcnt; /* size of GPT partition in sectors */
#ifdef DEBUG
dprintf("Looks like a GPT v1.0 disk.\n");
disk_gpt_header_dump(gpth);
#endif
- /* Verify checksum, fallback to backup, then bail if invalid */
- if (gpt_check_hdr_crc(di, &gpth))
- goto bail;
-
- gpt_loff = gpth->lba_table;
- gpt_lsiz = (uint64_t)gpth->part_size * gpth->part_count;
- gpt_lcnt = (gpt_lsiz + di->bps - 1) / di->bps;
-
- /*
- * disk_read_sectors allows reading of max 255 sectors, so we use
- * it as a sanity check base. EFI doesn't specify max (AFAIK).
- * Apart from that, some extensive sanity checks.
- */
- if (!(flags & PIF_RELAX) && (
- !gpt_loff || !gpt_lsiz || gpt_lcnt > 255u ||
- gpth->lba_first_usable > gpth->lba_last_usable ||
- !sane(gpt_loff, gpt_lcnt) ||
- gpt_loff + gpt_lcnt > gpth->lba_first_usable ||
- !sane(gpth->lba_last_usable, gpt_lcnt) ||
- gpth->lba_last_usable + gpt_lcnt >= gpth->lba_alt ||
- gpth->lba_alt >= di->lbacnt ||
- gpth->part_size < sizeof *gptl)) {
- error("Invalid GPT header's values.");
+ if (notsane_gpt_hdr(di, gpth, flags)) {
+ error("GPT header values are corrupted.");
goto bail;
}
- if (!(gptl = disk_read_sectors(di, gpt_loff, gpt_lcnt))) {
- error("Couldn't read GPT partition list.");
+
+ gptl = try_gpt_list(di, gpth, 0);
+ if (!gptl)
+ gptl = try_gpt_list(di, gpth, 1);
+ if (!gptl)
goto bail;
- }
- /* Check array checksum(s). */
- if (!valid_crc(gpth->table_chksum, (const uint8_t *)gptl, (unsigned
int)gpt_lsiz)) {
- warn("Checksum of the main GPT partition list is invalid, trying
backup.");
- free(gptl);
- /* secondary array directly precedes secondary header */
- if (!(gptl = disk_read_sectors(di, gpth->lba_alt - gpt_lcnt,
gpt_lcnt))) {
- error("Couldn't read backup GPT partition list.");
- goto bail;
- }
- if (!valid_crc(gpth->table_chksum, (const uint8_t *)gptl, gpt_lsiz)) {
- error("Checksum of the backup GPT partition list is invalid, giving
up.");
- goto bail;
- }
- }
+
/* looks like GPT */
ret = pi_gpt_ctor(iter, di, flags, gpth, gptl);
} else {
--
1.7.10.4
Michal Soltys
2014-Jun-29 19:41 UTC
[syslinux] [PATCH 2/6] chain/partiter: fix possible non-NULL value returned by pi_begin() on error
This patch fixes non-NULL value returned from pi_begin() in case of an
error (which resulted in further hang instead of proper exit). Probably
a leftover from pi_dealloc() times.
diff --git a/com32/chain/partiter.c b/com32/chain/partiter.c
index ee19e8b..5c5b015 100644
--- a/com32/chain/partiter.c
+++ b/com32/chain/partiter.c
@@ -637,12 +637,12 @@ struct part_iter *pi_begin(const struct disk_info *di, int
flags)
/* Preallocate iterator */
if (!(iter = pi_alloc()))
- goto bail;
+ goto out;
/* Read MBR */
if (!(mbr = disk_read_sectors(di, 0, 1))) {
error("Unable to read the first disk sector.");
- goto bail;
+ goto out;
}
/* Check for MBR magic */
@@ -650,7 +650,7 @@ struct part_iter *pi_begin(const struct disk_info *di, int
flags)
warn("No MBR magic, treating disk as raw.");
/* looks like RAW */
ret = pi_ctor(iter, di, flags);
- goto bail;
+ goto out;
}
/* Check for GPT protective MBR */
@@ -668,7 +668,7 @@ struct part_iter *pi_begin(const struct disk_info *di, int
flags)
*/
gpth = try_gpt_hdr(di, 1);
if (!gpth)
- goto bail;
+ goto out;
}
if (gpth && gpth->rev.uint32 == 0x00010000 &&
@@ -680,14 +680,14 @@ struct part_iter *pi_begin(const struct disk_info *di, int
flags)
#endif
if (notsane_gpt_hdr(di, gpth, flags)) {
error("GPT header values are corrupted.");
- goto bail;
+ goto out;
}
gptl = try_gpt_list(di, gpth, 0);
if (!gptl)
gptl = try_gpt_list(di, gpth, 1);
if (!gptl)
- goto bail;
+ goto out;
/* looks like GPT */
ret = pi_gpt_ctor(iter, di, flags, gpth, gptl);
@@ -695,9 +695,11 @@ struct part_iter *pi_begin(const struct disk_info *di, int
flags)
/* looks like MBR */
ret = pi_dos_ctor(iter, di, flags, mbr);
}
-bail:
- if (ret < 0)
+out:
+ if (ret < 0) {
free(iter);
+ iter = NULL;
+ }
free(mbr);
free(gpth);
free(gptl);
--
1.7.10.4
Michal Soltys
2014-Jun-29 19:41 UTC
[syslinux] [PATCH 3/6] chain/partiter: adjust error reporting
Use <0 for errors, 0 for normal state, and >0 for clean completion.
In future this would be necessary if it's decided to make partiter
a generic lib (similar to disklib) - though it has to be quieted first
and provide strerr()-like functionality in place of its verbosity.
diff --git a/com32/chain/chain.c b/com32/chain/chain.c
index ae95d45..c08ec6e 100644
--- a/com32/chain/chain.c
+++ b/com32/chain/chain.c
@@ -352,7 +352,7 @@ int find_dp(struct part_iter **_iter)
} while (!pi_next(iter));
/* broken part structure or other problems */
if (iter->status) {
- error("Can't find myself on the drive I booted from.");
+ error("Unable to find partition with syslinux (fs).");
goto bail;
}
}
@@ -372,7 +372,7 @@ int find_dp(struct part_iter **_iter)
break;
} while (!pi_next(iter));
if (iter->status) {
- error("Requested disk / partition combination not found.");
+ error("Unable to find requested disk / partition combination.");
goto bail;
}
}
diff --git a/com32/chain/mangle.c b/com32/chain/mangle.c
index ffdaab8..6963574 100644
--- a/com32/chain/mangle.c
+++ b/com32/chain/mangle.c
@@ -562,7 +562,7 @@ int manglepe_hide(struct part_iter *miter)
}
}
- if (pi_errored(iter))
+ if (iter->status < 0)
goto bail;
/* last update */
@@ -663,7 +663,7 @@ int manglepe_fixchs(struct part_iter *miter)
}
}
- if (pi_errored(iter))
+ if (iter->status < 0)
goto bail;
/* last update */
diff --git a/com32/chain/partiter.h b/com32/chain/partiter.h
index 13dec84..d01a650 100644
--- a/com32/chain/partiter.h
+++ b/com32/chain/partiter.h
@@ -42,7 +42,7 @@
/* status */
-enum {PI_OK, PI_DONE, PI_INSANE, PI_ERRLOAD};
+enum {PI_ERRLOAD = -31, PI_INSANE, PI_OK = 0, PI_DONE};
/* flags */
@@ -103,11 +103,6 @@ extern const struct itertype * const typeraw;
struct part_iter *pi_begin(const struct disk_info *, int flags);
void pi_del(struct part_iter **);
-static inline int pi_errored(struct part_iter *iter)
-{
- return iter->status > PI_DONE;
-}
-
/* inline virtuals */
static inline int pi_next(struct part_iter *iter)
{
--
1.7.10.4
Michal Soltys
2014-Jun-29 19:41 UTC
[syslinux] [PATCH 4/6] chain: add missing pi_del() in find*() functions
As partiter doesn't deallocate itself after finish (anymore), it should
be deleted after each loop iteration.
diff --git a/com32/chain/chain.c b/com32/chain/chain.c
index c08ec6e..f86cd7d 100644
--- a/com32/chain/chain.c
+++ b/com32/chain/chain.c
@@ -64,27 +64,23 @@ static int is_phys(uint8_t sdifs)
static int find_by_sig(uint32_t mbr_sig,
struct part_iter **_boot_part)
{
- struct part_iter *boot_part = NULL;
+ struct part_iter *iter = NULL;
struct disk_info diskinfo;
int drive;
for (drive = 0x80; drive < 0x80 + fixed_cnt; drive++) {
if (disk_get_params(drive, &diskinfo))
continue; /* Drive doesn't exist */
- if (!(boot_part = pi_begin(&diskinfo, opt.piflags)))
- continue;
- /* Check for a MBR disk */
- if (boot_part->type != typedos) {
- pi_del(&boot_part);
+ if (!(iter = pi_begin(&diskinfo, opt.piflags)))
continue;
- }
- if (boot_part->dos.disk_sig == mbr_sig) {
+ /* Check for a matching MBR disk */
+ if (iter->type == typedos && iter->dos.disk_sig == mbr_sig)
goto ok;
- }
+ pi_del(&iter);
}
drive = -1;
ok:
- *_boot_part = boot_part;
+ *_boot_part = iter;
return drive;
}
@@ -95,29 +91,26 @@ ok:
static int find_by_guid(const struct guid *gpt_guid,
struct part_iter **_boot_part)
{
- struct part_iter *boot_part = NULL;
+ struct part_iter *iter = NULL;
struct disk_info diskinfo;
int drive;
for (drive = 0x80; drive < 0x80 + fixed_cnt; drive++) {
if (disk_get_params(drive, &diskinfo))
continue; /* Drive doesn't exist */
- if (!(boot_part = pi_begin(&diskinfo, opt.piflags)))
- continue;
- /* Check for a GPT disk */
- if (boot_part->type != typegpt) {
- pi_del(&boot_part);
+ if (!(iter = pi_begin(&diskinfo, opt.piflags)))
continue;
- }
/* Check for a matching GPT disk/partition guid */
- do {
- if (!memcmp(&boot_part->gpt.part_guid, gpt_guid, sizeof *gpt_guid))
- goto ok;
- } while (!pi_next(boot_part));
+ if (iter->type == typegpt)
+ do {
+ if (!memcmp(&iter->gpt.part_guid, gpt_guid, sizeof *gpt_guid))
+ goto ok;
+ } while (!pi_next(iter));
+ pi_del(&iter);
}
drive = -1;
ok:
- *_boot_part = boot_part;
+ *_boot_part = iter;
return drive;
}
@@ -127,29 +120,26 @@ ok:
*/
static int find_by_label(const char *label, struct part_iter **_boot_part)
{
- struct part_iter *boot_part = NULL;
+ struct part_iter *iter = NULL;
struct disk_info diskinfo;
int drive;
for (drive = 0x80; drive < 0x80 + fixed_cnt; drive++) {
if (disk_get_params(drive, &diskinfo))
continue; /* Drive doesn't exist */
- if (!(boot_part = pi_begin(&diskinfo, opt.piflags)))
- continue;
- /* Check for a GPT disk */
- if (!(boot_part->type == typegpt)) {
- pi_del(&boot_part);
+ if (!(iter = pi_begin(&diskinfo, opt.piflags)))
continue;
- }
- /* Check for a matching partition */
- while (!pi_next(boot_part)) {
- if (!strcmp(label, boot_part->gpt.part_label))
- goto ok;
- }
+ /* Check for a matching GPT partition label */
+ if (iter->type == typegpt)
+ while (!pi_next(iter)) {
+ if (!strcmp(label, iter->gpt.part_label))
+ goto ok;
+ }
+ pi_del(&iter);
}
drive = -1;
ok:
- *_boot_part = boot_part;
+ *_boot_part = iter;
return drive;
}
--
1.7.10.4
Michal Soltys
2014-Jun-29 19:41 UTC
[syslinux] [PATCH 5/6] chain: implement strict=<0|1|2>
This provides more fine grained control than single relax flag. to cover
case with wrong disk sizes.
relax and nostrict are equivalent to strict=0
norelax and strict are equivalent to strict=2
strict=1 does the same as strict=2, but ignores checks against disk size
The current default is strict=1. Options: 'fixchs',
'[un]hide[all]' and
'save' will forcibly enable strict=2 (can be overridden by the user).
diff --git a/com32/chain/mangle.c b/com32/chain/mangle.c
index 6963574..3231e51 100644
--- a/com32/chain/mangle.c
+++ b/com32/chain/mangle.c
@@ -602,7 +602,7 @@ static int updchs(struct part_iter *iter, int ext)
* lower than the start CHS.
*
* Both are harmless in case of a hole (and in non-hole case will make
- * partiter complain about corrupt layout unless PIF_RELAX is set), but it
+ * partiter complain about corrupt layout if PIF_STRICT is set), but it
* makes everything look silly and not really correct.
*
* Thus the approach as seen below.
diff --git a/com32/chain/options.c b/com32/chain/options.c
index 4e722a0..a99e0d7 100644
--- a/com32/chain/options.c
+++ b/com32/chain/options.c
@@ -132,7 +132,9 @@ static void usage(void)
" keeppxe Keep the PXE and UNDI stacks in memory
(PXELINUX)",
" warn Wait for a keypress to continue
chainloading",
" break Don't chainload",
-" relax Relax sanity checks",
+" strict[=<0|1|2>] Set the level of strictness in sanity
checks",
+" - strict w/o any value is the same as
strict=2",
+" relax The same as strict=0",
" prefmbr On hybrid MBR/GPT disks, prefer legacy
layout",
"",
" file=<file> Load and execute <file>",
@@ -172,6 +174,7 @@ void opt_set_defs(void)
opt.maps = true; /* by def. map sector */
opt.hand = true; /* by def. prepare handover */
opt.brkchain = false; /* by def. do chainload */
+ opt.piflags = PIF_STRICT; /* by def. be strict, but ignore disk sizes */
opt.foff = opt.soff = opt.fip = opt.sip = 0x7C00;
opt.drivename = "boot";
#ifdef DEBUG
@@ -303,12 +306,16 @@ int opt_parse_args(int argc, char *argv[])
opt.hide = HIDE_OFF;
} else if (!strcmp(argv[i], "hide")) {
opt.hide = HIDE_ON;
+ opt.piflags |= PIF_STRICT | PIF_STRICTER;
} else if (!strcmp(argv[i], "hideall")) {
opt.hide = HIDE_ON | HIDE_EXT;
+ opt.piflags |= PIF_STRICT | PIF_STRICTER;
} else if (!strcmp(argv[i], "unhide")) {
opt.hide = HIDE_ON | HIDE_REV;
+ opt.piflags |= PIF_STRICT | PIF_STRICTER;
} else if (!strcmp(argv[i], "unhideall")) {
opt.hide = HIDE_ON | HIDE_EXT | HIDE_REV;
+ opt.piflags |= PIF_STRICT | PIF_STRICTER;
} else if (!strcmp(argv[i], "setbpb")) {
opt.setbpb = true;
} else if (!strcmp(argv[i], "nosetbpb")) {
@@ -329,16 +336,29 @@ int opt_parse_args(int argc, char *argv[])
opt.maps = false;
} else if (!strcmp(argv[i], "save")) {
opt.save = true;
+ opt.piflags |= PIF_STRICT | PIF_STRICTER;
} else if (!strcmp(argv[i], "nosave")) {
opt.save = false;
} else if (!strcmp(argv[i], "fixchs")) {
opt.fixchs = true;
+ opt.piflags |= PIF_STRICT | PIF_STRICTER;
} else if (!strcmp(argv[i], "nofixchs")) {
opt.fixchs = false;
- } else if (!strcmp(argv[i], "relax")) {
- opt.piflags |= PIF_RELAX;
- } else if (!strcmp(argv[i], "norelax")) {
- opt.piflags &= ~PIF_RELAX;
+ } else if (!strcmp(argv[i], "relax") || !strcmp(argv[i],
"nostrict")) {
+ opt.piflags &= ~(PIF_STRICT | PIF_STRICTER);
+ } else if (!strcmp(argv[i], "norelax") || !strcmp(argv[i],
"strict")) {
+ opt.piflags |= PIF_STRICT | PIF_STRICTER;
+ } else if (!strncmp(argv[i], "strict=", 7)) {
+ if (argv[i][7] < '0' || argv[i][7] > '2' ||
!argv[i][8]) {
+ error("Strict level must be 0, 1 or 2.");
+ goto bail;
+ }
+ opt.piflags &= ~(PIF_STRICT | PIF_STRICTER);
+ switch (argv[i][7]) {
+ case '2': opt.piflags |= PIF_STRICTER;
+ case '1': opt.piflags |= PIF_STRICT; break;
+ default:;
+ }
} else if (!strcmp(argv[i], "warn")) {
opt.warn = true;
} else if (!strcmp(argv[i], "nowarn")) {
diff --git a/com32/chain/partiter.c b/com32/chain/partiter.c
index 5c5b015..beeb1bd 100644
--- a/com32/chain/partiter.c
+++ b/com32/chain/partiter.c
@@ -176,7 +176,7 @@ static int notsane_logical(const struct part_iter *iter)
return -1;
}
- if (iter->flags & PIF_RELAX)
+ if (!(iter->flags & PIF_STRICT))
return 0;
end_log = dp[0].start_lba + dp[0].length;
@@ -215,7 +215,7 @@ static int notsane_extended(const struct part_iter *iter)
return -1;
}
- if (iter->flags & PIF_RELAX)
+ if (!(iter->flags & PIF_STRICT))
return 0;
end_ebr = dp[1].start_lba + dp[1].length;
@@ -245,13 +245,13 @@ static int notsane_primary(const struct part_iter *iter)
if (!dp->ostype)
return 0;
- if (iter->flags & PIF_RELAX)
+ if (!(iter->flags & PIF_STRICT))
return 0;
if (!dp->start_lba ||
!dp->length ||
!sane(dp->start_lba, dp->length) ||
- dp->start_lba + dp->length > iter->di.lbacnt) {
+ ((iter->flags & PIF_STRICTER) && (dp->start_lba +
dp->length > iter->di.lbacnt))) {
error("Primary partition (in MBR) with invalid offset and/or
length.");
return -1;
}
@@ -268,7 +268,7 @@ static int notsane_gpt(const struct part_iter *iter)
if (guid_is0(&gp->type))
return 0;
- if (iter->flags & PIF_RELAX)
+ if (!(iter->flags & PIF_STRICT))
return 0;
if (gp->lba_first < iter->gpt.ufirst ||
@@ -602,7 +602,7 @@ static int notsane_gpt_hdr(const struct disk_info *di, const
struct disk_gpt_hea
uint64_t gpt_lsiz; /* size of GPT partition list in bytes */
uint64_t gpt_lcnt; /* size of GPT partition in sectors */
- if (flags & PIF_RELAX)
+ if (!(flags & PIF_STRICT))
return 0;
gpt_loff = gpth->lba_table;
@@ -619,7 +619,7 @@ static int notsane_gpt_hdr(const struct disk_info *di, const
struct disk_gpt_hea
gpt_loff + gpt_lcnt > gpth->lba_first_usable ||
!sane(gpth->lba_last_usable, gpt_lcnt) ||
gpth->lba_last_usable + gpt_lcnt >= gpth->lba_alt ||
- gpth->lba_alt >= di->lbacnt ||
+ ((flags & PIF_STRICTER) && (gpth->lba_alt >=
di->lbacnt)) ||
gpth->part_size < sizeof(struct disk_gpt_part_entry))
return -1;
diff --git a/com32/chain/partiter.h b/com32/chain/partiter.h
index d01a650..22c0e42 100644
--- a/com32/chain/partiter.h
+++ b/com32/chain/partiter.h
@@ -46,7 +46,7 @@ enum {PI_ERRLOAD = -31, PI_INSANE, PI_OK = 0, PI_DONE};
/* flags */
-enum {PIF_STEPALL = 1, PIF_RELAX = 2, PIF_PREFMBR = 4};
+enum {PIF_STEPALL = 1, PIF_PREFMBR = 2, PIF_STRICT = 4, PIF_STRICTER = 8};
struct itertype;
struct part_iter;
diff --git a/doc/chain.txt b/doc/chain.txt
index 2321c10..effd508 100644
--- a/doc/chain.txt
+++ b/doc/chain.txt
@@ -160,6 +160,7 @@ useful to also fix its BPB values.
save
*nosave
+ save sets: strict=2
Fixing BPB values only in memory might not be enough. This option allows
writing of the corrected sector. You will probably want to use this option
@@ -195,6 +196,7 @@ drive we use during chainloading is not fd0 or hd0.
hide[all]
unhide[all]
*nohide
+ [un]hide[all] sets: strict=2
In certain situations it's useful to hide partitions - for example to make
sure
DOS gets C:. 'hide' will hide hidable primary partitions, except the
one we're
@@ -205,6 +207,7 @@ Writing is only performed, if the os type values actually
changed.
fixchs
*nofixchs
+ fixchs sets: strict=2
If you want to make a drive you're booting from totally compatible with
current
BIOS, you can use this to fix all partitions' CHS numbers. Good to silence
e.g.
@@ -232,15 +235,23 @@ Useful to see warnings emited by the chain module.
In the case of presence of non-standard hybrid MBR/GPT layout, this flag makes
chain module prefer MBR layout over GPT.
+ strict[=<0|1|2>]
+ *strict=1
relax
- *norelax
-
-This option inhibits sanity checks during the traversal of the partition table.
-This is potentially useful in corner cases, when for example an usb stick moved
-to some different computer would report smaller size than previously with
-partitions spanning the whole space. Normally partition iterator would report
-an error and abort in such case. Another case scenario is disk corruption in
-some later EMBR partition.
+
+Those options control the level of sanity checks used during the traversal of
+partition table(s). This is useful in buggy corner cases, when the disk size is
+reported differently across different computers or virtual machines (if it
+happens at all, the size usually differs by 1 sector). Normally the partition
+iterator would report an error and abort in such case. Another case scenario is
+disk corruption in some later EMBR partition.
+
+- strict=0 inhibits any checks
+- strict=1 enables checks, but ignores those that involve disk size
+- strict=2 enables all checks
+- relax and nostrict are equivalent to strict=0
+- norelax and strict are equivalent to strict=2
+
break
*nobreak
--
1.7.10.4
Michal Soltys
2014-Jun-29 19:41 UTC
[syslinux] [PATCH 6/6] chain/partiter: correct gpt header checks
In gpt header, lba_cur and lba_alt alternate depending on whether we
read primary or backup copy.
diff --git a/com32/chain/partiter.c b/com32/chain/partiter.c
index beeb1bd..d570d93 100644
--- a/com32/chain/partiter.c
+++ b/com32/chain/partiter.c
@@ -601,10 +601,15 @@ static int notsane_gpt_hdr(const struct disk_info *di,
const struct disk_gpt_hea
uint64_t gpt_loff; /* offset to GPT partition list in sectors */
uint64_t gpt_lsiz; /* size of GPT partition list in bytes */
uint64_t gpt_lcnt; /* size of GPT partition in sectors */
+ uint64_t gpt_sec; /* secondary gpt header */
if (!(flags & PIF_STRICT))
return 0;
+ if (gpth->lba_alt < gpth->lba_cur)
+ gpt_sec = gpth->lba_cur;
+ else
+ gpt_sec = gpth->lba_alt;
gpt_loff = gpth->lba_table;
gpt_lsiz = (uint64_t)gpth->part_size * gpth->part_count;
gpt_lcnt = (gpt_lsiz + di->bps - 1) / di->bps;
@@ -616,10 +621,9 @@ static int notsane_gpt_hdr(const struct disk_info *di,
const struct disk_gpt_hea
if (gpt_loff < 2 || !gpt_lsiz || gpt_lcnt > 255u ||
gpth->lba_first_usable > gpth->lba_last_usable ||
!sane(gpt_loff, gpt_lcnt) ||
- gpt_loff + gpt_lcnt > gpth->lba_first_usable ||
- !sane(gpth->lba_last_usable, gpt_lcnt) ||
- gpth->lba_last_usable + gpt_lcnt >= gpth->lba_alt ||
- ((flags & PIF_STRICTER) && (gpth->lba_alt >=
di->lbacnt)) ||
+ (gpt_loff + gpt_lcnt > gpth->lba_first_usable && gpt_loff
<= gpth->lba_last_usable) ||
+ gpt_loff + gpt_lcnt > gpt_sec ||
+ ((flags & PIF_STRICTER) && (gpt_sec >= di->lbacnt)) ||
gpth->part_size < sizeof(struct disk_gpt_part_entry))
return -1;
--
1.7.10.4
Op 2014-06-29 om 21:41 schreef Michal Soltys:> This small set fixes few bugs, improves gpt handling (under buggy conditions) > and implements strict flag with more fine grained control which should fix > issues with sanity checks against disk sizes. > > If this set is allright I'd want to do what I mentioned in older discussion > with Ady - backport missing patches from 6.x to 5.x and 4.x so all versions > have up to date chain version. > > > Michal Soltys (6): > chain/partiter: fix and improve gpt handling in buggy cases > chain/partiter: fix possible non-NULL value returned by pi_begin() on error > chain/partiter: adjust error reporting > chain: add missing pi_del() in find*() functions > chain: implement strict=<0|1|2> > chain/partiter: correct gpt header checks > > com32/chain/chain.c | 64 ++++++------- > com32/chain/mangle.c | 6 +- > com32/chain/options.c | 30 +++++-- > com32/chain/partiter.c | 234 ++++++++++++++++++++++++++++++------------------ > com32/chain/partiter.h | 9 +- > doc/chain.txt | 27 ++++-- > 6 files changed, 222 insertions(+), 148 deletions(-) >How helpfull is this reminder? Groeten Geert Stappers -- Leven en laten leven
> This small set fixes few bugs, improves gpt handling (under buggy conditions) > and implements strict flag with more fine grained control which should fix > issues with sanity checks against disk sizes. > > If this set is allright I'd want to do what I mentioned in older discussion > with Ady - backport missing patches from 6.x to 5.x and 4.x so all versions > have up to date chain version. > > > Michal Soltys (6): > chain/partiter: fix and improve gpt handling in buggy cases > chain/partiter: fix possible non-NULL value returned by pi_begin() on error > chain/partiter: adjust error reporting > chain: add missing pi_del() in find*() functions > chain: implement strict=<0|1|2> > chain/partiter: correct gpt header checks > > com32/chain/chain.c | 64 ++++++------- > com32/chain/mangle.c | 6 +- > com32/chain/options.c | 30 +++++-- > com32/chain/partiter.c | 234 ++++++++++++++++++++++++++++++------------------ > com32/chain/partiter.h | 9 +- > doc/chain.txt | 27 ++++-- > 6 files changed, 222 insertions(+), 148 deletions(-) > > -- > 1.7.10.4 >This set of patches has been included in 6.03-pre20. If someone had problems with chain.c32 (with whichever branch or version), it would be helpful to test with 6.03-pre20 and report back. Regressions are important too. @Michal, Is there some specific case that should be tested (specially if it wasn't working before and it is supposed to work now)? Is there some specific hardware / BIOS version that is known to have problems with prior chain.c32 versions that should be now re-tested (perhaps with some new combination of flags)? Thank you and Regards, Ady.
On 2014-06-29 21:41, Michal Soltys wrote:> This small set fixes few bugs, improves gpt handling (under buggy conditions) > and implements strict flag with more fine grained control which should fix > issues with sanity checks against disk sizes. > > If this set is allright I'd want to do what I mentioned in older discussion > with Ady - backport missing patches from 6.x to 5.x and 4.x so all versions > have up to date chain version. >So any yes/no regarding those patches or maybe aim at some different approach ?
On 08/27/2014 09:50 AM, Michal Soltys wrote:> On 2014-06-29 21:41, Michal Soltys wrote: >> This small set fixes few bugs, improves gpt handling (under buggy >> conditions) >> and implements strict flag with more fine grained control which should >> fix >> issues with sanity checks against disk sizes. >> >> If this set is allright I'd want to do what I mentioned in older >> discussion >> with Ady - backport missing patches from 6.x to 5.x and 4.x so all >> versions >> have up to date chain version. > > So any yes/no regarding those patches or maybe aim at some different > approach ? >If you want to backport, it would be best if you could provide a pullable git tree with the backport. I simply don't have any time to spend on the older branches anymore. -hpa