Hi,
our guard pages just told me that writefat is vulnerable to an off by one
for FAT12 filesystems. They shouldn't be that common anymore, but better
be safe than sorry.
The diff looks confusing at first, but this default-block is just split
into two cluster blocks, as it's done in readfat (line 176).
This diff has an effect on variable p, but from my point of view, it's no
issue because it's not used after for-loop anymore -- and the for-loop
will stop if "cl + 1 < boot->NumClusters".
Enough talk, here's how to trigger it (i386, 1M iso triggers it for me):
Create a 1M filesystem:
# dd if=/dev/zero of=poc.iso bs=1M count=1
# vnconfig vnd0c poc.iso
# newfs_msdos vnd0c
Break FAT and try to repair
# dd if=/dev/zero of=poc.iso conv=notrunc bs=1 count=1 seek=512
# fsck_msdos vnd0c
Output:
** /dev/rvnd0c (vnd0c)
** Phase 1 - Read and Compare FATs
FAT starts with odd byte sequence (00ffff)
Correct? [Fyn] y
** Phase 2 - Check Cluster Chains
Segmentation fault (core dumped)
# _
Tobias
Index: fat.c
===================================================================
RCS file: /cvs/src/sbin/fsck_msdos/fat.c,v
retrieving revision 1.19
diff -u -p -r1.19 fat.c
--- fat.c 9 Jun 2014 09:13:33 -0000 1.19
+++ fat.c 9 Jun 2014 19:06:26 -0000
@@ -471,13 +471,15 @@ writefat(int fs, struct bootblock *boot,
default:
if (fat[cl].next == CLUST_FREE)
boot->NumFree++;
- if (cl + 1 < boot->NumClusters
- && fat[cl + 1].next == CLUST_FREE)
- boot->NumFree++;
*p++ = (u_char)fat[cl].next;
- *p++ = (u_char)((fat[cl].next >> 8) & 0xf)
- |(u_char)(fat[cl+1].next << 4);
- *p++ = (u_char)(fat[++cl].next >> 4);
+ *p++ = (u_char)((fat[cl].next >> 8) & 0xf);
+ cl++;
+ if (cl >= boot->NumClusters)
+ break;
+ if (fat[cl].next == CLUST_FREE)
+ boot->NumFree++;
+ *p |= (u_char)(fat[cl + 1].next << 4);
+ *p++ = (u_char)(fat[cl + 1].next >> 4);
break;
}
}
