[PATCH] Errenous use of memset and memcpy

[PATCH] Errenous use of memset and memcpy

[Alexander Nyberg]

I stumbled across this, looks weird. Presumable fix is:

=== arch/ia64/ia32/ia32_signal.c 1.35 vs edited ==--- 1.35/arch/ia64/ia32/ia32_signal.c	2005-01-25 21:23:45 +01:00
+++ edited/arch/ia64/ia32/ia32_signal.c	2005-02-12 13:12:25 +01:00
@@ -460,10 +460,9 @@ __ia32_rt_sigsuspend (compat_sigset_t *s
 	sigset_t oldset, set;
 
 	scr->scratch_unat = 0;	/* avoid leaking kernel bits to user level */
-	memset(&set, 0, sizeof(&set));
+	memset(&set, 0, sizeof(sigset_t));
 
-	if (memcpy(&set.sig, &sset->sig, sigsetsize))
-		return -EFAULT;
+	memcpy(&set.sig, &sset->sig, sigsetsize);
 
 	sigdelsetmask(&set, ~_BLOCKABLE);
 

Re: [PATCH] Errenous use of memset and memcpy

[Andreas Schwab]

Alexander Nyberg <alexn@dsv.su.se> writes:

> I stumbled across this, looks weird. Presumable fix is:
>
> === arch/ia64/ia32/ia32_signal.c 1.35 vs edited ==> --- 1.35/arch/ia64/ia32/ia32_signal.c	2005-01-25 21:23:45 +01:00
> +++ edited/arch/ia64/ia32/ia32_signal.c	2005-02-12 13:12:25 +01:00
> @@ -460,10 +460,9 @@ __ia32_rt_sigsuspend (compat_sigset_t *s
>  	sigset_t oldset, set;
>  
>  	scr->scratch_unat = 0;	/* avoid leaking kernel bits to user level */
> -	memset(&set, 0, sizeof(&set));
> +	memset(&set, 0, sizeof(sigset_t));
>  
> -	if (memcpy(&set.sig, &sset->sig, sigsetsize))
> -		return -EFAULT;
> +	memcpy(&set.sig, &sset->sig, sigsetsize);

That should be copy_from_user, and the error checking needs to stay.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH] Errenous use of memset and memcpy

[Alexander Nyberg]

> That should be copy_from_user, and the error checking needs to stay.
> 

Makes sense, thanks.

=== arch/ia64/ia32/ia32_signal.c 1.35 vs edited ==--- 1.35/arch/ia64/ia32/ia32_signal.c	2005-01-25 21:23:45 +01:00
+++ edited/arch/ia64/ia32/ia32_signal.c	2005-02-20 11:32:55 +01:00
@@ -460,9 +460,9 @@ __ia32_rt_sigsuspend (compat_sigset_t *s
 	sigset_t oldset, set;
 
 	scr->scratch_unat = 0;	/* avoid leaking kernel bits to user level */
-	memset(&set, 0, sizeof(&set));
+	memset(&set, 0, sizeof(sigset_t));
 
-	if (memcpy(&set.sig, &sset->sig, sigsetsize))
+	if (copy_from_user(&set.sig, &sset->sig, sigsetsize))
 		return -EFAULT;
 
 	sigdelsetmask(&set, ~_BLOCKABLE);

Re: [PATCH] Errenous use of memset and memcpy

[Bjorn Helgaas]

On Sun, 2005-02-20 at 11:38 +0100, Alexander Nyberg wrote:
> === arch/ia64/ia32/ia32_signal.c 1.35 vs edited ==> --- 1.35/arch/ia64/ia32/ia32_signal.c	2005-01-25 21:23:45 +01:00
> +++ edited/arch/ia64/ia32/ia32_signal.c	2005-02-20 11:32:55 +01:00
> @@ -460,9 +460,9 @@ __ia32_rt_sigsuspend (compat_sigset_t *s
>  	sigset_t oldset, set;
>  
>  	scr->scratch_unat = 0;	/* avoid leaking kernel bits to user level */
> -	memset(&set, 0, sizeof(&set));
> +	memset(&set, 0, sizeof(sigset_t));

To check that the above is correct, you have to go look up
the declaration of "set".  Why not do this:

+	memset(&set, 0, sizeof(set));

which is clearly correct, regardless of the declaration?

Re: [PATCH] Errenous use of memset and memcpy

[Arun Sharma]

[-- Attachment #1: Type: text/plain, Size: 846 bytes --]

On 2/22/2005 9:37 AM, Bjorn Helgaas wrote:
> On Sun, 2005-02-20 at 11:38 +0100, Alexander Nyberg wrote:
>> ===== arch/ia64/ia32/ia32_signal.c 1.35 vs edited =====
>> --- 1.35/arch/ia64/ia32/ia32_signal.c	2005-01-25 21:23:45 +01:00
>> +++ edited/arch/ia64/ia32/ia32_signal.c	2005-02-20 11:32:55 +01:00
>> @@ -460,9 +460,9 @@ __ia32_rt_sigsuspend (compat_sigset_t *s
>>  	sigset_t oldset, set;
>>  
>>  	scr->scratch_unat = 0;	/* avoid leaking kernel bits to user level */
>> -	memset(&set, 0, sizeof(&set));
>> +	memset(&set, 0, sizeof(sigset_t));
> 
> To check that the above is correct, you have to go look up
> the declaration of "set".  Why not do this:
> 
> +	memset(&set, 0, sizeof(set));
> 
> which is clearly correct, regardless of the declaration?
>

Agree and I think memcpy is just fine. Tony, please apply the attached patch.

	-Arun


[-- Attachment #2: sigsuspend-memcpy.patch --]
[-- Type: text/plain, Size: 662 bytes --]

- Fix the incorrect argument to sizeof()
- memcpy should always succeed because it's a kernel space to kernel space copy.

Signed-off-by: Arun Sharma <arun.sharma@intel.com>

--- linux-2.6-cvs/arch/ia64/ia32/ia32_signal.c	3 Feb 2005 18:29:45 -0000	1.25
+++ linux-2.6-cvs/arch/ia64/ia32/ia32_signal.c	2 Mar 2005 23:43:09 -0000
@@ -460,10 +460,9 @@
 	sigset_t oldset, set;
 
 	scr->scratch_unat = 0;	/* avoid leaking kernel bits to user level */
-	memset(&set, 0, sizeof(&set));
+	memset(&set, 0, sizeof(set));
 
-	if (memcpy(&set.sig, &sset->sig, sigsetsize))
-		return -EFAULT;
+	memcpy(&set.sig, &sset->sig, sigsetsize);
 
 	sigdelsetmask(&set, ~_BLOCKABLE);