blk-mq-debugfs: don't allow write on attributes with seq_operations set

PATCH In review Eryu Guan 3 participants 5 messages Jan 23, 2018 – Jan 24, 2018 Block layer

Share & tools

Eryu Guan

author

2018-01-23 17:20 UTC #0 lore raw

PATCH 1/1 blk-mq-debugfs: don't allow write on attributes with seq_operations set

Attributes that only implement .seq_ops are read-only, any write to them should be rejected. But currently kernel would crash when writing to such debugfs entries, e.g.

chmod +w /sys/kernel/debug/block/<dev>/requeue_list echo 0 > /sys/kernel/debug/block/<dev>/requeue_list chmod -w /sys/kernel/debug/block/<dev>/requeue_list

Fix it by returning -EPERM in blk_mq_debugfs_write() when writing to such attributes.

Cc Ming Lei <[email protected]>

Signed-off-by Eryu Guan <[email protected]>

---
 block/blk-mq-debugfs.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

block/blk-mq-debugfs.c +5 -1

index b56a4f35720d..54bd8c31b822 100644
@@ -703,7 +703,11 @@ static ssize_t blk_mq_debugfs_write(struct file *file, const char __user *buf,
 	const struct blk_mq_debugfs_attr *attr = m->private;
 	void *data = d_inode(file->f_path.dentry->d_parent)->i_private;
 
-	if (!attr->write)
+	/*
+	 * Attributes that only implement .seq_ops are read-only and 'attr' is
+	 * the same with 'data' in this case.
+	 */
+	if (attr == data || !attr->write)
 		return -EPERM;
 
 	return attr->write(data, buf, count, ppos);

Signature · 1 line

2.14.3

Jens Axboe

maintainer

2018-01-23 21:32 UTC #1 lore raw

Quoted 10 lines · Eryu Guan

Attributes that only implement .seq_ops are read-only, any write to them should be rejected. But currently kernel would crash when writing to such debugfs entries, e.g.
chmod +w /sys/kernel/debug/block/<dev>/requeue_list echo 0 > /sys/kernel/debug/block/<dev>/requeue_list chmod -w /sys/kernel/debug/block/<dev>/requeue_list
Fix it by returning -EPERM in blk_mq_debugfs_write() when writing to such attributes.

I don't particularly like the fix, since it's not really clear why that comparison makes sense. Can't we just prevent anyone from making the debugfs entries writable? Seems like a much more sane approach.

Signature · 1 line

Jens Axboe

Ming Lei

2018-01-24 03:49 UTC #2 lore raw

Quoted 9 lines · Jens Axboe

Quoted 8 lines · Eryu Guan
Attributes that only implement .seq_ops are read-only, any write to them should be rejected. But currently kernel would crash when writing to such debugfs entries, e.g.
chmod +w /sys/kernel/debug/block/<dev>/requeue_list echo 0 > /sys/kernel/debug/block/<dev>/requeue_list chmod -w /sys/kernel/debug/block/<dev>/requeue_list

Quoted 5 lines

Quoted 2 lines
Fix it by returning -EPERM in blk_mq_debugfs_write() when writing to such attributes.
I don't particularly like the fix, since it's not really clear why that comparison makes sense. Can't we just prevent anyone from

It might be the simplest way to check if the attribute defines .seq_ops or not. If it is .seq_ops, it is wrong to interpret m->private as 'struct blk_mq_debugfs_attr *' because it actually points to 'struct request_queue *' or others, which depends on the specific attribute.

So it works for avoiding the oops.

Quoted 2 lines

making the debugfs entries writable? Seems like a much more sane approach.

I guess fs should allow root user to do 'chmod +w' on files in proc, debugfs or sysfs. I just tried, it works on proc, debugfs and sysfs.

Thanks,
Ming

Eryu Guan

author

2018-01-24 04:06 UTC #3 lore raw

Quoted 16 lines · Ming Lei

Quoted 9 lines · Jens Axboe
Quoted 8 lines · Eryu Guan
Attributes that only implement .seq_ops are read-only, any write to them should be rejected. But currently kernel would crash when writing to such debugfs entries, e.g.
chmod +w /sys/kernel/debug/block/<dev>/requeue_list echo 0 > /sys/kernel/debug/block/<dev>/requeue_list chmod -w /sys/kernel/debug/block/<dev>/requeue_list
Quoted 5 lines
Quoted 2 lines
Fix it by returning -EPERM in blk_mq_debugfs_write() when writing to such attributes.
I don't particularly like the fix, since it's not really clear why that comparison makes sense. Can't we just prevent anyone from

Quoted 6 lines

It might be the simplest way to check if the attribute defines .seq_ops or not. If it is .seq_ops, it is wrong to interpret m->private as 'struct blk_mq_debugfs_attr *' because it actually points to 'struct request_queue *' or others, which depends on the specific attribute.
So it works for avoiding the oops.

I agreed this is not a elegant fix but, as Ming suggested, might be the simplest. I could put more comments in the code about why the comparison makes sense.

Thanks,
Eryu

Quoted 10 lines

Quoted 3 lines
making the debugfs entries writable? Seems like a much more sane approach.
I guess fs should allow root user to do 'chmod +w' on files in proc, debugfs or sysfs. I just tried, it works on proc, debugfs and sysfs.
Thanks,
Ming

Jens Axboe

maintainer

2018-01-24 16:45 UTC #4 lore raw

In reply to Ming Lei

Quoted 21 lines · Ming Lei

Quoted 9 lines · Jens Axboe
Quoted 8 lines · Eryu Guan
Attributes that only implement .seq_ops are read-only, any write to them should be rejected. But currently kernel would crash when writing to such debugfs entries, e.g.
chmod +w /sys/kernel/debug/block/<dev>/requeue_list echo 0 > /sys/kernel/debug/block/<dev>/requeue_list chmod -w /sys/kernel/debug/block/<dev>/requeue_list
Quoted 5 lines
Quoted 2 lines
Fix it by returning -EPERM in blk_mq_debugfs_write() when writing to such attributes.
I don't particularly like the fix, since it's not really clear why that comparison makes sense. Can't we just prevent anyone from
It might be the simplest way to check if the attribute defines .seq_ops or not. If it is .seq_ops, it is wrong to interpret m->private as 'struct blk_mq_debugfs_attr *' because it actually points to 'struct request_queue *' or others, which depends on the specific attribute.

Quoted 7 lines

So it works for avoiding the oops.
Quoted 2 lines
making the debugfs entries writable? Seems like a much more sane approach.
I guess fs should allow root user to do 'chmod +w' on files in proc, debugfs or sysfs. I just tried, it works on proc, debugfs and sysfs.

Yeah good point, I guess the proposed fix is the simplest version we can do. At least it has a comment. I'll apply it, thanks.

Signature · 1 line

Jens Axboe