3
\$\begingroup\$

This is a function that checks if a valid FAT12 image would be created. Since all the information to create it is available on the "boot sector", this is the parameter. I wanted the code to be pretty readable, but I may have overdone it.

fat.h

/* This file defines some FAT structures, and functions for validating a FAT12
 * objects. */

#ifndef YMFAT12_FAT_H
#define YMFAT12_FAT_H

#include <stdio.h>

/* The names are similar to the specs. */

/* BIOS parameter block (BPB) */
struct fat_bpb {
    unsigned short      bytsPerSec;
    unsigned char       secPerClus;
    unsigned short      rsvdSecCnt;
    unsigned char       numFATs;
    unsigned short      rootEntCnt;
    unsigned short      totSec16;
    unsigned char       media;
    unsigned short      FATSz16;
    unsigned short      secPerTrk;
    unsigned short      numHeads;
    unsigned long       hiddSec;
    unsigned long       totSec32;
};

/* Extended BPB (EBPB) */
struct fat_ebpb {
    unsigned char       drvNum;
    unsigned char       reserved1;
    unsigned char       bootSig;
    unsigned char       volID;
    char                volLab[11];
    char                filSysType[8];
};

struct fat_bootsector {
    unsigned char       jmpBoot[3];
    char                oemName[8];
    struct fat_bpb      bpb;
    struct fat_ebpb     ebpb;
    unsigned char       code[448];
    unsigned char       signature_word[2];
};

/* Returns whether the bootsector information would make a valid FAT12 image.
 * @param       [in]    bs      The bootsector with the sizes of the FAT12
 *                              objects, and volume information.
 * @return                      Whether the FAT12 image would be valid.
 */
bool check_bs(struct fat_bootsector *bs);

#endif /* YMFAT12_FAT_H */

fat.c

/* Implements what is declared on "fat.h".
 *
 * The "robustness principle" is generally used here:
 *      >> Be conservative in what you send, be liberal in what you accept
 * It is mostly broken when compatibility with MS-DOS would restrict the specs.
 *
 * The specification used was from "August 30 2005" on
 * "https://academy.cba.mit.edu/classes/networking_communications/SD/FAT.pdf"
 * (accessed 2024/01/28)
 * When Wikipedia is mentioned the following article is being mentioned:
 * "https://en.wikipedia.org/wiki/Design_of_the_FAT_file_system" (accessed
 * 2024/01/28).
 * When OSDev is mentioned, the following article is being mentioned:
 * https://wiki.osdev.org/FAT (accessed 2024/01/28).
 *
 * Since values besides `char` can vary in length, the value size are checked
 * to be n-bytes long. This does assume 1 byte is 8-bits long, which is wild.
 */

#include "fat.h"

#include <string.h>
#include <limits.h>
#include "error.h"
#include "common.h"

/* For the specs, a FAT is 12, 16 or 32 according to its number of clusters.
 * Different specs define different limits. The used spec defines 4084 for
 * FAT12, as mkfs.dos and Linux driver. The following limit was decided for the
 * following reasons.
 * 1. Cluster _values_ 0 and 1 are reserved, so 4096-2=4094 would be the limit.
 * 2. For Wikipedia, cluster values 0xFF0-0xFF5 are reserved or used, so
 * for portability the limit would be 4094-6=4088.
 * 3. For Wikipedia, cluster value 0xFF6 is reserved. Then the limit would be
 * 4088-1 = 4087.
 * 4. Cluster values 0xFF7-0xFFF have special meaning, so the limit would be
 * 4087-9=4078.
 * Q.E.D.
 */
#define FAT12_MAX_CLUSTERS 4078

static bool overflows3_add_ul(
        unsigned long n1,
        unsigned long n2,
        unsigned long n3
)
{
        if (n1 + n2 < n1)
                return false;
        return n1 + n2 + n3 < n1 + n2;
}

static inline bool overflows_mult_ul(unsigned long n1, unsigned long n2)
{
        return n2 > ULONG_MAX / n1;
}

static inline bool is_16bit(unsigned short n)
{
        return (n & 0xFFFF) == n;
}

static inline bool is_32bit(unsigned long n)
{
        return (n & 0xFFFFFFFF) == n;
}

static inline bool is_lowercase_ascii(char n)
{
        return n >= 0x61 && n <= 0x7A;
}

