Skip to contentSkip to conversation

adfs: fix E+/F+ dir size > 2048 crashing kernel

PATCH Andrew Morton 1 participant 1 message Jan 19, 2011 1 message not in the archive

lore
Share & tools
lore
#1 lore raw
In reply to a message not in the archive
Quoted 6 lines · On Wed, 12 Jan 2011 17:56:41 +0000 Stuart Swales…

From: Stuart Swales<[email protected]>

Kernel crashes in fs/adfs module when accessing directories with a large number of objects on mounted Acorn ADFS E+/F+ format discs (or images) as the existing code writes off the end of the fixed array of struct buffer_head pointers.

Quoted 8 lines

Additionally, each directory access that didn't crash would leak a buffer as nr_buffers was not adjusted correctly for E+/F+ discs (was always left as one less than required).

The patch fixes this by allocating a dynamically-sized set of struct buffer_head pointers if necessary for the E+/F+ case (many directories still do in fact fit in 2048 bytes) and sets the correct nr_buffers so that all buffers are released.

I don't know adfs from a bar of soap. Poor me :(

Quoted 40 lines

...

(quoted diff) +12 -6
@@ -22,30 +23,53 @@ adfs_fplus_read(struct super_block *sb,
 
  	dir->nr_buffers = 0;
 
+	/* start off using fixed bh set - only alloc for big dirs */
+	dir->bh_fplus =&dir->bh[0];
+
  	block = __adfs_block_map(sb, id, 0);
  	if (!block) {
  		adfs_error(sb, "dir object %X has a hole at offset 0", id);
  		goto out;
  	}
 
-	dir->bh[0] = sb_bread(sb, block);
-	if (!dir->bh[0])
+	dir->bh_fplus[0] = sb_bread(sb, block);
+	if (!dir->bh_fplus[0])
  		goto out;
  	dir->nr_buffers += 1;
 
-	h = (struct adfs_bigdirheader *)dir->bh[0]->b_data;
+	h = (struct adfs_bigdirheader *)dir->bh_fplus[0]->b_data;
  	size = le32_to_cpu(h->bigdirsize);
  	if (size != sz) {
-		printk(KERN_WARNING "adfs: adfs_fplus_read: directory header size\n"
-				" does not match directory size\n");
+		printk(KERN_WARNING "adfs: adfs_fplus_read:"
+					" directory header size %X\n"
+					" does not match directory size %X\n",
+					size, sz);
  	}
 
  	if (h->bigdirversion[0] != 0 || h->bigdirversion[1] != 0 ||
  	    h->bigdirversion[2] != 0 || size&  2047 ||
-	    h->bigdirstartname != cpu_to_le32(BIGDIRSTARTNAME))
+	    h->bigdirstartname != cpu_to_le32(BIGDIRSTARTNAME)) {
+		printk(KERN_WARNING "adfs: dir object %X has"

Review adfs_fplus_read(struct super_block *sb, · line 62

Quoted 10 lines
(quoted diff) +7 -0
+					" malformed dir header\n", id);
  		goto out;
+	}
 
  	size>>= sb->s_blocksize_bits;
+	if (size>  sizeof(dir->bh)/sizeof(dir->bh[0])) {
+		/* this directory is too big for fixed bh set, must allocate */
+		struct buffer_head **bh_fplus =
+			kzalloc(size * sizeof(struct buffer_head *),
+				GFP_KERNEL);

kcalloc() would produce a safer and more pleasing result here.

Quoted 43 lines
(quoted diff) +24 -5
+		if (!bh_fplus) {
+			adfs_error(sb, "not enough memory for"
+					" dir object %X (%d blocks)", id, size);
+			goto out;
+		}
+		dir->bh_fplus = bh_fplus;
+		/* copy over the pointer to the block that we've already read */
+		dir->bh_fplus[0] = dir->bh[0];
+	}
+
  	for (blk = 1; blk<  size; blk++) {
  		block = __adfs_block_map(sb, id, blk);
  		if (!block) {
@@ -53,25 +77,44 @@ adfs_fplus_read(struct super_block *sb,
  			goto out;
  		}
 
-		dir->bh[blk] = sb_bread(sb, block);
-		if (!dir->bh[blk])
+		dir->bh_fplus[blk] = sb_bread(sb, block);
+		if (!dir->bh_fplus[blk]) {
+			adfs_error(sb,	"dir object %X failed read for"
+					" offset %d, mapped block %X",
+					id, blk, block);
  			goto out;
-		dir->nr_buffers = blk;
+		}
+
+		dir->nr_buffers += 1;
  	}
 
-	t = (struct adfs_bigdirtail *)(dir->bh[size - 1]->b_data + (sb->s_blocksize - 8));
+	t = (struct adfs_bigdirtail *)
+		(dir->bh_fplus[size - 1]->b_data + (sb->s_blocksize - 8));
 
  	if (t->bigdirendname != cpu_to_le32(BIGDIRENDNAME) ||
  	    t->bigdirendmasseq != h->startmasseq ||
-	    t->reserved[0] != 0 || t->reserved[1] != 0)
+	    t->reserved[0] != 0 || t->reserved[1] != 0) {
+		printk(KERN_WARNING "adfs: dir object %X has "
+					"malformed dir end\n", id);
  		goto out;
+	}

Review adfs_fplus_read(struct super_block *sb, · line 108

Quoted 10 lines
(quoted diff) +3 -2
 
  	dir->parent_id = le32_to_cpu(h->bigdirparent);
  	dir->sb = sb;
  	return 0;
+
  out:
-	for (i = 0; i<  dir->nr_buffers; i++)
-		brelse(dir->bh[i]);
+	if (dir->bh_fplus) {
+		for (i = 0; i<  dir->nr_buffers; i++)

Strange that checkpatch didn't complain about the layout here.

Review adfs_fplus_read(struct super_block *sb, · line 109

Quoted 1 line
(quoted diff) +1 -0
+			brelse(dir->bh_fplus[i]);

brelse() is only needed if the bh might be NULL. Otherwise, put_bh().

Quoted 22 lines
(quoted diff) +10 -2
+		if (&dir->bh[0] != dir->bh_fplus)
+			kfree(dir->bh_fplus);
+
+		dir->bh_fplus = NULL;
+	}
+
+	dir->nr_buffers = 0;
  	dir->sb = NULL;
  	return ret;
  }
 
...
@@ -174,8 +224,17 @@ adfs_fplus_free(struct adfs_dir *dir)
  {
  	int i;
 
-	for (i = 0; i<  dir->nr_buffers; i++)
-		brelse(dir->bh[i]);
+	if (dir->bh_fplus) {
+		for (i = 0; i<  dir->nr_buffers; i++)
+			brelse(dir->bh_fplus[i]);

Review adfs_fplus_free(struct adfs_dir *dir) · line 239

Quoted 10 lines
(quoted diff) +8 -0
+
+		if (&dir->bh[0] != dir->bh_fplus)
+			kfree(dir->bh_fplus);
+
+		dir->bh_fplus = NULL;
+	}
+
+	dir->nr_buffers = 0;
  	dir->sb = NULL;
  }

Dittoes.

Oh well, the code eyeballs OK to me and you tested it, so I'll toss it in there.