* [PATCH] powerpc/ptrace: Simplify vr_get/set() to avoid GCC warning
@ 2019-02-15 6:14 Michael Ellerman
2019-02-15 8:09 ` Mathieu Malaterre
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Michael Ellerman @ 2019-02-15 6:14 UTC (permalink / raw)
To: linuxppc-dev; +Cc: malat, mroos
GCC 8 warns about the logic in vr_get/set(), which with -Werror breaks
the build:
In function ‘user_regset_copyin’,
inlined from ‘vr_set’ at arch/powerpc/kernel/ptrace.c:628:9:
include/linux/regset.h:295:4: error: ‘memcpy’ offset [-527, -529] is
out of the bounds [0, 16] of object ‘vrsave’ with type ‘union
<anonymous>’ [-Werror=array-bounds]
arch/powerpc/kernel/ptrace.c: In function ‘vr_set’:
arch/powerpc/kernel/ptrace.c:623:5: note: ‘vrsave’ declared here
} vrsave;
This has been identified as a regression in GCC, see GCC bug 88273.
However we can avoid the warning and also simplify the logic and make
it more robust.
Currently we pass -1 as end_pos to user_regset_copyout(). This says
"copy up to the end of the regset".
The definition of the regset is:
[REGSET_VMX] = {
.core_note_type = NT_PPC_VMX, .n = 34,
.size = sizeof(vector128), .align = sizeof(vector128),
.active = vr_active, .get = vr_get, .set = vr_set
},
The end is calculated as (n * size), ie. 34 * sizeof(vector128).
In vr_get/set() we pass start_pos as 33 * sizeof(vector128), meaning
we can copy up to sizeof(vector128) into/out-of vrsave.
The on-stack vrsave is defined as:
union {
elf_vrreg_t reg;
u32 word;
} vrsave;
And elf_vrreg_t is:
typedef __vector128 elf_vrreg_t;
So there is no bug, but we rely on all those sizes lining up,
otherwise we would have a kernel stack exposure/overwrite on our
hands.
Rather than relying on that we can pass an explict end_pos based on
the sizeof(vrsave). The result should be exactly the same but it's
more obviously not over-reading/writing the stack and it avoids the
compiler warning.
Reported-by: Meelis Roos <mroos@linux.ee>
Reported-by: Mathieu Malaterre <malat@debian.org>
Cc: stable@vger.kernel.org
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
arch/powerpc/kernel/ptrace.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
index 7535f89e08cd..d9ac7d94656e 100644
--- a/arch/powerpc/kernel/ptrace.c
+++ b/arch/powerpc/kernel/ptrace.c
@@ -567,6 +567,7 @@ static int vr_get(struct task_struct *target, const struct user_regset *regset,
/*
* Copy out only the low-order word of vrsave.
*/
+ int start, end;
union {
elf_vrreg_t reg;
u32 word;
@@ -575,8 +576,10 @@ static int vr_get(struct task_struct *target, const struct user_regset *regset,
vrsave.word = target->thread.vrsave;
+ start = 33 * sizeof(vector128);
+ end = start + sizeof(vrsave);
ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf, &vrsave,
- 33 * sizeof(vector128), -1);
+ start, end);
}
return ret;
@@ -614,6 +617,7 @@ static int vr_set(struct task_struct *target, const struct user_regset *regset,
/*
* We use only the first word of vrsave.
*/
+ int start, end;
union {
elf_vrreg_t reg;
u32 word;
@@ -622,8 +626,10 @@ static int vr_set(struct task_struct *target, const struct user_regset *regset,
vrsave.word = target->thread.vrsave;
+ start = 33 * sizeof(vector128);
+ end = start + sizeof(vrsave);
ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf, &vrsave,
- 33 * sizeof(vector128), -1);
+ start, end);
if (!ret)
target->thread.vrsave = vrsave.word;
}
--
2.20.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] powerpc/ptrace: Simplify vr_get/set() to avoid GCC warning
2019-02-15 6:14 [PATCH] powerpc/ptrace: Simplify vr_get/set() to avoid GCC warning Michael Ellerman
@ 2019-02-15 8:09 ` Mathieu Malaterre
2019-02-15 9:20 ` Michael Ellerman
2019-02-16 19:15 ` Meelis Roos
2019-02-22 9:47 ` Michael Ellerman
2 siblings, 1 reply; 6+ messages in thread
From: Mathieu Malaterre @ 2019-02-15 8:09 UTC (permalink / raw)
To: Michael Ellerman; +Cc: linuxppc-dev, Meelis Roos
On Fri, Feb 15, 2019 at 7:14 AM Michael Ellerman <mpe@ellerman.id.au> wrote:
>
> GCC 8 warns about the logic in vr_get/set(), which with -Werror breaks
> the build:
>
> In function ‘user_regset_copyin’,
> inlined from ‘vr_set’ at arch/powerpc/kernel/ptrace.c:628:9:
> include/linux/regset.h:295:4: error: ‘memcpy’ offset [-527, -529] is
> out of the bounds [0, 16] of object ‘vrsave’ with type ‘union
> <anonymous>’ [-Werror=array-bounds]
> arch/powerpc/kernel/ptrace.c: In function ‘vr_set’:
> arch/powerpc/kernel/ptrace.c:623:5: note: ‘vrsave’ declared here
> } vrsave;
>
> This has been identified as a regression in GCC, see GCC bug 88273.
Good point, this does not seems this will be backported.
> However we can avoid the warning and also simplify the logic and make
> it more robust.
>
> Currently we pass -1 as end_pos to user_regset_copyout(). This says
> "copy up to the end of the regset".
>
> The definition of the regset is:
> [REGSET_VMX] = {
> .core_note_type = NT_PPC_VMX, .n = 34,
> .size = sizeof(vector128), .align = sizeof(vector128),
> .active = vr_active, .get = vr_get, .set = vr_set
> },
>
> The end is calculated as (n * size), ie. 34 * sizeof(vector128).
>
> In vr_get/set() we pass start_pos as 33 * sizeof(vector128), meaning
> we can copy up to sizeof(vector128) into/out-of vrsave.
>
> The on-stack vrsave is defined as:
> union {
> elf_vrreg_t reg;
> u32 word;
> } vrsave;
>
> And elf_vrreg_t is:
> typedef __vector128 elf_vrreg_t;
>
> So there is no bug, but we rely on all those sizes lining up,
> otherwise we would have a kernel stack exposure/overwrite on our
> hands.
>
> Rather than relying on that we can pass an explict end_pos based on
> the sizeof(vrsave). The result should be exactly the same but it's
> more obviously not over-reading/writing the stack and it avoids the
> compiler warning.
>
maybe:
Link: https://lkml.org/lkml/2018/8/16/117
In any case the warning is now gone:
Tested-by: Mathieu Malaterre <malat@debian.org>
> Reported-by: Meelis Roos <mroos@linux.ee>
> Reported-by: Mathieu Malaterre <malat@debian.org>
> Cc: stable@vger.kernel.org
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> ---
> arch/powerpc/kernel/ptrace.c | 10 ++++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
> index 7535f89e08cd..d9ac7d94656e 100644
> --- a/arch/powerpc/kernel/ptrace.c
> +++ b/arch/powerpc/kernel/ptrace.c
> @@ -567,6 +567,7 @@ static int vr_get(struct task_struct *target, const struct user_regset *regset,
> /*
> * Copy out only the low-order word of vrsave.
> */
> + int start, end;
> union {
> elf_vrreg_t reg;
> u32 word;
> @@ -575,8 +576,10 @@ static int vr_get(struct task_struct *target, const struct user_regset *regset,
>
> vrsave.word = target->thread.vrsave;
>
> + start = 33 * sizeof(vector128);
> + end = start + sizeof(vrsave);
> ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf, &vrsave,
> - 33 * sizeof(vector128), -1);
> + start, end);
> }
>
> return ret;
> @@ -614,6 +617,7 @@ static int vr_set(struct task_struct *target, const struct user_regset *regset,
> /*
> * We use only the first word of vrsave.
> */
> + int start, end;
> union {
> elf_vrreg_t reg;
> u32 word;
> @@ -622,8 +626,10 @@ static int vr_set(struct task_struct *target, const struct user_regset *regset,
>
> vrsave.word = target->thread.vrsave;
>
> + start = 33 * sizeof(vector128);
> + end = start + sizeof(vrsave);
> ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf, &vrsave,
> - 33 * sizeof(vector128), -1);
> + start, end);
> if (!ret)
> target->thread.vrsave = vrsave.word;
> }
> --
> 2.20.1
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] powerpc/ptrace: Simplify vr_get/set() to avoid GCC warning
2019-02-15 8:09 ` Mathieu Malaterre
@ 2019-02-15 9:20 ` Michael Ellerman
0 siblings, 0 replies; 6+ messages in thread
From: Michael Ellerman @ 2019-02-15 9:20 UTC (permalink / raw)
To: Mathieu Malaterre; +Cc: linuxppc-dev, Meelis Roos
Mathieu Malaterre <malat@debian.org> writes:
> On Fri, Feb 15, 2019 at 7:14 AM Michael Ellerman <mpe@ellerman.id.au> wrote:
>>
>> GCC 8 warns about the logic in vr_get/set(), which with -Werror breaks
>> the build:
>>
>> In function ‘user_regset_copyin’,
>> inlined from ‘vr_set’ at arch/powerpc/kernel/ptrace.c:628:9:
>> include/linux/regset.h:295:4: error: ‘memcpy’ offset [-527, -529] is
>> out of the bounds [0, 16] of object ‘vrsave’ with type ‘union
>> <anonymous>’ [-Werror=array-bounds]
>> arch/powerpc/kernel/ptrace.c: In function ‘vr_set’:
>> arch/powerpc/kernel/ptrace.c:623:5: note: ‘vrsave’ declared here
>> } vrsave;
>>
>> This has been identified as a regression in GCC, see GCC bug 88273.
>
> Good point, this does not seems this will be backported.
>
>> However we can avoid the warning and also simplify the logic and make
>> it more robust.
>>
>> Currently we pass -1 as end_pos to user_regset_copyout(). This says
>> "copy up to the end of the regset".
>>
>> The definition of the regset is:
>> [REGSET_VMX] = {
>> .core_note_type = NT_PPC_VMX, .n = 34,
>> .size = sizeof(vector128), .align = sizeof(vector128),
>> .active = vr_active, .get = vr_get, .set = vr_set
>> },
>>
>> The end is calculated as (n * size), ie. 34 * sizeof(vector128).
>>
>> In vr_get/set() we pass start_pos as 33 * sizeof(vector128), meaning
>> we can copy up to sizeof(vector128) into/out-of vrsave.
>>
>> The on-stack vrsave is defined as:
>> union {
>> elf_vrreg_t reg;
>> u32 word;
>> } vrsave;
>>
>> And elf_vrreg_t is:
>> typedef __vector128 elf_vrreg_t;
>>
>> So there is no bug, but we rely on all those sizes lining up,
>> otherwise we would have a kernel stack exposure/overwrite on our
>> hands.
>>
>> Rather than relying on that we can pass an explict end_pos based on
>> the sizeof(vrsave). The result should be exactly the same but it's
>> more obviously not over-reading/writing the stack and it avoids the
>> compiler warning.
>>
>
> maybe:
>
> Link: https://lkml.org/lkml/2018/8/16/117
Hmm, I prefer not to include links because they're unlikely to last
forever like the git history.
If we do include them the preferred form is a link to lkml.kernel.org
using the message id. That way the message is recorded and also the link
is "guaranteed" to work in future, eg:
http://lkml.kernel.org/r/alpine.LRH.2.21.1808161041350.16413@math.ut.ee
In this case I don't think the link adds much over what I have in the
change log, in particular I did credit Meelis as the reporter.
> In any case the warning is now gone:
>
> Tested-by: Mathieu Malaterre <malat@debian.org>
Thanks.
cheers
>> Reported-by: Meelis Roos <mroos@linux.ee>
>> Reported-by: Mathieu Malaterre <malat@debian.org>
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] powerpc/ptrace: Simplify vr_get/set() to avoid GCC warning
2019-02-15 6:14 [PATCH] powerpc/ptrace: Simplify vr_get/set() to avoid GCC warning Michael Ellerman
2019-02-15 8:09 ` Mathieu Malaterre
@ 2019-02-16 19:15 ` Meelis Roos
2019-02-19 1:02 ` Michael Ellerman
2019-02-22 9:47 ` Michael Ellerman
2 siblings, 1 reply; 6+ messages in thread
From: Meelis Roos @ 2019-02-16 19:15 UTC (permalink / raw)
To: Michael Ellerman, linuxppc-dev; +Cc: malat
> Rather than relying on that we can pass an explict end_pos based on
> the sizeof(vrsave). The result should be exactly the same but it's
> more obviously not over-reading/writing the stack and it avoids the
> compiler warning.
It works on my PowerMac G4 with Debian-ports unstable with gcc 8.
--
Meelis Roos
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] powerpc/ptrace: Simplify vr_get/set() to avoid GCC warning
2019-02-16 19:15 ` Meelis Roos
@ 2019-02-19 1:02 ` Michael Ellerman
0 siblings, 0 replies; 6+ messages in thread
From: Michael Ellerman @ 2019-02-19 1:02 UTC (permalink / raw)
To: Meelis Roos, linuxppc-dev; +Cc: malat
Meelis Roos <mroos@linux.ee> writes:
>> Rather than relying on that we can pass an explict end_pos based on
>> the sizeof(vrsave). The result should be exactly the same but it's
>> more obviously not over-reading/writing the stack and it avoids the
>> compiler warning.
>
> It works on my PowerMac G4 with Debian-ports unstable with gcc 8.
Thanks.
cheers
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: powerpc/ptrace: Simplify vr_get/set() to avoid GCC warning
2019-02-15 6:14 [PATCH] powerpc/ptrace: Simplify vr_get/set() to avoid GCC warning Michael Ellerman
2019-02-15 8:09 ` Mathieu Malaterre
2019-02-16 19:15 ` Meelis Roos
@ 2019-02-22 9:47 ` Michael Ellerman
2 siblings, 0 replies; 6+ messages in thread
From: Michael Ellerman @ 2019-02-22 9:47 UTC (permalink / raw)
To: Michael Ellerman, linuxppc-dev; +Cc: malat, mroos
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 2220 bytes --]
On Fri, 2019-02-15 at 06:14:00 UTC, Michael Ellerman wrote:
> GCC 8 warns about the logic in vr_get/set(), which with -Werror breaks
> the build:
>
> In function âuser_regset_copyinâ,
> inlined from âvr_setâ at arch/powerpc/kernel/ptrace.c:628:9:
> include/linux/regset.h:295:4: error: âmemcpyâ offset [-527, -529] is
> out of the bounds [0, 16] of object âvrsaveâ with type âunion
> <anonymous>â [-Werror=array-bounds]
> arch/powerpc/kernel/ptrace.c: In function âvr_setâ:
> arch/powerpc/kernel/ptrace.c:623:5: note: âvrsaveâ declared here
> } vrsave;
>
> This has been identified as a regression in GCC, see GCC bug 88273.
>
> However we can avoid the warning and also simplify the logic and make
> it more robust.
>
> Currently we pass -1 as end_pos to user_regset_copyout(). This says
> "copy up to the end of the regset".
>
> The definition of the regset is:
> [REGSET_VMX] = {
> .core_note_type = NT_PPC_VMX, .n = 34,
> .size = sizeof(vector128), .align = sizeof(vector128),
> .active = vr_active, .get = vr_get, .set = vr_set
> },
>
> The end is calculated as (n * size), ie. 34 * sizeof(vector128).
>
> In vr_get/set() we pass start_pos as 33 * sizeof(vector128), meaning
> we can copy up to sizeof(vector128) into/out-of vrsave.
>
> The on-stack vrsave is defined as:
> union {
> elf_vrreg_t reg;
> u32 word;
> } vrsave;
>
> And elf_vrreg_t is:
> typedef __vector128 elf_vrreg_t;
>
> So there is no bug, but we rely on all those sizes lining up,
> otherwise we would have a kernel stack exposure/overwrite on our
> hands.
>
> Rather than relying on that we can pass an explict end_pos based on
> the sizeof(vrsave). The result should be exactly the same but it's
> more obviously not over-reading/writing the stack and it avoids the
> compiler warning.
>
> Reported-by: Meelis Roos <mroos@linux.ee>
> Reported-by: Mathieu Malaterre <malat@debian.org>
> Cc: stable@vger.kernel.org
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> Tested-by: Mathieu Malaterre <malat@debian.org>
Applied to powerpc next.
https://git.kernel.org/powerpc/c/ca6d5149d2ad0a8d2f9c28cbe3798022
cheers
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-02-22 10:32 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-15 6:14 [PATCH] powerpc/ptrace: Simplify vr_get/set() to avoid GCC warning Michael Ellerman
2019-02-15 8:09 ` Mathieu Malaterre
2019-02-15 9:20 ` Michael Ellerman
2019-02-16 19:15 ` Meelis Roos
2019-02-19 1:02 ` Michael Ellerman
2019-02-22 9:47 ` Michael Ellerman
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).