Re: [RFC][patch] filesystem: Vmufat filesystem, version 4

Al Viro
2009-04-16T11:23:26Z

On Mon, Apr 13, 2009 at 09:32:48PM +0100, Adrian McMenamin wrote:
> +struct memcard {
> + long sb_bnum;
> + long fat_bnum;
> + long fat_len;
> + long dir_bnum;
> + long dir_len;
> + long numblocks;
> +};

Eh...
a) are any of those really signed?
b) can any of those be more than 32 bits?

> +struct vmufat_block_list {
> + struct list_head b_list;
> + int bno;
> +};

You've got to be kidding. So you want to keep a list of ints and do it
that way? With separate allocation for every sodding one and a cyclic
list going through the entire bunch?

Besides, 'int' is almost certainly a wrong type. I can buy 'u32', but...

> + do {
...
> + } while (1);

er... that's highly unidiomatic (and fairly common in your code below).
Any reasons for that?

> + bh_fat =
> + vmufat_sb_bread(sb, nextblock);
> + if (!bh_fat) {
> + error = -EIO;
> + goto fail;
> + }
> +
> + do {
> + fatdata = ((u16 *) bh_fat->b_data)[x];
> + if (fatdata == FAT_UNALLOCATED)
> + break; /*empty block */
> + if (--x < 0) {
> + put_bh(bh_fat);
> + if (--nextblock >= vmudetails->fat_bnum) {
> + x = VMU_BLK_SZ;
> + bh_fat = vmufat_sb_bread(sb, nextblock);
> + if (!bh_fat) {
> + error = -EIO;
> + goto fail;
> + }
> + } else
> + break;
> + }
> + } while (1);

Off-by-twice and that should've been a couple of nested loops.

> +static u16 vmufat_get_fat(struct super_block *sb, long block)
> +{
> + struct memcard *vmudetails = sb->s_fs_info;
> + struct buffer_head *bh;
> + int offset;
> + u16 block_content;
> + /* which block in the FAT */
> + offset = block / (VMU_BLK_SZ / 2);
> + if (offset >= vmudetails->fat_len)
> + return FAT_ERROR;
> +
> + bh = vmufat_sb_bread(sb, offset + 1 +
> + vmudetails->fat_bnum - vmudetails->fat_len);
> + if (!bh)
> + return FAT_ERROR;
> + /* look inside the block */
> + block_content = ((u16 *)bh->b_data)[block % (VMU_BLK_SZ / 2)];
> + put_bh(bh);
> + return block_content;
> +}

What's the endianness of that puppy?

