public inbox for linuxppc-dev@ozlabs.org 
 help / color / mirror / Atom feed

From: Michael Ellerman <mpe@ellerman•id.au>
To: Mathieu Malaterre <malat@debian•org>
Cc: linuxppc-dev@ozlabs•org, Meelis Roos <mroos@linux•ee>
Subject: Re: [PATCH] powerpc/ptrace: Simplify vr_get/set() to avoid GCC warning
Date: Fri, 15 Feb 2019 20:20:16 +1100	[thread overview]
Message-ID: <87pnrtcsrz.fsf@concordia.ellerman.id.au> (raw)
In-Reply-To: <CA+7wUszvErrmyo9jAuPk0dtvwKd3cw4jSb04z5Q4DkGEjGiY7w@mail.gmail.com>

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>

next prev parent reply	other threads:[~2019-02-15  9:22 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
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 [this message]
2019-02-16 19:15 ` Meelis Roos
2019-02-19  1:02   ` Michael Ellerman
2019-02-22  9:47 ` Michael Ellerman

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87pnrtcsrz.fsf@concordia.ellerman.id.au \
    --to=mpe@ellerman$(echo .)id.au \
    --cc=linuxppc-dev@ozlabs$(echo .)org \
    --cc=malat@debian$(echo .)org \
    --cc=mroos@linux$(echo .)ee \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox