problems parsing partition table?

[miker00lz]

i'm working on hard drive and partition support in my OS, and i've been having an issue getting the right numbers for where a partition starts on a hard disk. i've been using the info on the MBR layout from wikipedia, and my structure and math looks right to me, but apparently i'm doing something wrong.

i have no problems working with floppy disks. here's a screen shot of it trying to enumerate partitions on a hard drive. it's a single FAT16 partition, but it's reading everything all wrong and apparently it's because it's not calculating the correct starting point of the volume on the disk.




and i had it print out some debug info to vtty 2, and it's like it's calculating the CHS incorrectly? it should be starting to read the partition at offset 32256, but it apparently thinks it starts at 24064. i've verified that viewing the drive image in a hex editor.





here's the relevant code. go easy on me, i'm relatively new to C. i'm a BASIC veteran. still learning the C ropes. the disk reading routines which aren't pasted here should be just fine, they're flawless on floppies. you can see in the first screenshot the last line gets the disk label string correctly.

Code: Select all

void readparts(uint8 disknum) {
	struct structpart {
		uint8 status;
		uint8 head1;
		uint8 sectcyl11;
		uint8 sectcyl12;
		uint8 parttype;
		uint8 head2;
		uint8 sectcyl21;
		uint8 sectcyl22;
		uint32 lbastart;
		uint32 sectorcount;
	} tmppart;
	uint8 mbr[512];

	uint32 templong;
	uint16 tempword, tempvol;
	uint8 tempbyte = 0;

	sprintf(tmpstr, "        enumvol: "); ttyprint(maintty, tmpstr);

	if (readdisk(disknum, 0, 512, &mbr[0])) {
		sprintf(tmpstr, "error reading disk!\r\n"); ttyprint(maintty, tmpstr);
		return;
	} else if (mbr[510]!=0x55 || mbr[511]!=0xAA) {
		sprintf(tmpstr, "invalid partition table!\r\n"); ttyprint(maintty, tmpstr);
		return;
	}

	for (tempvol=0; tempvol<4; tempvol++) {
		if (readdisk(disknum, 0x1BE+(tempvol*16), 16, &tmppart)==0)
		  if ((tmppart.status==0 || tmppart.status==0x80) && (tmppart.parttype==0x04 || tmppart.parttype==0x06)) {
			volmap[lastvol].cyl1 = (((uint16)tmppart.sectcyl12&0xC0)<<2)|(uint16)tmppart.sectcyl11;
			volmap[lastvol].head1 = tmppart.head1;
			volmap[lastvol].sect1 = tmppart.sectcyl11&63;
			volmap[lastvol].cyl2 = (((uint16)tmppart.sectcyl22&0xC0)<<2)|(uint16)tmppart.sectcyl21;
			volmap[lastvol].head2 = tmppart.head2;
			volmap[lastvol].sect2 = tmppart.sectcyl21&63;
			volmap[lastvol].exist = 1;
			volmap[lastvol].sects = tmppart.sectorcount;
			volmap[lastvol].drive = disknum;
			//the "heads+1" in the next 2 lines are correct, the variable stores that and the cylinder count as the max addressable value, not the absolute total number.
			volmap[lastvol].startsect = ((uint32)volmap[lastvol].cyl1*((uint32)disk[disknum].heads+1)*(uint32)disk[disknum].spt)+(((uint32)volmap[lastvol].head1)*(uint32)disk[disknum].spt)+(uint32)volmap[lastvol].sect1-1;
			volmap[lastvol].endsect = ((uint32)volmap[lastvol].cyl2*((uint32)disk[disknum].heads+1)*(uint32)disk[disknum].spt)+(((uint32)volmap[lastvol].head2)*(uint32)disk[disknum].spt)+(uint32)volmap[lastvol].sect2-1;
			tmppart.sectorcount = volmap[lastvol].endsect - volmap[lastvol].startsect;
			sprintf(tmpstr, "\r\nparttype: %x\r\nhead1: %u\r\nsect1: %u\r\ncyl1: %u\r\n", tmppart.parttype, volmap[lastvol].head1, volmap[lastvol].sect1, volmap[lastvol].cyl1);
			ttyprint(maintty+1, tmpstr);

			initvol(lastvol++); //this calls a rotine in the FAT driver module that examines the volume boot record, works great on floppies.
			sprintf(tmpstr, "%u (%u MB), ", tempvol, (tmppart.sectorcount*512)/1048576);
			ttyprint(maintty, tmpstr);
			tempbyte++;
		}
	}
	sprintf(tmpstr, "%c%c [%u partitions]\r\n", 8, 8, tempbyte); ttyprint(maintty, tmpstr);
}