> +
> + /* Walk through blocks looking for place to write
> + * Is this an executible file? */
> + if (imode & 73) {

No comments. Really. There must be some limits on the language one is
willing to use on public maillist, after all.

> +static int vmufat_readdir(struct file *filp, void *dirent, filldir_t filldir)
> +{
> + int filenamelen, i, error = 0;
> + struct vmufat_file_info *saved_file = NULL;
> + struct dentry *dentry = filp->f_dentry;
> + struct inode *inode = dentry->d_inode;
> + struct super_block *sb = inode->i_sb;
> + struct memcard *vmudetails = sb->s_fs_info;
> + struct buffer_head *bh;
> +
> + int blck_read = vmudetails->dir_bnum;
> + bh = vmufat_sb_bread(sb, blck_read);
> + if (!bh) {
> + error = -EIO;
> + goto out;
> + }
> +
> + i = filp->f_pos;
> +
> + /* handle . for this directory and .. for parent */
> + switch ((unsigned int) filp->f_pos) {
> + case 0:
> + if (filldir(dirent, ".", 1, i++, inode->i_ino, DT_DIR) < 0)
> + goto finish;
> +
> + filp->f_pos++;
> + case 1:
> + if (filldir(dirent, "..", 2, i++,
> + dentry->d_parent->d_inode->i_ino, DT_DIR) < 0)
> + goto finish;
> +
> + filp->f_pos++;
> + default:
> + break;
> + }
> +
> + /* trap reading beyond the end of the directory */
> + if ((i - 2) > (vmudetails->dir_len * DIR_ENT_PER_BLK)) {
> + error = -EINVAL;
> + goto release_bh;
> + }
> +
> + saved_file =
> + kmalloc(sizeof(struct vmufat_file_info), GFP_KERNEL);
> + if (!saved_file) {
> + error = -ENOMEM;
> + goto release_bh;
> + }
> +
> + do {
> + if ((i - 2) / DIR_ENT_PER_BLK >
> + (vmudetails->dir_bnum - blck_read)) {
> + /* move to next block in directory */
> + blck_read--;
> + if (vmudetails->dir_bnum - vmudetails->dir_len <=
> + blck_read)
> + break;
> + brelse(bh);
> + bh = vmufat_sb_bread(sb, blck_read);
> + if (!bh) {
> + kfree(saved_file);
> + error = -EIO;
> + goto out;
> + }
> + }
> +
> + saved_file->ftype = bh->b_data[vmufat_index(i - 2)];
> +
> + if (saved_file->ftype == 0)
> + break;
> +
> + saved_file->fblk =
> + le16_to_cpu(((u16 *) bh->b_data)[1 +
> + vmufat_index_16(i - 2)]);
> + if (saved_file->fblk == 0)
> + saved_file->fblk = VMUFAT_ZEROBLOCK;
> +
> + memcpy(saved_file->fname,
> + bh->b_data + 4 + vmufat_index(i - 2), VMUFAT_NAMELEN);
> + filenamelen = strlen(saved_file->fname);

Who said there will be NUL anywhere at all?

> + if (filenamelen > VMUFAT_NAMELEN)
> + filenamelen = VMUFAT_NAMELEN;

See above.

> + if (filldir
> + (dirent, saved_file->fname, filenamelen, i++,
> + saved_file->fblk, DT_REG) < 0) {
> + goto finish;
> + }

WTF do we bother with that copying, anyway?

> +static int vmufat_list_blocks(struct inode *in)
> +{
> + struct vmufat_inode *vi = VMUFAT_I(in);
> + struct super_block *sb = in->i_sb;
> + long nextblock;
> + long ino = in->i_ino;
> + struct memcard *vmudetails;
> + int error;
> + struct list_head *iter, *iter2;
> + struct vmufat_block_list *vbl, *nvbl;
> + u16 fatdata;
> +
> + vmudetails = sb->s_fs_info;
> + nextblock = ino;
> + if (nextblock == VMUFAT_ZEROBLOCK)
> + nextblock = 0;
> +
> + /* Delete any previous list of blocks */
> + list_for_each_safe(iter, iter2, &vi->blocks.b_list) {
> + vbl = list_entry(iter, struct vmufat_block_list, b_list);
> + list_del(iter);
> + kmem_cache_free(vmufat_blist_cachep, vbl);
> + }
> + vi->nblcks = 0;
> + do {
> + vbl = kmem_cache_alloc(vmufat_blist_cachep,
> + GFP_KERNEL);
> + if (!vbl) {
> + error = -ENOMEM;
> + goto unwind_out;
> + }
> + INIT_LIST_HEAD(&vbl->b_list);
> + vbl->bno = nextblock;
> + list_add_tail(&vbl->b_list, &vi->blocks.b_list);
> + vi->nblcks++;
> +
> + /* Find next block in the FAT - if there is one */
> + fatdata = vmufat_get_fat(sb, nextblock);
> + if (fatdata == FAT_UNALLOCATED) {
> + printk(KERN_WARNING "VMUFAT: FAT table appears to have"
> + " been corrupted.\n");
> + error = -EIO;
> + goto unwind_out;
> + }
> + if (fatdata == FAT_FILE_END)
> + break; /*end of file */
> + nextblock = fatdata;
> + } while (1);
> +
> + return 0;
> +
> +unwind_out:
> + list_for_each_entry_safe(vbl, nvbl, &vi->blocks.b_list, b_list) {
> + list_del_init(&vbl->b_list);
> + kmem_cache_free(vmufat_blist_cachep, vbl);
> + }
> + return error;
> +}

And you call *that* on every block allocation?

> + if (le16_to_cpu(((u16 *) bh->b_data)
__le16 *, please, and the same for other places like that.
> + [(y % DIR_ENT_PER_BLK) *
> + DIR_REC_LEN / 2 + 0x01]) == ino)
> + break;

> + if ((((u8 *) bh->b_data)[0x01 + z] ==
> + 0x00) & ~(sb->s_flags & MS_RDONLY))
> + inode->i_mode |= S_IWUGO;
> + /* Is file executible - ie a game */
> + if ((((u8 *) bh->b_data)[z] ==
> + 0xcc) & ~(sb->s_flags & MS_NOEXEC))
> + inode->i_mode |= S_IXUGO;

a) there's such thing as local variables. Use them.
b) when you do that, do remember that names may be longer than one character.
c) linux-kernel is not an IOCCC. This & ~ above is a pure obfuscation.

> +static void vmufat_put_super(struct super_block *sb)
> +{
> + sb->s_dev = 0;
WTF for? Leave handling that to fs/super.c, please.
> + kfree(sb->s_fs_info);
> +}

> + /* Look through the FAT */
> + nextblock = vmudetails->fat_bnum + vmudetails->fat_len - 1;
> + x = sb->s_blocksize;
> + bh_fat = vmufat_sb_bread(sb, nextblock);
> + if (!bh_fat) {
> + error = -EIO;
> + goto out;
> + }
> +
> + do {
> + fatdata = ((u16 *) bh_fat->b_data)[x];
> + if (fatdata == FAT_UNALLOCATED)
> + free++;
> + if (--x < 0) {
> + brelse(bh_fat);
> + if (--nextblock >= vmudetails->fat_bnum) {
> + x = sb->s_blocksize;
> + bh_fat = vmufat_sb_bread(sb, nextblock);
> + if (!bh_fat) {
> + error = -EIO;
> + goto out;
> + }
> + } else
> + break;
> + }
> + } while (1);

Pardon me, but... what the hell is going on in that code? In particular,
is there any reason for not making it a straightforward for() going through
blocks and equally straightforward inner for() going through each block?
BTW, there seems to be an off-by-factor-of-2 above nicely obfuscated by
all that mess. Namely,
x = sb->s_blocksize;
((u16 *) bh_fat->b_data)[x];
will end up accessing data at offset 2 * blocksize, which is twice the size
of actual block.

> +static int vmufat_unlink(struct inode *dir, struct dentry *dentry)
> +{
> + struct inode *in;
> +
> + in = dentry->d_inode;
> + if (!in)
> + return -EIO;
> + vmufat_delete_inode(in);
> + return 0;
> +}

And what happens when I open a file, unlink it and try to read?

Overall: code badly needs deobfuscation before anything else can be done
with it.