[PATCH] stm class: fix a missing-check bug

[PATCH] stm class: fix a missing-check bug

[Wenwen Wang]

In stm_char_policy_set_ioctl(), the 'size' field of the struct
'stp_polic_id' is firstly copied from the user space and then checked,
because the length of the 'id' field in this struct, which represents an
identification string, is not fixed. If the 'size' field cannot pass the
check, an error code EINVAL will be returned. However, after the check, the
whole struct is copied again from the user space. Given that the user data
resides in the user space, a malicious user-space process can race to
change the size between the two copies. By doing so, the attacker can
bypass the check on the 'size' field and inject malicious data.

This patch removes the re-copying of the 'size' field in the second copy to
avoid the above issue.

Signed-off-by: Wenwen Wang <wang6495@umn.edu>
---
 drivers/hwtracing/stm/core.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/hwtracing/stm/core.c b/drivers/hwtracing/stm/core.c
index 10bcb5d..7617fb4 100644
--- a/drivers/hwtracing/stm/core.c
+++ b/drivers/hwtracing/stm/core.c
@@ -570,11 +570,13 @@ static int stm_char_policy_set_ioctl(struct stm_file *stmf, void __user *arg)
 	if (!id)
 		return -ENOMEM;
 
-	if (copy_from_user(id, arg, size)) {
+	if (copy_from_user(&id->master, (char __user *)arg + sizeof(size),
+				size - sizeof(size))) {
 		ret = -EFAULT;
 		goto err_free;
 	}
 
+	id->size = size;
 	if (id->__reserved_0 || id->__reserved_1)
 		goto err_free;
 
-- 
2.7.4

Re: [PATCH] stm class: fix a missing-check bug

[Alexander Shishkin]

Wenwen Wang <wang6495@umn.edu> writes:

> In stm_char_policy_set_ioctl(), the 'size' field of the struct
> 'stp_polic_id' is firstly copied from the user space and then checked,
> because the length of the 'id' field in this struct, which represents an
> identification string, is not fixed. If the 'size' field cannot pass the
> check, an error code EINVAL will be returned. However, after the check, the
> whole struct is copied again from the user space. Given that the user data
> resides in the user space, a malicious user-space process can race to
> change the size between the two copies. By doing so, the attacker can
> bypass the check on the 'size' field and inject malicious data.

How? The id->size is not used for anything.

And even if there was a problem, this:

> -	if (copy_from_user(id, arg, size)) {
> +	if (copy_from_user(&id->master, (char __user *)arg + sizeof(size),
> +				size - sizeof(size))) {

is completely pointless.

>  		ret = -EFAULT;
>  		goto err_free;
>  	}
>  
> +	id->size = size;

So, if we did use id->size after the copying, we'd indeed have this line
in the code. But since we don't, it's also pointless, so it's not there.

Thanks,
--
Alex

Re: [PATCH] stm class: fix a missing-check bug

[Wenwen Wang]

On Wed, Oct 3, 2018 at 2:57 AM Alexander Shishkin
<alexander.shishkin@linux.intel.com> wrote:
>
> Wenwen Wang <wang6495@umn.edu> writes:
>
> > In stm_char_policy_set_ioctl(), the 'size' field of the struct
> > 'stp_polic_id' is firstly copied from the user space and then checked,
> > because the length of the 'id' field in this struct, which represents an
> > identification string, is not fixed. If the 'size' field cannot pass the
> > check, an error code EINVAL will be returned. However, after the check, the
> > whole struct is copied again from the user space. Given that the user data
> > resides in the user space, a malicious user-space process can race to
> > change the size between the two copies. By doing so, the attacker can
> > bypass the check on the 'size' field and inject malicious data.
>
> How? The id->size is not used for anything.
>
> And even if there was a problem, this:
>
> > -     if (copy_from_user(id, arg, size)) {
> > +     if (copy_from_user(&id->master, (char __user *)arg + sizeof(size),
> > +                             size - sizeof(size))) {
>
> is completely pointless.

Given that id->size is not used, it should not be copied from the user
space. This code is used to remove such redundant copy.

>
> >               ret = -EFAULT;
> >               goto err_free;
> >       }
> >
> > +     id->size = size;
>
> So, if we did use id->size after the copying, we'd indeed have this line
> in the code. But since we don't, it's also pointless, so it's not there.
>
> Thanks,
> --
> Alex