Hi,
sometimes I see writing to msdosfs fail with EINVAL. This seems to occur
when writing a file reaches the end of the partition. But it only happens
if all bits in the pm_inusemap array are actually used, i.e. if
pm_maxcluster % 32 == 31 . This means that for the number N of data
clusters in the partition the condition N % 32 == 30 is true, as the first
two cluster numbers have special meaning and do not represent data
clusters on disk. For partition sizes that do not fulfill this condition,
there is always a bit set at the end of the pm_inusemap that prevents
chainlength() from overrunning the size of the partition.
Reproducer:
# dd if=/dev/zero bs=1k seek=20019 of=msdos.img count=1
1+0 records in
1+0 records out
1024 bytes transferred in 0.000 secs (52258229 bytes/sec)
# VND=$(vnconfig msdos.img ) && echo $VND
vnd0
# newfs_msdos ${VND}c
/dev/rvnd0c: 39928 sectors in 9982 FAT16 clusters (2048 bytes/cluster)
bps=512 spc=4 res=1 nft=2 rde=512 sec=40040 mid=0xf0 spf=39 spt=63 hds=1 hid=0
# mount_msdos /dev/${VND}c /mnt
# dd if=/dev/zero bs=1k of=/mnt/fff
dd: /mnt/fff: Invalid argument
12829+0 records in
12828+0 records out
13135872 bytes transferred in 1.884 secs (6969515 bytes/sec)
# df -h /mnt
Filesystem Size Used Avail Capacity Mounted on
/dev/vnd0c 19.5M 12.5M 7.0M 65% /mnt
# umount /mnt/
# vnconfig -u ${VND}
There is this fix in FreeBSD:
commit 097a1d5fbb7990980f8f806c6878537c964adf32
Author: kib <k...@freebsd.org>
Date: Fri Oct 28 11:34:32 2016 +0000
Ensure that cluster allocations never allocate clusters outside the
volume limits. In particular:
- Assert that usemap_alloc() and usemap_free() cluster number argument
is valid.
- In chainlength(), return 0 if cluster start is after the max cluster.
- In chainlength(), cut the calculated cluster chain length at the max
cluster.
- For true paranoia, after the pm_inusemap is calculated in
fillinusemap(), reset all bits in the array for clusters after the
max cluster, as in-use.
However, the for loop added to the end of fillinusemap() by that commit is
a no-op because a) the bits have already been set by the for loop at the
start of the function and b) the boundary conditions in the for loop mix
cluster numbers and indexes to pm_inusemap causing the loop to never be
executed.
Therefore, I omit that for loop alltogether but adapted the rest of the
FreeBSD fix to OpenBSD. This seems to fix the issue.
Are there any heavy users of msdosfs who can give this a test? Or any
comments or oks?
Cheers,
Stefan
diff --git a/sys/msdosfs/msdosfs_fat.c b/sys/msdosfs/msdosfs_fat.c
index c15b6257d43..0ab1463ea67 100644
--- a/sys/msdosfs/msdosfs_fat.c
+++ b/sys/msdosfs/msdosfs_fat.c
@@ -409,6 +409,7 @@ updatefats(struct msdosfsmount *pmp, struct buf *bp,
uint32_t fatbn)
static __inline void
usemap_alloc(struct msdosfsmount *pmp, uint32_t cn)
{
+ KASSERT(cn <= pmp->pm_maxcluster);
pmp->pm_inusemap[cn / N_INUSEBITS] |= 1 << (cn % N_INUSEBITS);
pmp->pm_freeclustercount--;
@@ -417,6 +418,7 @@ usemap_alloc(struct msdosfsmount *pmp, uint32_t cn)
static __inline void
usemap_free(struct msdosfsmount *pmp, uint32_t cn)
{
+ KASSERT(cn <= pmp->pm_maxcluster);
pmp->pm_freeclustercount++;
pmp->pm_inusemap[cn / N_INUSEBITS] &= ~(1 << (cn % N_INUSEBITS));
@@ -644,6 +646,8 @@ chainlength(struct msdosfsmount *pmp, uint32_t start,
uint32_t count)
u_int map;
uint32_t len;
+ if (start > pmp->pm_maxcluster)
+ return (0);
max_idx = pmp->pm_maxcluster / N_INUSEBITS;
idx = start / N_INUSEBITS;
start %= N_INUSEBITS;
@@ -651,11 +655,15 @@ chainlength(struct msdosfsmount *pmp, uint32_t start,
uint32_t count)
map &= ~((1 << start) - 1);
if (map) {
len = ffs(map) - 1 - start;
- return (len > count ? count : len);
+ len = MIN(len, count);
+ len = MIN(len, pmp->pm_maxcluster - start + 1);
+ return (len);
}
len = N_INUSEBITS - start;
- if (len >= count)
- return (count);
+ if (len >= count) {
+ len = MIN(count, pmp->pm_maxcluster - start + 1);
+ return (len);
+ }
while (++idx <= max_idx) {
if (len >= count)
break;
@@ -665,7 +673,9 @@ chainlength(struct msdosfsmount *pmp, uint32_t start,
uint32_t count)
}
len += N_INUSEBITS;
}
- return (len > count ? count : len);
+ len = MIN(len, count);
+ len = MIN(len, pmp->pm_maxcluster - start + 1);
+ return (len);
}
/*
