Harmful casts in ufs

[Martin Natano]

Casting the result of ext2fs_size() and DIP(ip, size) to int potentially
truncates the result. Issue found by Stefan Kempf, see
https://marc.info/?l=openbsd-tech&m=145495905416536 .

While there I also removed the cast in the ext2fs_chmod() call, because
the function expects a mode_t argument anyway.

Comments?

natano


Index: ufs//ext2fs/ext2fs_readwrite.c
===================================================================
RCS file: /cvs/src/sys/ufs/ext2fs/ext2fs_readwrite.c,v
retrieving revision 1.37
diff -u -p -u -r1.37 ext2fs_readwrite.c
--- ufs//ext2fs/ext2fs_readwrite.c 16 Feb 2016 17:56:12 -0000 1.37
+++ ufs//ext2fs/ext2fs_readwrite.c 17 Feb 2016 09:13:59 -0000
@@ -95,7 +95,7 @@ ext2_ind_read(struct vnode *vp, struct i
panic("%s: mode", "ext2fs_read");

if (vp->v_type == VLNK) {
- if ((int)ext2fs_size(ip) < vp->v_mount->mnt_maxsymlinklen ||
+ if (ext2fs_size(ip) < vp->v_mount->mnt_maxsymlinklen ||
(vp->v_mount->mnt_maxsymlinklen == 0 &&
ip->i_e2fs_nblock == 0))
panic("%s: short symlink", "ext2fs_read");
Index: ufs//ext2fs/ext2fs_vnops.c
===================================================================
RCS file: /cvs/src/sys/ufs/ext2fs/ext2fs_vnops.c,v
retrieving revision 1.74
diff -u -p -u -r1.74 ext2fs_vnops.c
--- ufs//ext2fs/ext2fs_vnops.c 16 Feb 2016 17:56:12 -0000 1.74
+++ ufs//ext2fs/ext2fs_vnops.c 17 Feb 2016 09:13:59 -0000
@@ -322,7 +322,7 @@ ext2fs_setattr(void *v)
if (vap->va_mode != (mode_t)VNOVAL) {
if (vp->v_mount->mnt_flag & MNT_RDONLY)
return (EROFS);
- error = ext2fs_chmod(vp, (int)vap->va_mode, cred, p);
+ error = ext2fs_chmod(vp, vap->va_mode, cred, p);
}
return (error);
}
Index: ufs//ffs/ffs_vnops.c
===================================================================
RCS file: /cvs/src/sys/ufs/ffs/ffs_vnops.c,v
retrieving revision 1.82
diff -u -p -u -r1.82 ffs_vnops.c
--- ufs//ffs/ffs_vnops.c 16 Feb 2016 17:56:12 -0000 1.82
+++ ufs//ffs/ffs_vnops.c 17 Feb 2016 09:13:59 -0000
@@ -207,7 +207,7 @@ ffs_read(void *v)
panic("ffs_read: mode");

if (vp->v_type == VLNK) {
- if ((int)DIP(ip, size) < vp->v_mount->mnt_maxsymlinklen ||
+ if (DIP(ip, size) < vp->v_mount->mnt_maxsymlinklen ||
(vp->v_mount->mnt_maxsymlinklen == 0 &&
DIP(ip, blocks) == 0))
panic("ffs_read: short symlink");

[Todd C. Miller]

Post by Martin Natano
Casting the result of ext2fs_size() and DIP(ip, size) to int potentially
truncates the result. Issue found by Stefan Kempf, see
https://marc.info/?l=openbsd-tech&m=145495905416536 .
While there I also removed the cast in the ext2fs_chmod() call, because
the function expects a mode_t argument anyway.

There is currently code that checks for mnt_maxsymlinklen <= 0.
Removing the cast will cause other problems for ffs if the maxsymlinklen
value is negative. I don't think it is safe to make this change
unless mnt_maxsymlinklen is made unsigned in struct mount and a
check is added to the assignment of mnt_maxsymlinklen from
fs_maxsymlinklen in ufs/ffs/ffs_vfsops.c to avoid assigning a
negative value.

- todd

[Martin Natano]

Post by Todd C. Miller

Post by Martin Natano
Casting the result of ext2fs_size() and DIP(ip, size) to int potentially
truncates the result. Issue found by Stefan Kempf, see
https://marc.info/?l=openbsd-tech&m=145495905416536 .
While there I also removed the cast in the ext2fs_chmod() call, because
the function expects a mode_t argument anyway.

There is currently code that checks for mnt_maxsymlinklen <= 0.
Removing the cast will cause other problems for ffs if the maxsymlinklen
value is negative. I don't think it is safe to make this change
unless mnt_maxsymlinklen is made unsigned in struct mount and a
check is added to the assignment of mnt_maxsymlinklen from
fs_maxsymlinklen in ufs/ffs/ffs_vfsops.c to avoid assigning a
negative value.

Thank you for your input, I somehow missed that mnt_maxsymlinklen might
be negative. This will need more work.

natano

[Martin Natano]

Post by Todd C. Miller
There is currently code that checks for mnt_maxsymlinklen <= 0.
Removing the cast will cause other problems for ffs if the maxsymlinklen
value is negative. I don't think it is safe to make this change
unless mnt_maxsymlinklen is made unsigned in struct mount and a
check is added to the assignment of mnt_maxsymlinklen from
fs_maxsymlinklen in ufs/ffs/ffs_vfsops.c to avoid assigning a
negative value.

The diff below addresses the issues you mentioned. It converts
mnt_maxsymlinklen to unsigned and adds a check to ffs_validate() that
makes sure, that fs_maxsymlinklen is >= 0. That function is called
during mount and on fsck. This should make sure we won't get a bogus
fs_maxsymlinklen from the superblock.

natano


Index: sys/mount.h
===================================================================
RCS file: /cvs/src/sys/sys/mount.h,v
retrieving revision 1.121
diff -u -p -r1.121 mount.h
--- sys/mount.h 8 Sep 2014 01:47:06 -0000 1.121
+++ sys/mount.h 21 Feb 2016 00:44:11 -0000
@@ -355,7 +355,7 @@ struct mount {
struct vnodelst mnt_vnodelist; /* list of vnodes this mount */
struct rwlock mnt_lock; /* mount structure lock */
int mnt_flag; /* flags */
- int mnt_maxsymlinklen; /* max size of short symlink */
+ unsigned int mnt_maxsymlinklen; /* max size of short symlink */
struct statfs mnt_stat; /* cache of filesystem stats */
void *mnt_data; /* private data */
};
Index: ufs/ext2fs/ext2fs_readwrite.c
===================================================================
RCS file: /cvs/src/sys/ufs/ext2fs/ext2fs_readwrite.c,v
retrieving revision 1.37
diff -u -p -r1.37 ext2fs_readwrite.c
--- ufs/ext2fs/ext2fs_readwrite.c 16 Feb 2016 17:56:12 -0000 1.37
+++ ufs/ext2fs/ext2fs_readwrite.c 21 Feb 2016 00:44:11 -0000
@@ -95,7 +95,7 @@ ext2_ind_read(struct vnode *vp, struct i
panic("%s: mode", "ext2fs_read");

if (vp->v_type == VLNK) {
- if ((int)ext2fs_size(ip) < vp->v_mount->mnt_maxsymlinklen ||
+ if (ext2fs_size(ip) < vp->v_mount->mnt_maxsymlinklen ||
(vp->v_mount->mnt_maxsymlinklen == 0 &&
ip->i_e2fs_nblock == 0))
panic("%s: short symlink", "ext2fs_read");
Index: ufs/ext2fs/ext2fs_vnops.c
===================================================================
RCS file: /cvs/src/sys/ufs/ext2fs/ext2fs_vnops.c,v
retrieving revision 1.74
diff -u -p -r1.74 ext2fs_vnops.c
--- ufs/ext2fs/ext2fs_vnops.c 16 Feb 2016 17:56:12 -0000 1.74
+++ ufs/ext2fs/ext2fs_vnops.c 21 Feb 2016 00:44:11 -0000
@@ -322,7 +322,7 @@ ext2fs_setattr(void *v)
if (vap->va_mode != (mode_t)VNOVAL) {
if (vp->v_mount->mnt_flag & MNT_RDONLY)
return (EROFS);
- error = ext2fs_chmod(vp, (int)vap->va_mode, cred, p);
+ error = ext2fs_chmod(vp, vap->va_mode, cred, p);
}
return (error);
}
Index: ufs/ffs/ffs_vfsops.c
===================================================================
RCS file: /cvs/src/sys/ufs/ffs/ffs_vfsops.c,v
retrieving revision 1.150
diff -u -p -r1.150 ffs_vfsops.c
--- ufs/ffs/ffs_vfsops.c 12 Jan 2016 11:41:00 -0000 1.150
+++ ufs/ffs/ffs_vfsops.c 21 Feb 2016 00:44:11 -0000
@@ -642,6 +642,9 @@ ffs_validate(struct fs *fsp)
if ((u_int)fsp->fs_frag > MAXFRAG || fragtbl[fsp->fs_frag] == NULL)
return (0); /* Invalid number of fragments */

+ if (fsp->fs_maxsymlinklen < 0)
+ return (0); /* Invalid max size of short symlink */
+
return (1); /* Super block is okay */
}

Index: ufs/ffs/ffs_vnops.c
===================================================================
RCS file: /cvs/src/sys/ufs/ffs/ffs_vnops.c,v
retrieving revision 1.82
diff -u -p -r1.82 ffs_vnops.c
--- ufs/ffs/ffs_vnops.c 16 Feb 2016 17:56:12 -0000 1.82
+++ ufs/ffs/ffs_vnops.c 21 Feb 2016 00:44:11 -0000
@@ -207,7 +207,7 @@ ffs_read(void *v)
panic("ffs_read: mode");

if (vp->v_type == VLNK) {
- if ((int)DIP(ip, size) < vp->v_mount->mnt_maxsymlinklen ||
+ if (DIP(ip, size) < vp->v_mount->mnt_maxsymlinklen ||
(vp->v_mount->mnt_maxsymlinklen == 0 &&
DIP(ip, blocks) == 0))
panic("ffs_read: short symlink");

[Stefan Kempf]

Post by Martin Natano
The diff below addresses the issues you mentioned. It converts
mnt_maxsymlinklen to unsigned and adds a check to ffs_validate() that
makes sure, that fs_maxsymlinklen is >= 0. That function is called
during mount and on fsck. This should make sure we won't get a bogus
fs_maxsymlinklen from the superblock.

I think it is better to just set fsp->fs_maxsymlinklen to 0 if it
is negative in the superblock. We shouldn't fail to mount the
filesystem in this case.

newfs back to 4.4BSD sets fs_maxsymlinklen to 0 for the "old" format.
A negative value should never happen unless this superblock field is
corrupted.

Should we really mount the FS in that case? If the FS was of the
"new" format, then short symlinks would store the destination path in the
inode directly. I think we'd not be able to correctly follow these symlinks
if we set fs_maxsymlinklen to 0 when encountering a negative value.

- todd

[Stefan Kempf]

Post by Todd C. Miller

Post by Martin Natano
Casting the result of ext2fs_size() and DIP(ip, size) to int potentially
truncates the result. Issue found by Stefan Kempf, see
https://marc.info/?l=openbsd-tech&m=145495905416536 .
While there I also removed the cast in the ext2fs_chmod() call, because
the function expects a mode_t argument anyway.

There is currently code that checks for mnt_maxsymlinklen <= 0.
Removing the cast will cause other problems for ffs if the maxsymlinklen
value is negative. I don't think it is safe to make this change
unless mnt_maxsymlinklen is made unsigned in struct mount and a
check is added to the assignment of mnt_maxsymlinklen from
fs_maxsymlinklen in ufs/ffs/ffs_vfsops.c to avoid assigning a
negative value.

That makes sense. Those <= 0 checks look whether the FFS is in the
"old" format. When creating an old format FFS, newfs creates a superblock
with fs_maxsymlinklen of 0. A negative fs_maxsymlinklen should never
happen except for bogus superblocks. So checking for this when mounting
the filesystem looks reasonable.

Post by Todd C. Miller
- todd