| Message ID | 20180123172000.20919-1-eguan@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 1/23/18 10:20 AM, Eryu Guan wrote: > 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.
On Tue, Jan 23, 2018 at 02:32:06PM -0700, Jens Axboe wrote: > On 1/23/18 10:20 AM, Eryu Guan wrote: > > 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 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. > 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
On Wed, Jan 24, 2018 at 11:49:26AM +0800, Ming Lei wrote: > On Tue, Jan 23, 2018 at 02:32:06PM -0700, Jens Axboe wrote: > > On 1/23/18 10:20 AM, Eryu Guan wrote: > > > 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 > > 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 > > > 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
On 1/23/18 8:49 PM, Ming Lei wrote: > On Tue, Jan 23, 2018 at 02:32:06PM -0700, Jens Axboe wrote: >> On 1/23/18 10:20 AM, Eryu Guan wrote: >>> 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 > > 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. > >> 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.
diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c index b56a4f35720d..54bd8c31b822 100644 --- a/block/blk-mq-debugfs.c +++ b/block/blk-mq-debugfs.c @@ -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);
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 <ming.lei@redhat.com> Signed-off-by: Eryu Guan <eguan@redhat.com> --- block/blk-mq-debugfs.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)