/* Whether is a valid 8.3 name */
static bool is_83name(const char *n)
{
        const size_t sz = 11;
        size_t i;
        if (n[0] == 0x20)
                return false;
        for (i = 0; i < sz; i++) {
                if (is_lowercase_ascii(n[i]))
                        return false;
                if (n[i] < 0x20)
                        return false;
                if (    n[i] == 0x22 || n[i] == 0x2A || n[i] == 0x2B
                ||      n[i] == 0x2C || n[i] == 0x2E || n[i] == 0x2F
                ||      n[i] == 0x3A || n[i] == 0x3B || n[i] == 0x3C
                ||      n[i] == 0x3D || n[i] == 0x3E || n[i] == 0x3F
                ||      n[i] == 0x5B || n[i] == 0x5C || n[i] == 0x5D
                ||      n[i] == 0x7C) {
                        return false;
                }
        }
        return true;
}

/* Checks if each field is within specs. */
static bool check_bs_fields(struct fat_bootsector *bs)
{
        /* The function is long, but breaking it would not help, so I added
         * marks for what was being checked. */

        /* jmpBoot */
        /* For OSDev, Windows and OS X do not recognize the volume if the
         * bootjump does not follow the standard. */
        if (bs->jmpBoot[0] == 0xEB) {
                if (bs->jmpBoot[2] != 0x90)
                        return false;
        } else if (bs->jmpBoot[0] != 0xE9)
                return false;

        /* oemName does not need checking. It can be whatever. */

        /* bytsPerSec */
        if (    bs->bpb.bytsPerSec != 512
        &&      bs->bpb.bytsPerSec != 1024
        &&      bs->bpb.bytsPerSec != 2048
        &&      bs->bpb.bytsPerSec != 4096) {
                return false;
        }

        /* secPerClus */
        if (    bs->bpb.secPerClus != 1
        &&      bs->bpb.secPerClus != 2
        &&      bs->bpb.secPerClus != 4
        &&      bs->bpb.secPerClus != 8
        &&      bs->bpb.secPerClus != 16
        &&      bs->bpb.secPerClus != 32
        &&      bs->bpb.secPerClus != 64
        &&      bs->bpb.secPerClus != 128) {
                return false;
        }

        /* rsvdSecCnt */
        if (!is_16bit(bs->bpb.rsvdSecCnt))
                return false;
        if (bs->bpb.rsvdSecCnt == 0)
                return false;

        /* numFATs */
        /* Maybe only 1 and 2 values should be allowed. */
        if (!is_16bit(bs->bpb.numFATs))
                return false;
        if (bs->bpb.numFATs == 0)
                return false;

        /* rootEntCnt */
        if (!is_16bit(bs->bpb.rootEntCnt))
                return false;
        /* Specs does not exclude 0, but it is natural. */
        if (bs->bpb.rootEntCnt == 0)
                return false;
        if ((long)bs->bpb.rootEntCnt * 32 % bs->bpb.bytsPerSec != 0)
                return false;

        /* totSec16 */
        /* If it is 0, then totSec32 is used instead. */
        if (!is_16bit(bs->bpb.totSec16))
                return false;

        /* media */
        /* Wikipedia asserts there is more. */
        if (            bs->bpb.media != 0xF0
                &&      bs->bpb.media != 0xF8
                &&      bs->bpb.media != 0xF9
                &&      bs->bpb.media != 0xFA
                &&      bs->bpb.media != 0xFB
                &&      bs->bpb.media != 0xFC
                &&      bs->bpb.media != 0xFD
                &&      bs->bpb.media != 0xFE
                &&      bs->bpb.media != 0xFF) {
                return false;
        }

        /* FATSz16 */
        if (!is_16bit(bs->bpb.media))
                return false;
        /* Specs does not exclude 0, but it is natural. */
        if (bs->bpb.FATSz16 == 0)
                return false;

        /* secPerTrk */
        if (!is_16bit(bs->bpb.secPerTrk))
                return false;

        /* numHeads */
        if (!is_16bit(bs->bpb.numHeads))
                return false;
        /* Wikipedia says MS-DOS had a bug for CHS with 256 heads up to
         * including 7.1. So most BIOSes do not support more than 255 heads. */
        if (bs->bpb.numHeads >= 256)
                return false;

        /* hiddSec */
        if (!is_16bit(bs->bpb.hiddSec))
                return false;

        /* totSec32 */
        if (!is_32bit(bs->bpb.totSec32))
                return false;
        if (bs->bpb.totSec32 == 0 && bs->bpb.totSec16 == 0)
                return false;

        /* drvNum */
        if (bs->ebpb.drvNum != 0x00 && bs->ebpb.drvNum != 0x80)
                return false;

        /* reserved1 */
        if (bs->ebpb.reserved1 != 0)
                return false;

        /* bootSig */
        /* For Wikipedia, it can also be 0x28, in which case the following
         * fields are not used. */
        if (bs->ebpb.bootSig != 0x29)
                return false;

        /* volID does not need checking. */

        /* volLab */
        /* The specs do not affirm its format, but the examples align with
         * 8.3 names, and that it should be same as the volume file. */
        if (!is_83name(bs->ebpb.volLab))
                return false;

        /* filSysType */
        /* Since it is informational, it could be allowed to be whatever. */
        if (memcmp(bs->ebpb.filSysType, "FAT12   ", 8) != 0)
                return false;

        /* signature_word */
        /* For Wikipedia, MS-DOS <3.3 checked if it was 0x55 0xAA. */
        if (bs->signature_word[0] != 0x55 || bs->signature_word[1] != 0xAA)
                return false;

        return true;
}

