* Re: [PATCH] Errenous use of memset and memcpy
2005-02-19 21:25 [PATCH] Errenous use of memset and memcpy Alexander Nyberg
@ 2005-02-19 21:55 ` Andreas Schwab
2005-02-20 10:38 ` Alexander Nyberg
` (2 subsequent siblings)
3 siblings, 0 replies; 5+ messages in thread
From: Andreas Schwab @ 2005-02-19 21:55 UTC (permalink / raw)
To: linux-ia64
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."
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] Errenous use of memset and memcpy
2005-02-19 21:25 [PATCH] Errenous use of memset and memcpy Alexander Nyberg
2005-02-19 21:55 ` Andreas Schwab
@ 2005-02-20 10:38 ` Alexander Nyberg
2005-02-22 17:37 ` Bjorn Helgaas
2005-03-03 20:53 ` Arun Sharma
3 siblings, 0 replies; 5+ messages in thread
From: Alexander Nyberg @ 2005-02-20 10:38 UTC (permalink / raw)
To: linux-ia64
> 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);
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Errenous use of memset and memcpy
2005-02-19 21:25 [PATCH] Errenous use of memset and memcpy Alexander Nyberg
2005-02-19 21:55 ` Andreas Schwab
2005-02-20 10:38 ` Alexander Nyberg
@ 2005-02-22 17:37 ` Bjorn Helgaas
2005-03-03 20:53 ` Arun Sharma
3 siblings, 0 replies; 5+ messages in thread
From: Bjorn Helgaas @ 2005-02-22 17:37 UTC (permalink / raw)
To: linux-ia64
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?
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Errenous use of memset and memcpy
2005-02-19 21:25 [PATCH] Errenous use of memset and memcpy Alexander Nyberg
` (2 preceding siblings ...)
2005-02-22 17:37 ` Bjorn Helgaas
@ 2005-03-03 20:53 ` Arun Sharma
3 siblings, 0 replies; 5+ messages in thread
From: Arun Sharma @ 2005-03-03 20:53 UTC (permalink / raw)
To: linux-ia64
[-- 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);
^ permalink raw reply [flat|nested] 5+ messages in thread