here is the volmap structure that's declared elsewhere:

Code: Select all

struct structvolmap {
	uint8 drive;
	uint8 drivepartnum;
	uint32 startsect;
	uint32 endsect;
	uint16 spt;
	uint8 heads;
	uint8 sects;
	uint16 cyl1;
	uint8 sect1;
	uint8 head1;
	uint16 cyl2;
	uint8 head2;
	uint8 sect2;
	char exist;
} volmap[0x100];

can anybody see any problems with my calculations? thanks!

[miker00lz]

just want to add that if i force the initvol function to start try parsing the partition at offset 32256, via hardcoding to start there, it correctly gets the partition label "MSDOS710 "

so, yes the disk reading routine is definitely working correctly. it has to be something in the code i've pasted. i just wanted to be sure of that.

[miker00lz]

Why not start by zeroing the structures you have allocated on the stack?

it's putting my structures on the stack? again, i've not been using C that long but i didn't think it would be putting them in the stack. that wouldn't make any sense at all. are you talking about the tmppart structure? i might be misunderstanding you..

i didn't think it allocated the structures RAM in the stack, i thought it would do it in some other area. would zeroing it make a difference? the readdisk call would overwrite whatevers there anyway.

[Combuster]

Start with checking all parameters: What's the CHS geometry of the image? What geometry does IDENTIFY return? What's the starting and ending LBA of the partition, and what is the starting and ending LBA in the partition table, and do both fit within the reported geometry (both CHS and LBA)? QEmu guesses geometry by default, as does linux' fdisk/parted, and you will probably have supplied or generated geometry for the harddisk image as well. If there's any mismatch, it will appear that CHS offsets are wrong, whereas LBA isn't.

[miker00lz]

Start with checking all parameters: What's the CHS geometry of the image? What geometry does IDENTIFY return? What's the starting and ending LBA of the partition, and what is the starting and ending LBA in the partition table, and do both fit within the reported geometry (both CHS and LBA)? QEmu guesses geometry by default, as does linux' fdisk/parted, and you will probably have supplied or generated geometry for the harddisk image as well. If there's any mismatch, it will appear that CHS offsets are wrong, whereas LBA isn't.

i don't have support for IDENTIFY or LBA yet, as my target platform is basically really old computers like 286 and lower. i have an ancient canon notejet with a 486 and 125 MB hard drive though, it has the same problem on there and it definitely gets the CHS correctly.

i've been designing my drive I/O functions to work with LBA for their parameters, since i do want to add LBA support at some point. for now it just does conversion from that to CHS.

[Brendan]

Hi,

here's the relevant code. go easy on me, i'm relatively new to C. i'm a BASIC veteran.

Heh. The more mistakes that someone mentions the better. Each mistake that nobody mentions could be a mistake that you make repeatedly for years.

Here's my list of suggestions:

1) "readparts(uint8 disknum)" reads the entire MBR into "uint8 mbr[512]" and only uses 2 bytes of this. Then it reads the each partition table entry from disk instead of using the data it's already got in "uint8 mbr[512]". You could/should avoid the overhead of reading from disk (or disk cache) 4 times too many.