bool check_bs(struct fat_bootsector *bs)
{
        unsigned long fatTot, res, fat, rDir, data;
        if (!check_bs_fields(bs))
                return false;

        /* Some ugly overflow checks were needed. */
        fatTot = bs->bpb.totSec16 == 0  ? bs->bpb.totSec32
                                        : bs->bpb.totSec16;
        res = (unsigned long)bs->bpb.rsvdSecCnt * bs->bpb.bytsPerSec;
        fat = (unsigned long)bs->bpb.numFATs * bs->bpb.FATSz16;
        /* On section 4 of the spec, "The size of the FAT is limited to 6K
         * sectors on volumes formatted FAT12". */
        if (fat > 6 * 1028)
                return false;
        if (overflows_mult_ul(fat, bs->bpb.bytsPerSec))
                return false;
        fat *= bs->bpb.bytsPerSec;
        rDir = (unsigned long)bs->bpb.rootEntCnt * 32;
        if (res + fat + rDir > fatTot)
                return false;
        if (overflows3_add_ul(res, fat, rDir))
                return false;
        data = fatTot - (res + fat + rDir);

        if (data > FAT12_MAX_CLUSTERS)
                return false;

        return true;
}
\$\endgroup\$
0

1 Answer 1

3
\$\begingroup\$

fat.h has no need to #include <stdio.h>, so remove that.

However, it does require <stdbool.h>.


I believe that the on-disk structures have specific sizes, rather than depending on the platform's char, short and long. If I'm right, we should be using fixed-width types from <stdint.h>. We might also need to take measures to avoid being tripped up by structure padding.


Instead of using preprocessor #define FAT12_MAX_CLUSTERS 4078, prefer to use an integer constant.


I don't like the dependence of overflows3_add_ul on unsigned overflow, even though that's well-defined. It would be more transparent if it tested against ULONG_MAX as its multiplicative sister does:

    return n2 > ULONG_MAX - n1
        || n3 > ULONG_MAX - n2 - n1;

The is_lowercase_ascii() test might need a comment to say why we're using numeric values for the comparison (i.e. to ensure that the code is still correct when run in a non-ASCII environment) and prevent a future maintainer erroneously "correcting" to 'a' and 'z'.


I'm not convinced of the correctness of is_83name(), having read the description of directory table. I think it's better to whitelist the allowed characters, rather than attempting to enumerate what's not allowed. And we do we need to handle 0x05/0xE5 in the first position?


Testing for exact powers of two might be easier with the common x & (x-1) bit-hack, plus a simple range test.


is_16bit(bs->bpb.hiddSec) truncates the unsigned long to unsigned short - we need to widen the function's formal arguments, or perform an additional test before truncating with a cast.


There's a whole series of tests for non-zero 16-bit values: we might be better off implementing bool is_in_range(unsigned long value, unsigned long min, unsigned long max) and using that throughout instead. A reasonable optimiser knows that x & 0xffff != x and x <= 0xffff are equivalent.


We have redundancy here:

    if (!is_16bit(bs->bpb.numHeads))
            return false;
    if (bs->bpb.numHeads >= 256)
            return false;

The first test can be eliminated.


I don't like the cast from unsigned to signed here:

    if ((long)bs->bpb.rootEntCnt * 32 % bs->bpb.bytsPerSec != 0)

unsigned long would be a better choice.


I don't particularly like the magic-number length here:

     if (memcmp(bs->ebpb.filSysType, "FAT12   ", 8) != 0)

Consider using sizeof bs->ebpb.filSysType in place of 8 there for clarity.

\$\endgroup\$
1
  • \$\begingroup\$ Thank you, Toby! I really liked the idea for a rage function. I used that definition on is_83name() because the specs define that way, but I should prohibit ' ' anyway, since programs did not expect spaces. And I had not thought of that about is_lowercase_ascii(), though I have never experienced a non-ASCII environment. \$\endgroup\$
    – Schilive
    Feb 1 at 17:42

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.