[PATCH] Set bounds on what /proc/self/make-it-fail accepts.

[PATCH] Set bounds on what /proc/self/make-it-fail accepts.

[Dave Jones]

/proc/self/make-it-fail is a boolean, but accepts any number, including
negative ones. Change variable to unsigned, and cap upper bound at 1.

Signed-off-by: Dave Jones <davej@fedoraproject.org>

diff --git a/fs/proc/base.c b/fs/proc/base.c
index 51507065263b..b926377c354f 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -1207,7 +1207,7 @@ static ssize_t proc_fault_inject_read(struct file * file, char __user * buf,
 	struct task_struct *task = get_proc_task(file_inode(file));
 	char buffer[PROC_NUMBUF];
 	size_t len;
-	int make_it_fail;
+	unsigned int make_it_fail;
 
 	if (!task)
 		return -ESRCH;
@@ -1224,7 +1224,7 @@ static ssize_t proc_fault_inject_write(struct file * file,
 {
 	struct task_struct *task;
 	char buffer[PROC_NUMBUF], *end;
-	int make_it_fail;
+	unsigned int make_it_fail;
 
 	if (!capable(CAP_SYS_RESOURCE))
 		return -EPERM;
@@ -1236,6 +1236,9 @@ static ssize_t proc_fault_inject_write(struct file * file,
 	make_it_fail = simple_strtol(strstrip(buffer), &end, 0);
 	if (*end)
 		return -EINVAL;
+	if (make_it_fail > 1)
+		return -EINVAL;
+
 	task = get_proc_task(file_inode(file));
 	if (!task)
 		return -ESRCH;

Re: [PATCH] Set bounds on what /proc/self/make-it-fail accepts.

[David Rientjes]

On Tue, 18 Feb 2014, Dave Jones wrote:

> /proc/self/make-it-fail is a boolean, but accepts any number, including
> negative ones. Change variable to unsigned, and cap upper bound at 1.
> 
> Signed-off-by: Dave Jones <davej@fedoraproject.org>
> 

Hmm, this would break anything that uses anything other than one to enable 
it, but it looks like Documentation/fault-injection/fault-injection.txt 
only provides an example for when it does equal one, so it's probably an 
ok change.  I'm just wondering why non-zero is wrong?  Is this an 
interface that will be extended to support other modes?

Adding Akinobu to the cc.

> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index 51507065263b..b926377c354f 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -1207,7 +1207,7 @@ static ssize_t proc_fault_inject_read(struct file * file, char __user * buf,
>  	struct task_struct *task = get_proc_task(file_inode(file));
>  	char buffer[PROC_NUMBUF];
>  	size_t len;
> -	int make_it_fail;
> +	unsigned int make_it_fail;
>  
>  	if (!task)
>  		return -ESRCH;
> @@ -1224,7 +1224,7 @@ static ssize_t proc_fault_inject_write(struct file * file,
>  {
>  	struct task_struct *task;
>  	char buffer[PROC_NUMBUF], *end;
> -	int make_it_fail;
> +	unsigned int make_it_fail;
>  
>  	if (!capable(CAP_SYS_RESOURCE))
>  		return -EPERM;
> @@ -1236,6 +1236,9 @@ static ssize_t proc_fault_inject_write(struct file * file,
>  	make_it_fail = simple_strtol(strstrip(buffer), &end, 0);
>  	if (*end)
>  		return -EINVAL;
> +	if (make_it_fail > 1)
> +		return -EINVAL;
> +
>  	task = get_proc_task(file_inode(file));
>  	if (!task)
>  		return -ESRCH;

Re: [PATCH] Set bounds on what /proc/self/make-it-fail accepts.

[Dave Jones]

On Tue, Feb 18, 2014 at 02:32:02PM -0800, David Rientjes wrote:
 > On Tue, 18 Feb 2014, Dave Jones wrote:
 > 
 > > /proc/self/make-it-fail is a boolean, but accepts any number, including
 > > negative ones. Change variable to unsigned, and cap upper bound at 1.
 > 
 > Hmm, this would break anything that uses anything other than one to enable 
 > it, but it looks like Documentation/fault-injection/fault-injection.txt 
 > only provides an example for when it does equal one, so it's probably an 
 > ok change.  I'm just wondering why non-zero is wrong?  Is this an 
 > interface that will be extended to support other modes?

"Wrong" is perhaps too strong a word, but we only ever check it for non-zero state,
so it seems at best suboptimal to allow strange configurations.

When I saw I could set it to nonsense values like -1, I figured it could
use some idiot proofing. The lack of any checking at all surprised me.

Future extension of this interface seems unlikely given the boolean sounding name.
(Though we've done that in the past with things like the overcommit_memory sysctl,
 with pretty awful end-user results).

	Dave

Re: [PATCH] Set bounds on what /proc/self/make-it-fail accepts.

[Akinobu Mita]

2014-02-19 8:27 GMT+09:00 Dave Jones <davej@redhat.com>:
> On Tue, Feb 18, 2014 at 02:32:02PM -0800, David Rientjes wrote:
>  > On Tue, 18 Feb 2014, Dave Jones wrote:
>  >
>  > > /proc/self/make-it-fail is a boolean, but accepts any number, including
>  > > negative ones. Change variable to unsigned, and cap upper bound at 1.
>  >
>  > Hmm, this would break anything that uses anything other than one to enable
>  > it, but it looks like Documentation/fault-injection/fault-injection.txt
>  > only provides an example for when it does equal one, so it's probably an
>  > ok change.  I'm just wondering why non-zero is wrong?  Is this an
>  > interface that will be extended to support other modes?
>
> "Wrong" is perhaps too strong a word, but we only ever check it for non-zero state,
> so it seems at best suboptimal to allow strange configurations.
>
> When I saw I could set it to nonsense values like -1, I figured it could
> use some idiot proofing. The lack of any checking at all surprised me.
>
> Future extension of this interface seems unlikely given the boolean sounding name.
> (Though we've done that in the past with things like the overcommit_memory sysctl,
>  with pretty awful end-user results).

I don't have any plans to extend /proc/self/make-it-fail to support
other than 0 or 1.  So I have no objection against this change.

Reviewed-by: Akinobu Mita <akinobu.mita@gmail.com>

Re: [PATCH] Set bounds on what /proc/self/make-it-fail accepts.

[David Rientjes]

On Wed, 19 Feb 2014, Akinobu Mita wrote:

> I don't have any plans to extend /proc/self/make-it-fail to support
> other than 0 or 1.  So I have no objection against this change.
> 
> Reviewed-by: Akinobu Mita <akinobu.mita@gmail.com>
> 

Ok, thanks.  I hoped that the simple_strtol() would have been replaced by 
kstrtoint() since it's the preferred interface to make a lot of this 
simpler and then just test for val == !!val, but the minimal change is 
fine as well.

Acked-by: David Rientjes <rientjes@google.com>

Re: [PATCH] Set bounds on what /proc/self/make-it-fail accepts.

[Andrew Morton]

On Tue, 18 Feb 2014 17:06:06 -0500 Dave Jones <davej@redhat.com> wrote:

> /proc/self/make-it-fail is a boolean, but accepts any number, including
> negative ones. Change variable to unsigned, and cap upper bound at 1.
> 
> Signed-off-by: Dave Jones <davej@fedoraproject.org>
> 
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index 51507065263b..b926377c354f 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -1207,7 +1207,7 @@ static ssize_t proc_fault_inject_read(struct file * file, char __user * buf,
>  	struct task_struct *task = get_proc_task(file_inode(file));
>  	char buffer[PROC_NUMBUF];
>  	size_t len;
> -	int make_it_fail;
> +	unsigned int make_it_fail;
>  
>  	if (!task)
>  		return -ESRCH;
> @@ -1224,7 +1224,7 @@ static ssize_t proc_fault_inject_write(struct file * file,
>  {
>  	struct task_struct *task;
>  	char buffer[PROC_NUMBUF], *end;
> -	int make_it_fail;
> +	unsigned int make_it_fail;
>  
>  	if (!capable(CAP_SYS_RESOURCE))
>  		return -EPERM;
> @@ -1236,6 +1236,9 @@ static ssize_t proc_fault_inject_write(struct file * file,
>  	make_it_fail = simple_strtol(strstrip(buffer), &end, 0);
>  	if (*end)
>  		return -EINVAL;
> +	if (make_it_fail > 1)
> +		return -EINVAL;
> +
>  	task = get_proc_task(file_inode(file));
>  	if (!task)
>  		return -ESRCH;

Switching `make_it_fail' to unsigned makes the test simpler but it does
rather muck up the typing in there.  task_struct.make_it_fail is still
an int, we should now use simple_strtoul rather than simple_strtol,
proc_fault_inject_read()'s snprintf() should now use %u, etc.  None of
which actually matters, but still...

Rather than address all that I'm inclined to leave `make_it_fail' as an
int, turning your patch into a one-liner?

--- a/fs/proc/base.c~fault-injection-set-bounds-on-what-proc-self-make-it-fail-accepts-fix
+++ a/fs/proc/base.c
@@ -1207,7 +1207,7 @@ static ssize_t proc_fault_inject_read(st
 	struct task_struct *task = get_proc_task(file_inode(file));
 	char buffer[PROC_NUMBUF];
 	size_t len;
-	unsigned int make_it_fail;
+	int make_it_fail;
 
 	if (!task)
 		return -ESRCH;
@@ -1224,7 +1224,7 @@ static ssize_t proc_fault_inject_write(s
 {
 	struct task_struct *task;
 	char buffer[PROC_NUMBUF], *end;
-	unsigned int make_it_fail;
+	int make_it_fail;
 
 	if (!capable(CAP_SYS_RESOURCE))
 		return -EPERM;
@@ -1236,7 +1236,7 @@ static ssize_t proc_fault_inject_write(s
 	make_it_fail = simple_strtol(strstrip(buffer), &end, 0);
 	if (*end)
 		return -EINVAL;
-	if (make_it_fail > 1)
+	if (make_it_fail < 0 || make_it_fail > 1)
 		return -EINVAL;
 
 	task = get_proc_task(file_inode(file));
_

Re: [PATCH] Set bounds on what /proc/self/make-it-fail accepts.

[Dave Jones]

On Wed, Feb 19, 2014 at 01:40:25PM -0800, Andrew Morton wrote:
 
 > Switching `make_it_fail' to unsigned makes the test simpler but it does
 > rather muck up the typing in there.  task_struct.make_it_fail is still
 > an int, we should now use simple_strtoul rather than simple_strtol,
 > proc_fault_inject_read()'s snprintf() should now use %u, etc.  None of
 > which actually matters, but still...
 
I toyed with the idea of changing task_struct.make_it_fail to unsigned too,
but only realized I missed that after I'd sent out the diff.

 > Rather than address all that I'm inclined to leave `make_it_fail' as an
 > int, turning your patch into a one-liner?

That works for me too.

	Dave

Re: [PATCH] Set bounds on what /proc/self/make-it-fail accepts.

[Andrew Morton]

On Wed, 19 Feb 2014 16:55:05 -0500 Dave Jones <davej@redhat.com> wrote:

> On Wed, Feb 19, 2014 at 01:40:25PM -0800, Andrew Morton wrote:
>  
>  > Switching `make_it_fail' to unsigned makes the test simpler but it does
>  > rather muck up the typing in there.  task_struct.make_it_fail is still
>  > an int, we should now use simple_strtoul rather than simple_strtol,
>  > proc_fault_inject_read()'s snprintf() should now use %u, etc.  None of
>  > which actually matters, but still...
>  
> I toyed with the idea of changing task_struct.make_it_fail to unsigned too,
> but only realized I missed that after I'd sent out the diff.

If we're touching the task_struct we could make it a bool.

Or just a single bit(field).  task_struct already has a bunch of
bitfields in it (strangely, they aren't contiguous).  But some locking
would be needed if tasks-other-than-current can modify the bit.

Re: [PATCH] Set bounds on what /proc/self/make-it-fail accepts.

[David Rientjes]

On Wed, 19 Feb 2014, Andrew Morton wrote:

> If we're touching the task_struct we could make it a bool.
> 
> Or just a single bit(field).  task_struct already has a bunch of
> bitfields in it (strangely, they aren't contiguous).  But some locking
> would be needed if tasks-other-than-current can modify the bit.
> 

Or store to any adjacent bit in the word.  I think the simplest solution 
is to use kstrtoint() and test for val == !!val since, as we all know, 
{simple,strict}_strto* is deprecated per checkpatch :)

Re: [PATCH] Set bounds on what /proc/self/make-it-fail accepts.

[Dave Jones]

On Wed, Feb 19, 2014 at 02:00:21PM -0800, Andrew Morton wrote:
 
 > > I toyed with the idea of changing task_struct.make_it_fail to unsigned too,
 > > but only realized I missed that after I'd sent out the diff.
 > 
 > If we're touching the task_struct we could make it a bool.
 > 
 > Or just a single bit(field).  task_struct already has a bunch of
 > bitfields in it (strangely, they aren't contiguous).

afaics, asides from brk_randomized, they're contiguous, and gcc dtrt..

        unsigned int               in_execve:1;          /*   768:31  4 */
        unsigned int               in_iowait:1;          /*   768:30  4 */
        unsigned int               no_new_privs:1;       /*   768:29  4 */
        unsigned int               sched_reset_on_fork:1; /*   768:28  4 */
        unsigned int               sched_contributes_to_load:1; /*   768:27  4 */

So we could move the COMPAT_BRK ifdef and save 4 bytes for all the people still using libc5.
(Or those who are for some reason averse to heap randomization).

It's not really worth doing unless you're moving a bunch of other stuff around
in task_struct though, because as it is now, that struct has a bunch of alignment padding
& holes, so you're not going to save anything.

The other tricky part with reorganizing that struct is that so much of it is configurable.

	Dave