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

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

[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.

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(-)

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);
-- 
2.14.3

Re: [PATCH] blk-mq-debugfs: don't allow write on attributes with seq_operations set

[Jens Axboe]

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.

-- 
Jens Axboe

Re: [PATCH] blk-mq-debugfs: don't allow write on attributes with seq_operations set

[Ming Lei]

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

Re: [PATCH] blk-mq-debugfs: don't allow write on attributes with seq_operations set

[Eryu Guan]

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

Re: [PATCH] blk-mq-debugfs: don't allow write on attributes with seq_operations set

[Jens Axboe]

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.

-- 
Jens Axboe