diff mbox series

sysctl: Limit the size of I/Os to PAGE_SIZE

Message ID 20210513160649.2280429-1-willy@infradead.org (mailing list archive)
State New
Headers show
Series sysctl: Limit the size of I/Os to PAGE_SIZE | expand

Commit Message

Matthew Wilcox May 13, 2021, 4:06 p.m. UTC
We currently allow a read or a write that is up to KMALLOC_MAX_SIZE.
This has caused problems when cat decides to do a 64kB read and
so we allocate a 64kB buffer for the sysctl handler to store into.
The immediate problem was fixed by switching to kvmalloc(), but it's
ridiculous to allocate so much memory to read what is likely to be a
few bytes.

sysfs limits reads and writes to PAGE_SIZE, and I feel we should do the
same for sysctl.  The largest sysctl anyone's been able to come up with
is 433 bytes for /proc/sys/dev/cdrom/info

This will allow simplifying the BPF sysctl code later, but I'll leave
that for someone who understands it better.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 fs/proc/proc_sysctl.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

Comments

Josef Bacik May 13, 2021, 4:12 p.m. UTC | #1
On 5/13/21 12:06 PM, Matthew Wilcox (Oracle) wrote:
> We currently allow a read or a write that is up to KMALLOC_MAX_SIZE.
> This has caused problems when cat decides to do a 64kB read and
> so we allocate a 64kB buffer for the sysctl handler to store into.
> The immediate problem was fixed by switching to kvmalloc(), but it's
> ridiculous to allocate so much memory to read what is likely to be a
> few bytes.
> 
> sysfs limits reads and writes to PAGE_SIZE, and I feel we should do the
> same for sysctl.  The largest sysctl anyone's been able to come up with
> is 433 bytes for /proc/sys/dev/cdrom/info
> 
> This will allow simplifying the BPF sysctl code later, but I'll leave
> that for someone who understands it better.
> 
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> ---
>   fs/proc/proc_sysctl.c | 15 +++++++++------
>   1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
> index dea0f5ee540c..a97a8a4ff270 100644
> --- a/fs/proc/proc_sysctl.c
> +++ b/fs/proc/proc_sysctl.c
> @@ -562,11 +562,14 @@ static ssize_t proc_sys_call_handler(struct kiocb *iocb, struct iov_iter *iter,
>   	if (!table->proc_handler)
>   		goto out;
>   
> -	/* don't even try if the size is too large */
> +	/* reads may return short values; large writes must fail now */
> +	if (count >= PAGE_SIZE) {
> +		if (write)
> +			goto out;
> +		count = PAGE_SIZE;
> +	}
>   	error = -ENOMEM;
> -	if (count >= KMALLOC_MAX_SIZE)
> -		goto out;
> -	kbuf = kvzalloc(count + 1, GFP_KERNEL);
> +	kbuf = kmalloc(PAGE_SIZE, GFP_KERNEL);
>   	if (!kbuf)
>   		goto out;
>   

Below here we have

kbuf[count] = '\0';

with will overflow with this patch.  So maybe instead we do

if (count >= PAGE_SIZE)
	count = PAGE_SIZE - 1;
kbuf = kmalloc(count);

and that solves your problem and keeps us from overflowing.  Thanks,

Josef
Josef Bacik May 13, 2021, 4:15 p.m. UTC | #2
On 5/13/21 12:12 PM, Josef Bacik wrote:
> On 5/13/21 12:06 PM, Matthew Wilcox (Oracle) wrote:
>> We currently allow a read or a write that is up to KMALLOC_MAX_SIZE.
>> This has caused problems when cat decides to do a 64kB read and
>> so we allocate a 64kB buffer for the sysctl handler to store into.
>> The immediate problem was fixed by switching to kvmalloc(), but it's
>> ridiculous to allocate so much memory to read what is likely to be a
>> few bytes.
>>
>> sysfs limits reads and writes to PAGE_SIZE, and I feel we should do the
>> same for sysctl.  The largest sysctl anyone's been able to come up with
>> is 433 bytes for /proc/sys/dev/cdrom/info
>>
>> This will allow simplifying the BPF sysctl code later, but I'll leave
>> that for someone who understands it better.
>>
>> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
>> ---
>>   fs/proc/proc_sysctl.c | 15 +++++++++------
>>   1 file changed, 9 insertions(+), 6 deletions(-)
>>
>> diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
>> index dea0f5ee540c..a97a8a4ff270 100644
>> --- a/fs/proc/proc_sysctl.c
>> +++ b/fs/proc/proc_sysctl.c
>> @@ -562,11 +562,14 @@ static ssize_t proc_sys_call_handler(struct kiocb *iocb, 
>> struct iov_iter *iter,
>>       if (!table->proc_handler)
>>           goto out;
>> -    /* don't even try if the size is too large */
>> +    /* reads may return short values; large writes must fail now */
>> +    if (count >= PAGE_SIZE) {
>> +        if (write)
>> +            goto out;
>> +        count = PAGE_SIZE;
>> +    }
>>       error = -ENOMEM;
>> -    if (count >= KMALLOC_MAX_SIZE)
>> -        goto out;
>> -    kbuf = kvzalloc(count + 1, GFP_KERNEL);
>> +    kbuf = kmalloc(PAGE_SIZE, GFP_KERNEL);
>>       if (!kbuf)
>>           goto out;
> 
> Below here we have
> 
> kbuf[count] = '\0';
> 

Nevermind I just re-read it and it's

if (write)
	kbuf[count] = '\0';

I'm stupid, you can add

Reviewed-by: Josef Bacik <josef@toxicpanda.com>

Thanks,

Josef
diff mbox series

Patch

diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
index dea0f5ee540c..a97a8a4ff270 100644
--- a/fs/proc/proc_sysctl.c
+++ b/fs/proc/proc_sysctl.c
@@ -562,11 +562,14 @@  static ssize_t proc_sys_call_handler(struct kiocb *iocb, struct iov_iter *iter,
 	if (!table->proc_handler)
 		goto out;
 
-	/* don't even try if the size is too large */
+	/* reads may return short values; large writes must fail now */
+	if (count >= PAGE_SIZE) {
+		if (write)
+			goto out;
+		count = PAGE_SIZE;
+	}
 	error = -ENOMEM;
-	if (count >= KMALLOC_MAX_SIZE)
-		goto out;
-	kbuf = kvzalloc(count + 1, GFP_KERNEL);
+	kbuf = kmalloc(PAGE_SIZE, GFP_KERNEL);
 	if (!kbuf)
 		goto out;
 
@@ -582,12 +585,12 @@  static ssize_t proc_sys_call_handler(struct kiocb *iocb, struct iov_iter *iter,
 	if (error)
 		goto out_free_buf;
 
-	/* careful: calling conventions are nasty here */
 	error = table->proc_handler(table, write, kbuf, &count, &iocb->ki_pos);
 	if (error)
 		goto out_free_buf;
 
 	if (!write) {
+		/* Give BPF the chance to override a read result here? */
 		error = -EFAULT;
 		if (copy_to_iter(kbuf, count, iter) < count)
 			goto out_free_buf;
@@ -595,7 +598,7 @@  static ssize_t proc_sys_call_handler(struct kiocb *iocb, struct iov_iter *iter,
 
 	error = count;
 out_free_buf:
-	kvfree(kbuf);
+	kfree(kbuf);
 out:
 	sysctl_head_finish(head);