Re: [PATCH] sysctl: Fix proc_do_large_bitmap for large input buffers

[Eric Sandeen]

On 2/20/19 5:32 PM, Eric Sandeen wrote:
> Today, proc_do_large_bitmap() truncates a large write input buffer
> to PAGE_SIZE - 1, which may result in misparsed numbers at the
> (truncated) end of the buffer.  Further, it fails to notify the caller
> that the buffer was truncated, so it doesn't get called iteratively
> to finish the entire input buffer.
> 
> Tell the caller if there's more work to do by adding the skipped
> amount back to left/*lenp before returning.
> 
> To fix the misparsing, reset the position if we have completely
> consumed a truncated buffer (or if just one char is left, which
> may be a "-" in a range), and ask the caller to come back for
> more.
> 
> Signed-off-by: Eric Sandeen <sandeen@xxxxxxxxxx>

Would be nice to fix this bug.  I submitted the test node patch as well
as an attempt to integrate it into the test harness, though there's
wonkiness there still, and I could use more experienced eyes.

Can we move this forward?

Thanks,
-Eric

> ---
> 
> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> index ba4d9e85feb8..970a96659809 100644
> --- a/kernel/sysctl.c
> +++ b/kernel/sysctl.c
> @@ -3089,9 +3089,13 @@ int proc_do_large_bitmap(struct ctl_table *table, int write,
>  
>  	if (write) {
>  		char *kbuf, *p;
> +		size_t skipped = 0;
>  
> -		if (left > PAGE_SIZE - 1)
> +		if (left > PAGE_SIZE - 1) {
>  			left = PAGE_SIZE - 1;
> +			/* How much of the buffer we'll skip this pass */
> +			skipped = *lenp - left;
> +		}
>  
>  		p = kbuf = memdup_user_nul(buffer, left);
>  		if (IS_ERR(kbuf))
> @@ -3108,9 +3112,22 @@ int proc_do_large_bitmap(struct ctl_table *table, int write,
>  		while (!err && left) {
>  			unsigned long val_a, val_b;
>  			bool neg;
> +			size_t saved_left;
>  
> +			/* In case we stop parsing mid-number, we can reset */
> +			saved_left = left;
>  			err = proc_get_long(&p, &left, &val_a, &neg, tr_a,
>  					     sizeof(tr_a), &c);
> +			/*
> +			 * If we consumed the entirety of a truncated buffer or
> +			 * only one char is left (may be a "-"), then stop here,
> +			 * reset, & come back for more.
> +			 */
> +			if ((left <= 1) && skipped) {
> +				left = saved_left;
> +				break;
> +			}
> +
>  			if (err)
>  				break;
>  			if (val_a >= bitmap_len || neg) {
> @@ -3128,6 +3145,15 @@ int proc_do_large_bitmap(struct ctl_table *table, int write,
>  				err = proc_get_long(&p, &left, &val_b,
>  						     &neg, tr_b, sizeof(tr_b),
>  						     &c);
> +				/*
> +				 * If we consumed all of a truncated buffer or
> +				 * then stop here, reset, & come back for more.
> +				 */
> +				if (!left && skipped) {
> +					left = saved_left;
> +					break;
> +				}
> +
>  				if (err)
>  					break;
>  				if (val_b >= bitmap_len || neg ||
> @@ -3146,6 +3172,7 @@ int proc_do_large_bitmap(struct ctl_table *table, int write,
>  			proc_skip_char(&p, &left, '\n');
>  		}
>  		kfree(kbuf);
> +		left += skipped;
>  	} else {
>  		unsigned long bit_a, bit_b = 0;
>  
> 
>