2) The "0x55, 0xAA" signature tells the BIOS if the MBR is bootable or not (and doesn't tell you if the MBR contains a partition table or not). If someone has a second hard drive that is partitioned but isn't bootable, then this "0x55, 0xAA" shouldn't be present. Some well known OSs (e.g. Linux) get this wrong and make unbootable data disks "bootable", which means if the primary hard disk fails (or is removed) the BIOS attempts to boot from the unbootable data disk and crashes (rather than displaying some sort of "Can't find a bootable disk" error message).

3) The compiler is free to do whatever it likes with your structures, including inserting padding between fields. Because there's no guarantee that the layout of the structure in memory will match what you expect, you probably shouldn't be using a structure. The most robust way of doing it is to use an array of bytes (which you've already got). For example "volmap[lastvol].sects = mbr[entry_offset+0x0C] || (mbr[entry_offset+0x0C+1] << 8 ) || (mbr[entry_offset+0x0C+2] << 16) || (mbr[entry_offset+0x0C+3] << 24);". As a bonus, this method also works perfectly on both little-endian and big-endian machines.

4) To work around limitations in the original "int 0x13" BIOS disk interface, there's several difference schemes used by BIOSs to translate the disk's actual geometry into something else (here's a web page with good information about the translation schemes). For historical reasons (e.g. DOS compatability) and for practical reasons (e.g. the partition table is often used in conjuction with BIOS services by boot code), the CHS values in the partition table must use the fake/translated geometry of the drive (as reported by the BIOS) and not the actual geometry of the drive (as reported by the disk drive's "IDENTIFY" command). Because a lot of modern OSs are poo they don't record the geometry of the drive/s as reported by the BIOS during boot, and after these OSs have started (and discarded the BIOS) they have no way to reliably tell what geometry the partition table should use. There's also problems when a drive that was correctly partitioned on one computer (with one CHS translation scheme) is shifted to a different computer (with a different CHS translation scheme). Finally, the CHS values in the partition table are unable to handle disks that are larger than 8 GiB. For all of these reasons, the CHS values in the partition table should never be relied on unless there's absolutely no choice. Instead, use LBA addressing (and the LBAstart and sectorcount fields in partition table entries), and use the actual geometry of the drive to convert LBA back into CHS if the drive doesn't support LBA.

5) I've got no idea where the geometry you're using (e.g. "(uint32)disk[disknum].spt") comes from. You've said you don't support the "IDENTIFY" command (to get the actual geometry from the disk), so I can only assume the geometry comes from the BIOS (e.g. the "Get Drive Parameters" and/or the "Int 0x13 Extensions - Get Drive Parameters" BIOS functions). I also have no idea how you're accessing the disk (using BIOS functions, or with your own driver). It is possible that you're mixing different CHS schemes (e.g. using the BIOS's fake/translated CHS with your own driver that doesn't do the translation) and reading the wrong sectors.

6) The formula you're using to convert CHS into LBA looks right (although it could be expressed as (cylinder * totalHeads + head) * sectorsPerTrack + sector rather than cylinder * totalHeads * sectorsPerTrack + head * sectorsPerTrack + sector). It's mostly a waste of time though, as you could/should be doing "volmap[lastvol].startsect = tmppart.lbastart; volmap[lastvol].endsect = volmap[lastvol].startsect + tmppart.sectorcount;" instead.

7) I don't understand why you'd call anything in a FAT driver module before you've determined if the partition is FAT (or not). Eventually you'll need something to determine what to do with each partition (if anything) - for e.g. "/etc/fstab" in Linux.

8 ) There's a buffer overflow bug - if someone happens to have more than 256 partitions you'll attempt to write past the end of the "volmap[0x100]" array. You should do some sort of "if(lastvol >= MAX_VOLS) " somewhere to protect against this.

9) I don't know how you're intending to use "volmap[].drivepartnum" (your code never sets it). Eventually, you'll want to support extended partitions, and maybe "extended extended" partitions and things like BSD slices (and maybe GPT too). I don't think there's a way of using "volmap[].drivepartnum" that is able to adequately handle all possible cases. Mostly you've got a large chunk of data (e.g. a disk in this case, but it could just as easily be a file or disk image) that may be recursively subdivided in a variety of different ways into one or more smaller chunks of data. Partitioning (and extended partitions) is one way of of subdividing these chunks of data. If you think about it, a file system is just another (more complex) way of subdividing chunks of data. As an example, you might want a name and/or identifier for each chunk of data, plus a reference to that chunk's parent and something to keep track of how the parent was subdivided. For example, you might build a tree of entries like this:

Code: Select all

<"/cdrom/hello.txt> has parent <CD_ROM> which is subdivided by <ISO9660 file system code>
<"/cdrom/disk.img"> has parent <CD_ROM> which is subdivided by <ISO9660 file system code>
    <partition 32> has parent <"/cdrom/disk.img"> which is subdivided by <primary partition code>
    <partition 33> has parent <"/cdrom/disk.img"> which is subdivided by <primary partition code>
        <partition 123> has parent <partition 33> which is subdivided by <extended partition code>
            <"/fat/foo.txt"> has parent <partition 123> which is subdivided by <FAT file system code>
            <"/fat/bar.txt"> has parent <partition 123> which is subdivided by <FAT file system code>
    <partition 39> has parent <"/cdrom/disk.img"> which is subdivided by <primary partition code>
        <"/work/mystuff.zip"> has parent <partition 39> which is subdivided by <ext3 file system code>
        <"/work/mystuff.zip/myPetAlligator.jpg"> has parent <"/work/mystuff.zip"> which is subdivided by <ZIP archiver>
        <"/work/mystuff.zip/myPetChicken_before.jpg"> has parent <"/work/mystuff.zip"> which is subdivided by <ZIP archiver>
        <"/work/mystuff.zip/myPetChicken_after.jpg"> has parent <"/work/mystuff.zip"> which is subdivided by <ZIP archiver>

Most OSs implement something like this in a "virtual file system" layer, where larger chunks (e.g. disks) are represented as files and files are "mounted" by the code that subdivides it into more files (e.g. for the example above, the FAT file system code might mount <partition 123> at "/fat" and make the file "/fat/foo.txt" available).


Cheers,

Brendan

[miker00lz]

thanks for the tips. i'm aware of much of that, like the BIOS possibly using different geometry than what the drive physically has but i am using int 13h to do reads, and that's also what i use to get the drive parameters.. it also does check partition type before calling initvol. you can see it checks tmppart.parttype to be either 4 (16-bit FAT up to 32 MB) or 6 (16-bit FAT over 32 MB) but now that you mentioned it, i forgot to have it consider value of 1 to be valid as well, which is FAT12.

as far as reading the structure separately after already reading the MBR, your suggestion of just using the already-retrieved data is actually how i planned to eventually do it, but decided to do the separate structure at the time for easier reading while debugging the code. i will change that now though and see if it helps. i thought a C compiler would always make a structure contiginous in memory. thanks for the info there. now that i think about it, that makes sense as some architectures perform better with data is word aligned so that would be one reason for padding. im thinking this very well might be the source of my problem. ill let you know what happens when i change it.

for the record i'm using the Borland Turbo C++ 3.0 compiler (although my code for this is of course C, not C++)

this whole readparts routine i've posted here is called by the drive enumeration function after time it successfully determines a drive exists and gets it's parameters.

i didn't notice that i neglected to set drivepartnum, thanks for catching that! i will add the check for going over the max partition number too. that's a bug i was aware of but wasn't overly concerned for the time being. the routine's not quite finished but i wanted to figure out what's causing my problems so far.

once i've got basic partition functionality working, i am definitely adding support for extended parts. i'm not going to touch subpartitions though, and for the moment i don't plan to support anything but FAT. i'm not trying to create anything too full-fledged with this OS. it supports (and has working) multi-tasking though. basically the end goal is a sort of multi-tasking DOS with a sprinkling of watered-down POSIX-ness. eventually, i'm going to integrate drivers for some ethernet cards and a TCP/IP module.

[miker00lz]

Start with checking all parameters: What's the CHS geometry of the image? What geometry does IDENTIFY return? What's the starting and ending LBA of the partition, and what is the starting and ending LBA in the partition table, and do both fit within the reported geometry (both CHS and LBA)? QEmu guesses geometry by default, as does linux' fdisk/parted, and you will probably have supplied or generated geometry for the harddisk image as well. If there's any mismatch, it will appear that CHS offsets are wrong, whereas LBA isn't.

it does check the geometry of a drive before this routine is called as i pointed out in the previous post up there, and you can see in my code that it does determine the starting and ending LBA by doing some math with the CHS start and end points. i am not using the LBA starting point and size in sectors which you nowadays find in a partition entry, since i'm mainly targetting very old computers with this. didn't think they filled in those values because they have no LBA support, or do they fill it in anyway?

[miker00lz]

alright, getting rid of the struct didn't change anything but combuster was right on, i had to use the LBA starting value in the partition table entry, rather than calculating it from the CHS numbers. THANKS!