* [PATCH] powerpc/signal32: Use struct_group() to zero spe regs
@ 2021-11-18 20:36 ` Kees Cook
0 siblings, 0 replies; 19+ messages in thread
From: Kees Cook @ 2021-11-18 20:36 UTC (permalink / raw)
To: Michael Ellerman
Cc: Kees Cook, kernel test robot, Benjamin Herrenschmidt,
Paul Mackerras, Christophe Leroy, Sudeep Holla, Peter Zijlstra,
Nicholas Piggin, Aneesh Kumar K.V, Eric W. Biederman,
linux-kernel, linuxppc-dev, linux-hardening
In preparation for FORTIFY_SOURCE performing compile-time and run-time
field bounds checking for memset(), avoid intentionally writing across
neighboring fields.
Add a struct_group() for the spe registers so that memset() can correctly reason
about the size:
In function 'fortify_memset_chk',
inlined from 'restore_user_regs.part.0' at arch/powerpc/kernel/signal_32.c:539:3:
>> include/linux/fortify-string.h:195:4: error: call to '__write_overflow_field' declared with attribute warning: detected write beyond size of field (1st parameter); maybe use struct_group()? [-Werror=attribute-warning]
195 | __write_overflow_field();
| ^~~~~~~~~~~~~~~~~~~~~~~~
Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
---
arch/powerpc/include/asm/processor.h | 6 ++++--
arch/powerpc/kernel/signal_32.c | 14 +++++++++-----
2 files changed, 13 insertions(+), 7 deletions(-)
diff --git a/arch/powerpc/include/asm/processor.h b/arch/powerpc/include/asm/processor.h
index e39bd0ff69f3..978a80308466 100644
--- a/arch/powerpc/include/asm/processor.h
+++ b/arch/powerpc/include/asm/processor.h
@@ -191,8 +191,10 @@ struct thread_struct {
int used_vsr; /* set if process has used VSX */
#endif /* CONFIG_VSX */
#ifdef CONFIG_SPE
- unsigned long evr[32]; /* upper 32-bits of SPE regs */
- u64 acc; /* Accumulator */
+ struct_group(spe,
+ unsigned long evr[32]; /* upper 32-bits of SPE regs */
+ u64 acc; /* Accumulator */
+ );
unsigned long spefscr; /* SPE & eFP status */
unsigned long spefscr_last; /* SPEFSCR value on last prctl
call or trap return */
diff --git a/arch/powerpc/kernel/signal_32.c b/arch/powerpc/kernel/signal_32.c
index 00a9c9cd6d42..5e1664b501e4 100644
--- a/arch/powerpc/kernel/signal_32.c
+++ b/arch/powerpc/kernel/signal_32.c
@@ -527,16 +527,20 @@ static long restore_user_regs(struct pt_regs *regs,
regs_set_return_msr(regs, regs->msr & ~(MSR_FP | MSR_FE0 | MSR_FE1));
#ifdef CONFIG_SPE
- /* force the process to reload the spe registers from
- current->thread when it next does spe instructions */
+ /*
+ * Force the process to reload the spe registers from
+ * current->thread when it next does spe instructions.
+ * Since this is user ABI, we must enforce the sizing.
+ */
+ BUILD_BUG_ON(sizeof(current->thread.spe) != ELF_NEVRREG * sizeof(u32));
regs_set_return_msr(regs, regs->msr & ~MSR_SPE);
if (msr & MSR_SPE) {
/* restore spe registers from the stack */
- unsafe_copy_from_user(current->thread.evr, &sr->mc_vregs,
- ELF_NEVRREG * sizeof(u32), failed);
+ unsafe_copy_from_user(¤t->thread.spe, &sr->mc_vregs,
+ sizeof(current->thread.spe), failed);
current->thread.used_spe = true;
} else if (current->thread.used_spe)
- memset(current->thread.evr, 0, ELF_NEVRREG * sizeof(u32));
+ memset(¤t->thread.spe, 0, sizeof(current->thread.spe));
/* Always get SPEFSCR back */
unsafe_get_user(current->thread.spefscr, (u32 __user *)&sr->mc_vregs + ELF_NEVRREG, failed);
--
2.30.2
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH] powerpc/signal32: Use struct_group() to zero spe regs
@ 2021-11-18 20:36 ` Kees Cook
0 siblings, 0 replies; 19+ messages in thread
From: Kees Cook @ 2021-11-18 20:36 UTC (permalink / raw)
To: Michael Ellerman
Cc: Aneesh Kumar K.V, Eric W. Biederman, Kees Cook, Peter Zijlstra,
linux-kernel, linux-hardening, Paul Mackerras, Nicholas Piggin,
Sudeep Holla, linuxppc-dev, kernel test robot
In preparation for FORTIFY_SOURCE performing compile-time and run-time
field bounds checking for memset(), avoid intentionally writing across
neighboring fields.
Add a struct_group() for the spe registers so that memset() can correctly reason
about the size:
In function 'fortify_memset_chk',
inlined from 'restore_user_regs.part.0' at arch/powerpc/kernel/signal_32.c:539:3:
>> include/linux/fortify-string.h:195:4: error: call to '__write_overflow_field' declared with attribute warning: detected write beyond size of field (1st parameter); maybe use struct_group()? [-Werror=attribute-warning]
195 | __write_overflow_field();
| ^~~~~~~~~~~~~~~~~~~~~~~~
Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
---
arch/powerpc/include/asm/processor.h | 6 ++++--
arch/powerpc/kernel/signal_32.c | 14 +++++++++-----
2 files changed, 13 insertions(+), 7 deletions(-)
diff --git a/arch/powerpc/include/asm/processor.h b/arch/powerpc/include/asm/processor.h
index e39bd0ff69f3..978a80308466 100644
--- a/arch/powerpc/include/asm/processor.h
+++ b/arch/powerpc/include/asm/processor.h
@@ -191,8 +191,10 @@ struct thread_struct {
int used_vsr; /* set if process has used VSX */
#endif /* CONFIG_VSX */
#ifdef CONFIG_SPE
- unsigned long evr[32]; /* upper 32-bits of SPE regs */
- u64 acc; /* Accumulator */
+ struct_group(spe,
+ unsigned long evr[32]; /* upper 32-bits of SPE regs */
+ u64 acc; /* Accumulator */
+ );
unsigned long spefscr; /* SPE & eFP status */
unsigned long spefscr_last; /* SPEFSCR value on last prctl
call or trap return */
diff --git a/arch/powerpc/kernel/signal_32.c b/arch/powerpc/kernel/signal_32.c
index 00a9c9cd6d42..5e1664b501e4 100644
--- a/arch/powerpc/kernel/signal_32.c
+++ b/arch/powerpc/kernel/signal_32.c
@@ -527,16 +527,20 @@ static long restore_user_regs(struct pt_regs *regs,
regs_set_return_msr(regs, regs->msr & ~(MSR_FP | MSR_FE0 | MSR_FE1));
#ifdef CONFIG_SPE
- /* force the process to reload the spe registers from
- current->thread when it next does spe instructions */
+ /*
+ * Force the process to reload the spe registers from
+ * current->thread when it next does spe instructions.
+ * Since this is user ABI, we must enforce the sizing.
+ */
+ BUILD_BUG_ON(sizeof(current->thread.spe) != ELF_NEVRREG * sizeof(u32));
regs_set_return_msr(regs, regs->msr & ~MSR_SPE);
if (msr & MSR_SPE) {
/* restore spe registers from the stack */
- unsafe_copy_from_user(current->thread.evr, &sr->mc_vregs,
- ELF_NEVRREG * sizeof(u32), failed);
+ unsafe_copy_from_user(¤t->thread.spe, &sr->mc_vregs,
+ sizeof(current->thread.spe), failed);
current->thread.used_spe = true;
} else if (current->thread.used_spe)
- memset(current->thread.evr, 0, ELF_NEVRREG * sizeof(u32));
+ memset(¤t->thread.spe, 0, sizeof(current->thread.spe));
/* Always get SPEFSCR back */
unsafe_get_user(current->thread.spefscr, (u32 __user *)&sr->mc_vregs + ELF_NEVRREG, failed);
--
2.30.2
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH] powerpc/signal32: Use struct_group() to zero spe regs
2021-11-18 20:36 ` Kees Cook
(?)
@ 2021-11-19 8:46 ` LEROY Christophe
2021-11-19 16:28 ` Kees Cook
2021-11-22 5:43 ` Michael Ellerman
-1 siblings, 2 replies; 19+ messages in thread
From: LEROY Christophe @ 2021-11-19 8:46 UTC (permalink / raw)
To: Kees Cook, Michael Ellerman
Cc: kernel test robot, Peter Zijlstra, linux-kernel, Nicholas Piggin,
linux-hardening, Paul Mackerras, Aneesh Kumar K.V, Sudeep Holla,
linuxppc-dev, Eric W. Biederman
Le 18/11/2021 à 21:36, Kees Cook a écrit :
> In preparation for FORTIFY_SOURCE performing compile-time and run-time
> field bounds checking for memset(), avoid intentionally writing across
> neighboring fields.
>
> Add a struct_group() for the spe registers so that memset() can correctly reason
> about the size:
>
> In function 'fortify_memset_chk',
> inlined from 'restore_user_regs.part.0' at arch/powerpc/kernel/signal_32.c:539:3:
> >> include/linux/fortify-string.h:195:4: error: call to '__write_overflow_field' declared with attribute warning: detected write beyond size of field (1st parameter); maybe use struct_group()? [-Werror=attribute-warning]
> 195 | __write_overflow_field();
> | ^~~~~~~~~~~~~~~~~~~~~~~~
>
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Kees Cook <keescook@chromium.org>
Reviewed-by: Christophe Leroy <christophe.leroy@csgroup.eu>
However, is it really worth adding that grouping ? Wouldn't it be
cleaner to handle evr[] and acc separately ? Now that we are using
unsafe variants of get/put user performance wouldn't be impacted.
I have some doubts about things like:
unsafe_copy_to_user(&frame->mc_vregs, current->thread.evr,
ELF_NEVRREG * sizeof(u32), failed);
Because as far as I can see, ELF_NEVRREG is 34 but mc_vregs is a table
of 33 u32 and is at the end of the structure:
struct mcontext {
elf_gregset_t mc_gregs;
elf_fpregset_t mc_fregs;
unsigned long mc_pad[2];
elf_vrregset_t mc_vregs __attribute__((__aligned__(16)));
};
typedef elf_vrreg_t elf_vrregset_t[ELF_NVRREG];
# define ELF_NEVRREG 34 /* includes acc (as 2) */
# define ELF_NVRREG 33 /* includes vscr */
> ---
> arch/powerpc/include/asm/processor.h | 6 ++++--
> arch/powerpc/kernel/signal_32.c | 14 +++++++++-----
> 2 files changed, 13 insertions(+), 7 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/processor.h b/arch/powerpc/include/asm/processor.h
> index e39bd0ff69f3..978a80308466 100644
> --- a/arch/powerpc/include/asm/processor.h
> +++ b/arch/powerpc/include/asm/processor.h
> @@ -191,8 +191,10 @@ struct thread_struct {
> int used_vsr; /* set if process has used VSX */
> #endif /* CONFIG_VSX */
> #ifdef CONFIG_SPE
> - unsigned long evr[32]; /* upper 32-bits of SPE regs */
> - u64 acc; /* Accumulator */
> + struct_group(spe,
> + unsigned long evr[32]; /* upper 32-bits of SPE regs */
> + u64 acc; /* Accumulator */
> + );
> unsigned long spefscr; /* SPE & eFP status */
> unsigned long spefscr_last; /* SPEFSCR value on last prctl
> call or trap return */
> diff --git a/arch/powerpc/kernel/signal_32.c b/arch/powerpc/kernel/signal_32.c
> index 00a9c9cd6d42..5e1664b501e4 100644
> --- a/arch/powerpc/kernel/signal_32.c
> +++ b/arch/powerpc/kernel/signal_32.c
> @@ -527,16 +527,20 @@ static long restore_user_regs(struct pt_regs *regs,
> regs_set_return_msr(regs, regs->msr & ~(MSR_FP | MSR_FE0 | MSR_FE1));
>
> #ifdef CONFIG_SPE
> - /* force the process to reload the spe registers from
> - current->thread when it next does spe instructions */
> + /*
> + * Force the process to reload the spe registers from
> + * current->thread when it next does spe instructions.
> + * Since this is user ABI, we must enforce the sizing.
> + */
> + BUILD_BUG_ON(sizeof(current->thread.spe) != ELF_NEVRREG * sizeof(u32));
> regs_set_return_msr(regs, regs->msr & ~MSR_SPE);
> if (msr & MSR_SPE) {
> /* restore spe registers from the stack */
> - unsafe_copy_from_user(current->thread.evr, &sr->mc_vregs,
> - ELF_NEVRREG * sizeof(u32), failed);
> + unsafe_copy_from_user(¤t->thread.spe, &sr->mc_vregs,
> + sizeof(current->thread.spe), failed);
> current->thread.used_spe = true;
> } else if (current->thread.used_spe)
> - memset(current->thread.evr, 0, ELF_NEVRREG * sizeof(u32));
> + memset(¤t->thread.spe, 0, sizeof(current->thread.spe));
>
> /* Always get SPEFSCR back */
> unsafe_get_user(current->thread.spefscr, (u32 __user *)&sr->mc_vregs + ELF_NEVRREG, failed);
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] powerpc/signal32: Use struct_group() to zero spe regs
2021-11-19 8:46 ` LEROY Christophe
@ 2021-11-19 16:28 ` Kees Cook
2021-11-22 5:43 ` Michael Ellerman
1 sibling, 0 replies; 19+ messages in thread
From: Kees Cook @ 2021-11-19 16:28 UTC (permalink / raw)
To: LEROY Christophe
Cc: Michael Ellerman, kernel test robot, Benjamin Herrenschmidt,
Paul Mackerras, Sudeep Holla, Peter Zijlstra, Nicholas Piggin,
Aneesh Kumar K.V, Eric W. Biederman, linux-kernel, linuxppc-dev,
linux-hardening
On Fri, Nov 19, 2021 at 08:46:27AM +0000, LEROY Christophe wrote:
>
>
> Le 18/11/2021 à 21:36, Kees Cook a écrit :
> > In preparation for FORTIFY_SOURCE performing compile-time and run-time
> > field bounds checking for memset(), avoid intentionally writing across
> > neighboring fields.
> >
> > Add a struct_group() for the spe registers so that memset() can correctly reason
> > about the size:
> >
> > In function 'fortify_memset_chk',
> > inlined from 'restore_user_regs.part.0' at arch/powerpc/kernel/signal_32.c:539:3:
> > >> include/linux/fortify-string.h:195:4: error: call to '__write_overflow_field' declared with attribute warning: detected write beyond size of field (1st parameter); maybe use struct_group()? [-Werror=attribute-warning]
> > 195 | __write_overflow_field();
> > | ^~~~~~~~~~~~~~~~~~~~~~~~
> >
> > Reported-by: kernel test robot <lkp@intel.com>
> > Signed-off-by: Kees Cook <keescook@chromium.org>
>
> Reviewed-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>
> However, is it really worth adding that grouping ? Wouldn't it be
> cleaner to handle evr[] and acc separately ? Now that we are using
> unsafe variants of get/put user performance wouldn't be impacted.
I'm fine with whatever is desired here. I reworked an earlier version of
this patch based on mpe's feedback, so I can certain rework it again. :)
>
> I have some doubts about things like:
>
> unsafe_copy_to_user(&frame->mc_vregs, current->thread.evr,
> ELF_NEVRREG * sizeof(u32), failed);
>
> Because as far as I can see, ELF_NEVRREG is 34 but mc_vregs is a table
> of 33 u32 and is at the end of the structure:
>
> struct mcontext {
> elf_gregset_t mc_gregs;
> elf_fpregset_t mc_fregs;
> unsigned long mc_pad[2];
> elf_vrregset_t mc_vregs __attribute__((__aligned__(16)));
> };
>
> typedef elf_vrreg_t elf_vrregset_t[ELF_NVRREG];
>
> # define ELF_NEVRREG 34 /* includes acc (as 2) */
> # define ELF_NVRREG 33 /* includes vscr */
I don't know these internals very well -- do you want me to change this
specifically somehow? With the BUILD_BUG_ON()s added, there's no binary
change here -- I wanted to make sure nothing was different in the
output.
-Kees
>
>
>
> > ---
> > arch/powerpc/include/asm/processor.h | 6 ++++--
> > arch/powerpc/kernel/signal_32.c | 14 +++++++++-----
> > 2 files changed, 13 insertions(+), 7 deletions(-)
> >
> > diff --git a/arch/powerpc/include/asm/processor.h b/arch/powerpc/include/asm/processor.h
> > index e39bd0ff69f3..978a80308466 100644
> > --- a/arch/powerpc/include/asm/processor.h
> > +++ b/arch/powerpc/include/asm/processor.h
> > @@ -191,8 +191,10 @@ struct thread_struct {
> > int used_vsr; /* set if process has used VSX */
> > #endif /* CONFIG_VSX */
> > #ifdef CONFIG_SPE
> > - unsigned long evr[32]; /* upper 32-bits of SPE regs */
> > - u64 acc; /* Accumulator */
> > + struct_group(spe,
> > + unsigned long evr[32]; /* upper 32-bits of SPE regs */
> > + u64 acc; /* Accumulator */
> > + );
> > unsigned long spefscr; /* SPE & eFP status */
> > unsigned long spefscr_last; /* SPEFSCR value on last prctl
> > call or trap return */
> > diff --git a/arch/powerpc/kernel/signal_32.c b/arch/powerpc/kernel/signal_32.c
> > index 00a9c9cd6d42..5e1664b501e4 100644
> > --- a/arch/powerpc/kernel/signal_32.c
> > +++ b/arch/powerpc/kernel/signal_32.c
> > @@ -527,16 +527,20 @@ static long restore_user_regs(struct pt_regs *regs,
> > regs_set_return_msr(regs, regs->msr & ~(MSR_FP | MSR_FE0 | MSR_FE1));
> >
> > #ifdef CONFIG_SPE
> > - /* force the process to reload the spe registers from
> > - current->thread when it next does spe instructions */
> > + /*
> > + * Force the process to reload the spe registers from
> > + * current->thread when it next does spe instructions.
> > + * Since this is user ABI, we must enforce the sizing.
> > + */
> > + BUILD_BUG_ON(sizeof(current->thread.spe) != ELF_NEVRREG * sizeof(u32));
> > regs_set_return_msr(regs, regs->msr & ~MSR_SPE);
> > if (msr & MSR_SPE) {
> > /* restore spe registers from the stack */
> > - unsafe_copy_from_user(current->thread.evr, &sr->mc_vregs,
> > - ELF_NEVRREG * sizeof(u32), failed);
> > + unsafe_copy_from_user(¤t->thread.spe, &sr->mc_vregs,
> > + sizeof(current->thread.spe), failed);
> > current->thread.used_spe = true;
> > } else if (current->thread.used_spe)
> > - memset(current->thread.evr, 0, ELF_NEVRREG * sizeof(u32));
> > + memset(¤t->thread.spe, 0, sizeof(current->thread.spe));
> >
> > /* Always get SPEFSCR back */
> > unsafe_get_user(current->thread.spefscr, (u32 __user *)&sr->mc_vregs + ELF_NEVRREG, failed);
> >
--
Kees Cook
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] powerpc/signal32: Use struct_group() to zero spe regs
@ 2021-11-19 16:28 ` Kees Cook
0 siblings, 0 replies; 19+ messages in thread
From: Kees Cook @ 2021-11-19 16:28 UTC (permalink / raw)
To: LEROY Christophe
Cc: kernel test robot, Peter Zijlstra, linux-kernel, Nicholas Piggin,
linux-hardening, Paul Mackerras, Aneesh Kumar K.V, Sudeep Holla,
linuxppc-dev, Eric W. Biederman
On Fri, Nov 19, 2021 at 08:46:27AM +0000, LEROY Christophe wrote:
>
>
> Le 18/11/2021 à 21:36, Kees Cook a écrit :
> > In preparation for FORTIFY_SOURCE performing compile-time and run-time
> > field bounds checking for memset(), avoid intentionally writing across
> > neighboring fields.
> >
> > Add a struct_group() for the spe registers so that memset() can correctly reason
> > about the size:
> >
> > In function 'fortify_memset_chk',
> > inlined from 'restore_user_regs.part.0' at arch/powerpc/kernel/signal_32.c:539:3:
> > >> include/linux/fortify-string.h:195:4: error: call to '__write_overflow_field' declared with attribute warning: detected write beyond size of field (1st parameter); maybe use struct_group()? [-Werror=attribute-warning]
> > 195 | __write_overflow_field();
> > | ^~~~~~~~~~~~~~~~~~~~~~~~
> >
> > Reported-by: kernel test robot <lkp@intel.com>
> > Signed-off-by: Kees Cook <keescook@chromium.org>
>
> Reviewed-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>
> However, is it really worth adding that grouping ? Wouldn't it be
> cleaner to handle evr[] and acc separately ? Now that we are using
> unsafe variants of get/put user performance wouldn't be impacted.
I'm fine with whatever is desired here. I reworked an earlier version of
this patch based on mpe's feedback, so I can certain rework it again. :)
>
> I have some doubts about things like:
>
> unsafe_copy_to_user(&frame->mc_vregs, current->thread.evr,
> ELF_NEVRREG * sizeof(u32), failed);
>
> Because as far as I can see, ELF_NEVRREG is 34 but mc_vregs is a table
> of 33 u32 and is at the end of the structure:
>
> struct mcontext {
> elf_gregset_t mc_gregs;
> elf_fpregset_t mc_fregs;
> unsigned long mc_pad[2];
> elf_vrregset_t mc_vregs __attribute__((__aligned__(16)));
> };
>
> typedef elf_vrreg_t elf_vrregset_t[ELF_NVRREG];
>
> # define ELF_NEVRREG 34 /* includes acc (as 2) */
> # define ELF_NVRREG 33 /* includes vscr */
I don't know these internals very well -- do you want me to change this
specifically somehow? With the BUILD_BUG_ON()s added, there's no binary
change here -- I wanted to make sure nothing was different in the
output.
-Kees
>
>
>
> > ---
> > arch/powerpc/include/asm/processor.h | 6 ++++--
> > arch/powerpc/kernel/signal_32.c | 14 +++++++++-----
> > 2 files changed, 13 insertions(+), 7 deletions(-)
> >
> > diff --git a/arch/powerpc/include/asm/processor.h b/arch/powerpc/include/asm/processor.h
> > index e39bd0ff69f3..978a80308466 100644
> > --- a/arch/powerpc/include/asm/processor.h
> > +++ b/arch/powerpc/include/asm/processor.h
> > @@ -191,8 +191,10 @@ struct thread_struct {
> > int used_vsr; /* set if process has used VSX */
> > #endif /* CONFIG_VSX */
> > #ifdef CONFIG_SPE
> > - unsigned long evr[32]; /* upper 32-bits of SPE regs */
> > - u64 acc; /* Accumulator */
> > + struct_group(spe,
> > + unsigned long evr[32]; /* upper 32-bits of SPE regs */
> > + u64 acc; /* Accumulator */
> > + );
> > unsigned long spefscr; /* SPE & eFP status */
> > unsigned long spefscr_last; /* SPEFSCR value on last prctl
> > call or trap return */
> > diff --git a/arch/powerpc/kernel/signal_32.c b/arch/powerpc/kernel/signal_32.c
> > index 00a9c9cd6d42..5e1664b501e4 100644
> > --- a/arch/powerpc/kernel/signal_32.c
> > +++ b/arch/powerpc/kernel/signal_32.c
> > @@ -527,16 +527,20 @@ static long restore_user_regs(struct pt_regs *regs,
> > regs_set_return_msr(regs, regs->msr & ~(MSR_FP | MSR_FE0 | MSR_FE1));
> >
> > #ifdef CONFIG_SPE
> > - /* force the process to reload the spe registers from
> > - current->thread when it next does spe instructions */
> > + /*
> > + * Force the process to reload the spe registers from
> > + * current->thread when it next does spe instructions.
> > + * Since this is user ABI, we must enforce the sizing.
> > + */
> > + BUILD_BUG_ON(sizeof(current->thread.spe) != ELF_NEVRREG * sizeof(u32));
> > regs_set_return_msr(regs, regs->msr & ~MSR_SPE);
> > if (msr & MSR_SPE) {
> > /* restore spe registers from the stack */
> > - unsafe_copy_from_user(current->thread.evr, &sr->mc_vregs,
> > - ELF_NEVRREG * sizeof(u32), failed);
> > + unsafe_copy_from_user(¤t->thread.spe, &sr->mc_vregs,
> > + sizeof(current->thread.spe), failed);
> > current->thread.used_spe = true;
> > } else if (current->thread.used_spe)
> > - memset(current->thread.evr, 0, ELF_NEVRREG * sizeof(u32));
> > + memset(¤t->thread.spe, 0, sizeof(current->thread.spe));
> >
> > /* Always get SPEFSCR back */
> > unsafe_get_user(current->thread.spefscr, (u32 __user *)&sr->mc_vregs + ELF_NEVRREG, failed);
> >
--
Kees Cook
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] powerpc/signal32: Use struct_group() to zero spe regs
2021-11-19 16:28 ` Kees Cook
@ 2021-11-19 16:35 ` Christophe Leroy
-1 siblings, 0 replies; 19+ messages in thread
From: Christophe Leroy @ 2021-11-19 16:35 UTC (permalink / raw)
To: Kees Cook
Cc: Michael Ellerman, kernel test robot, Benjamin Herrenschmidt,
Paul Mackerras, Sudeep Holla, Peter Zijlstra, Nicholas Piggin,
Aneesh Kumar K.V, Eric W. Biederman, linux-kernel, linuxppc-dev,
linux-hardening
Le 19/11/2021 à 17:28, Kees Cook a écrit :
> On Fri, Nov 19, 2021 at 08:46:27AM +0000, LEROY Christophe wrote:
>>
>>
>> Le 18/11/2021 à 21:36, Kees Cook a écrit :
>>> In preparation for FORTIFY_SOURCE performing compile-time and run-time
>>> field bounds checking for memset(), avoid intentionally writing across
>>> neighboring fields.
>>>
>>> Add a struct_group() for the spe registers so that memset() can correctly reason
>>> about the size:
>>>
>>> In function 'fortify_memset_chk',
>>> inlined from 'restore_user_regs.part.0' at arch/powerpc/kernel/signal_32.c:539:3:
>>> >> include/linux/fortify-string.h:195:4: error: call to '__write_overflow_field' declared with attribute warning: detected write beyond size of field (1st parameter); maybe use struct_group()? [-Werror=attribute-warning]
>>> 195 | __write_overflow_field();
>>> | ^~~~~~~~~~~~~~~~~~~~~~~~
>>>
>>> Reported-by: kernel test robot <lkp@intel.com>
>>> Signed-off-by: Kees Cook <keescook@chromium.org>
>>
>> Reviewed-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>>
>> However, is it really worth adding that grouping ? Wouldn't it be
>> cleaner to handle evr[] and acc separately ? Now that we are using
>> unsafe variants of get/put user performance wouldn't be impacted.
>
> I'm fine with whatever is desired here. I reworked an earlier version of
> this patch based on mpe's feedback, so I can certain rework it again. :)
Well, with oddities like the below, it may not be straight forward. If
the objective is to enable FORTIFY_SOURCE, maybe that's good enough.
Let see if Michael has any opinion.
>
>>
>> I have some doubts about things like:
>>
>> unsafe_copy_to_user(&frame->mc_vregs, current->thread.evr,
>> ELF_NEVRREG * sizeof(u32), failed);
>>
>> Because as far as I can see, ELF_NEVRREG is 34 but mc_vregs is a table
>> of 33 u32 and is at the end of the structure:
>>
>> struct mcontext {
>> elf_gregset_t mc_gregs;
>> elf_fpregset_t mc_fregs;
>> unsigned long mc_pad[2];
>> elf_vrregset_t mc_vregs __attribute__((__aligned__(16)));
>> };
>>
>> typedef elf_vrreg_t elf_vrregset_t[ELF_NVRREG];
>>
>> # define ELF_NEVRREG 34 /* includes acc (as 2) */
>> # define ELF_NVRREG 33 /* includes vscr */
>
> I don't know these internals very well -- do you want me to change this
> specifically somehow? With the BUILD_BUG_ON()s added, there's no binary
> change here -- I wanted to make sure nothing was different in the
> output.
>
Neither do I. I was just scared by what I saw while reviewing your
patch. A cleanup is probably required but it can be another patch.
Christophe
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] powerpc/signal32: Use struct_group() to zero spe regs
@ 2021-11-19 16:35 ` Christophe Leroy
0 siblings, 0 replies; 19+ messages in thread
From: Christophe Leroy @ 2021-11-19 16:35 UTC (permalink / raw)
To: Kees Cook
Cc: kernel test robot, Peter Zijlstra, linux-kernel, Nicholas Piggin,
linux-hardening, Paul Mackerras, Aneesh Kumar K.V, Sudeep Holla,
linuxppc-dev, Eric W. Biederman
Le 19/11/2021 à 17:28, Kees Cook a écrit :
> On Fri, Nov 19, 2021 at 08:46:27AM +0000, LEROY Christophe wrote:
>>
>>
>> Le 18/11/2021 à 21:36, Kees Cook a écrit :
>>> In preparation for FORTIFY_SOURCE performing compile-time and run-time
>>> field bounds checking for memset(), avoid intentionally writing across
>>> neighboring fields.
>>>
>>> Add a struct_group() for the spe registers so that memset() can correctly reason
>>> about the size:
>>>
>>> In function 'fortify_memset_chk',
>>> inlined from 'restore_user_regs.part.0' at arch/powerpc/kernel/signal_32.c:539:3:
>>> >> include/linux/fortify-string.h:195:4: error: call to '__write_overflow_field' declared with attribute warning: detected write beyond size of field (1st parameter); maybe use struct_group()? [-Werror=attribute-warning]
>>> 195 | __write_overflow_field();
>>> | ^~~~~~~~~~~~~~~~~~~~~~~~
>>>
>>> Reported-by: kernel test robot <lkp@intel.com>
>>> Signed-off-by: Kees Cook <keescook@chromium.org>
>>
>> Reviewed-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>>
>> However, is it really worth adding that grouping ? Wouldn't it be
>> cleaner to handle evr[] and acc separately ? Now that we are using
>> unsafe variants of get/put user performance wouldn't be impacted.
>
> I'm fine with whatever is desired here. I reworked an earlier version of
> this patch based on mpe's feedback, so I can certain rework it again. :)
Well, with oddities like the below, it may not be straight forward. If
the objective is to enable FORTIFY_SOURCE, maybe that's good enough.
Let see if Michael has any opinion.
>
>>
>> I have some doubts about things like:
>>
>> unsafe_copy_to_user(&frame->mc_vregs, current->thread.evr,
>> ELF_NEVRREG * sizeof(u32), failed);
>>
>> Because as far as I can see, ELF_NEVRREG is 34 but mc_vregs is a table
>> of 33 u32 and is at the end of the structure:
>>
>> struct mcontext {
>> elf_gregset_t mc_gregs;
>> elf_fpregset_t mc_fregs;
>> unsigned long mc_pad[2];
>> elf_vrregset_t mc_vregs __attribute__((__aligned__(16)));
>> };
>>
>> typedef elf_vrreg_t elf_vrregset_t[ELF_NVRREG];
>>
>> # define ELF_NEVRREG 34 /* includes acc (as 2) */
>> # define ELF_NVRREG 33 /* includes vscr */
>
> I don't know these internals very well -- do you want me to change this
> specifically somehow? With the BUILD_BUG_ON()s added, there's no binary
> change here -- I wanted to make sure nothing was different in the
> output.
>
Neither do I. I was just scared by what I saw while reviewing your
patch. A cleanup is probably required but it can be another patch.
Christophe
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] powerpc/signal32: Use struct_group() to zero spe regs
2021-11-19 16:35 ` Christophe Leroy
@ 2021-11-19 16:42 ` Kees Cook
-1 siblings, 0 replies; 19+ messages in thread
From: Kees Cook @ 2021-11-19 16:42 UTC (permalink / raw)
To: Christophe Leroy
Cc: Michael Ellerman, kernel test robot, Benjamin Herrenschmidt,
Paul Mackerras, Sudeep Holla, Peter Zijlstra, Nicholas Piggin,
Aneesh Kumar K.V, Eric W. Biederman, linux-kernel, linuxppc-dev,
linux-hardening
On Fri, Nov 19, 2021 at 05:35:00PM +0100, Christophe Leroy wrote:
> Neither do I. I was just scared by what I saw while reviewing your patch. A
> cleanup is probably required but it can be another patch.
Heh, understood! For my end, my objective with the fortify work is to
either split cross-member memcpy() calls (which is usually undesirable) or
add a struct group so it can be seen as a "single member" memcpy again
(and usually results in 0 differences in binary output). :)
--
Kees Cook
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] powerpc/signal32: Use struct_group() to zero spe regs
@ 2021-11-19 16:42 ` Kees Cook
0 siblings, 0 replies; 19+ messages in thread
From: Kees Cook @ 2021-11-19 16:42 UTC (permalink / raw)
To: Christophe Leroy
Cc: kernel test robot, Peter Zijlstra, linux-kernel, Nicholas Piggin,
linux-hardening, Paul Mackerras, Aneesh Kumar K.V, Sudeep Holla,
linuxppc-dev, Eric W. Biederman
On Fri, Nov 19, 2021 at 05:35:00PM +0100, Christophe Leroy wrote:
> Neither do I. I was just scared by what I saw while reviewing your patch. A
> cleanup is probably required but it can be another patch.
Heh, understood! For my end, my objective with the fortify work is to
either split cross-member memcpy() calls (which is usually undesirable) or
add a struct group so it can be seen as a "single member" memcpy again
(and usually results in 0 differences in binary output). :)
--
Kees Cook
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] powerpc/signal32: Use struct_group() to zero spe regs
2021-11-19 8:46 ` LEROY Christophe
@ 2021-11-22 5:43 ` Michael Ellerman
2021-11-22 5:43 ` Michael Ellerman
1 sibling, 0 replies; 19+ messages in thread
From: Michael Ellerman @ 2021-11-22 5:43 UTC (permalink / raw)
To: LEROY Christophe, Kees Cook
Cc: kernel test robot, Benjamin Herrenschmidt, Paul Mackerras,
Sudeep Holla, Peter Zijlstra, Nicholas Piggin, Aneesh Kumar K.V,
Eric W. Biederman, linux-kernel, linuxppc-dev, linux-hardening
LEROY Christophe <christophe.leroy@csgroup.eu> writes:
> Le 18/11/2021 à 21:36, Kees Cook a écrit :
>> In preparation for FORTIFY_SOURCE performing compile-time and run-time
>> field bounds checking for memset(), avoid intentionally writing across
>> neighboring fields.
>>
>> Add a struct_group() for the spe registers so that memset() can correctly reason
>> about the size:
>>
>> In function 'fortify_memset_chk',
>> inlined from 'restore_user_regs.part.0' at arch/powerpc/kernel/signal_32.c:539:3:
>> >> include/linux/fortify-string.h:195:4: error: call to '__write_overflow_field' declared with attribute warning: detected write beyond size of field (1st parameter); maybe use struct_group()? [-Werror=attribute-warning]
>> 195 | __write_overflow_field();
>> | ^~~~~~~~~~~~~~~~~~~~~~~~
>>
>> Reported-by: kernel test robot <lkp@intel.com>
>> Signed-off-by: Kees Cook <keescook@chromium.org>
>
> Reviewed-by: Christophe Leroy <christophe.leroy@csgroup.eu>
Acked-by: Michael Ellerman <mpe@ellerman.id.au>
> However, is it really worth adding that grouping ? Wouldn't it be
> cleaner to handle evr[] and acc separately ? Now that we are using
> unsafe variants of get/put user performance wouldn't be impacted.
Yeah I agree we should be able to do less of these multi-field copies
now that we have unsafe get/put user.
But I think that's an issue for another patch, Kees' patch is an
improvement, even if the code could be improved further in future.
Though TBH I'm not sure what the future of SPE support is. Both GCC and
glibc have dropped support for it, more than 2 years ago, so it's not
clear to me if we should continue to support it in the kernel much
longer.
cheers
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] powerpc/signal32: Use struct_group() to zero spe regs
@ 2021-11-22 5:43 ` Michael Ellerman
0 siblings, 0 replies; 19+ messages in thread
From: Michael Ellerman @ 2021-11-22 5:43 UTC (permalink / raw)
To: LEROY Christophe, Kees Cook
Cc: kernel test robot, Peter Zijlstra, linux-kernel, Nicholas Piggin,
linux-hardening, Paul Mackerras, Aneesh Kumar K.V, Sudeep Holla,
linuxppc-dev, Eric W. Biederman
LEROY Christophe <christophe.leroy@csgroup.eu> writes:
> Le 18/11/2021 à 21:36, Kees Cook a écrit :
>> In preparation for FORTIFY_SOURCE performing compile-time and run-time
>> field bounds checking for memset(), avoid intentionally writing across
>> neighboring fields.
>>
>> Add a struct_group() for the spe registers so that memset() can correctly reason
>> about the size:
>>
>> In function 'fortify_memset_chk',
>> inlined from 'restore_user_regs.part.0' at arch/powerpc/kernel/signal_32.c:539:3:
>> >> include/linux/fortify-string.h:195:4: error: call to '__write_overflow_field' declared with attribute warning: detected write beyond size of field (1st parameter); maybe use struct_group()? [-Werror=attribute-warning]
>> 195 | __write_overflow_field();
>> | ^~~~~~~~~~~~~~~~~~~~~~~~
>>
>> Reported-by: kernel test robot <lkp@intel.com>
>> Signed-off-by: Kees Cook <keescook@chromium.org>
>
> Reviewed-by: Christophe Leroy <christophe.leroy@csgroup.eu>
Acked-by: Michael Ellerman <mpe@ellerman.id.au>
> However, is it really worth adding that grouping ? Wouldn't it be
> cleaner to handle evr[] and acc separately ? Now that we are using
> unsafe variants of get/put user performance wouldn't be impacted.
Yeah I agree we should be able to do less of these multi-field copies
now that we have unsafe get/put user.
But I think that's an issue for another patch, Kees' patch is an
improvement, even if the code could be improved further in future.
Though TBH I'm not sure what the future of SPE support is. Both GCC and
glibc have dropped support for it, more than 2 years ago, so it's not
clear to me if we should continue to support it in the kernel much
longer.
cheers
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] powerpc/signal32: Use struct_group() to zero spe regs
2021-11-22 5:43 ` Michael Ellerman
@ 2021-11-22 20:47 ` Kees Cook
-1 siblings, 0 replies; 19+ messages in thread
From: Kees Cook @ 2021-11-22 20:47 UTC (permalink / raw)
To: Michael Ellerman
Cc: LEROY Christophe, kernel test robot, Benjamin Herrenschmidt,
Paul Mackerras, Sudeep Holla, Peter Zijlstra, Nicholas Piggin,
Aneesh Kumar K.V, Eric W. Biederman, linux-kernel, linuxppc-dev,
linux-hardening
On Mon, Nov 22, 2021 at 04:43:36PM +1100, Michael Ellerman wrote:
> LEROY Christophe <christophe.leroy@csgroup.eu> writes:
> > Le 18/11/2021 à 21:36, Kees Cook a écrit :
> >> In preparation for FORTIFY_SOURCE performing compile-time and run-time
> >> field bounds checking for memset(), avoid intentionally writing across
> >> neighboring fields.
> >>
> >> Add a struct_group() for the spe registers so that memset() can correctly reason
> >> about the size:
> >>
> >> In function 'fortify_memset_chk',
> >> inlined from 'restore_user_regs.part.0' at arch/powerpc/kernel/signal_32.c:539:3:
> >> >> include/linux/fortify-string.h:195:4: error: call to '__write_overflow_field' declared with attribute warning: detected write beyond size of field (1st parameter); maybe use struct_group()? [-Werror=attribute-warning]
> >> 195 | __write_overflow_field();
> >> | ^~~~~~~~~~~~~~~~~~~~~~~~
> >>
> >> Reported-by: kernel test robot <lkp@intel.com>
> >> Signed-off-by: Kees Cook <keescook@chromium.org>
> >
> > Reviewed-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>
> Acked-by: Michael Ellerman <mpe@ellerman.id.au>
Thanks! Should I take this via my tree, or do you want to take it via
ppc?
-Kees
--
Kees Cook
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] powerpc/signal32: Use struct_group() to zero spe regs
@ 2021-11-22 20:47 ` Kees Cook
0 siblings, 0 replies; 19+ messages in thread
From: Kees Cook @ 2021-11-22 20:47 UTC (permalink / raw)
To: Michael Ellerman
Cc: Aneesh Kumar K.V, kernel test robot, Peter Zijlstra,
linux-kernel, linux-hardening, Paul Mackerras, Nicholas Piggin,
Sudeep Holla, linuxppc-dev, Eric W. Biederman
On Mon, Nov 22, 2021 at 04:43:36PM +1100, Michael Ellerman wrote:
> LEROY Christophe <christophe.leroy@csgroup.eu> writes:
> > Le 18/11/2021 à 21:36, Kees Cook a écrit :
> >> In preparation for FORTIFY_SOURCE performing compile-time and run-time
> >> field bounds checking for memset(), avoid intentionally writing across
> >> neighboring fields.
> >>
> >> Add a struct_group() for the spe registers so that memset() can correctly reason
> >> about the size:
> >>
> >> In function 'fortify_memset_chk',
> >> inlined from 'restore_user_regs.part.0' at arch/powerpc/kernel/signal_32.c:539:3:
> >> >> include/linux/fortify-string.h:195:4: error: call to '__write_overflow_field' declared with attribute warning: detected write beyond size of field (1st parameter); maybe use struct_group()? [-Werror=attribute-warning]
> >> 195 | __write_overflow_field();
> >> | ^~~~~~~~~~~~~~~~~~~~~~~~
> >>
> >> Reported-by: kernel test robot <lkp@intel.com>
> >> Signed-off-by: Kees Cook <keescook@chromium.org>
> >
> > Reviewed-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>
> Acked-by: Michael Ellerman <mpe@ellerman.id.au>
Thanks! Should I take this via my tree, or do you want to take it via
ppc?
-Kees
--
Kees Cook
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] powerpc/signal32: Use struct_group() to zero spe regs
2021-11-22 20:47 ` Kees Cook
@ 2021-11-24 0:08 ` Michael Ellerman
-1 siblings, 0 replies; 19+ messages in thread
From: Michael Ellerman @ 2021-11-24 0:08 UTC (permalink / raw)
To: Kees Cook
Cc: LEROY Christophe, kernel test robot, Benjamin Herrenschmidt,
Paul Mackerras, Sudeep Holla, Peter Zijlstra, Nicholas Piggin,
Aneesh Kumar K.V, Eric W. Biederman, linux-kernel, linuxppc-dev,
linux-hardening
Kees Cook <keescook@chromium.org> writes:
> On Mon, Nov 22, 2021 at 04:43:36PM +1100, Michael Ellerman wrote:
>> LEROY Christophe <christophe.leroy@csgroup.eu> writes:
>> > Le 18/11/2021 à 21:36, Kees Cook a écrit :
>> >> In preparation for FORTIFY_SOURCE performing compile-time and run-time
>> >> field bounds checking for memset(), avoid intentionally writing across
>> >> neighboring fields.
>> >>
>> >> Add a struct_group() for the spe registers so that memset() can correctly reason
>> >> about the size:
>> >>
>> >> In function 'fortify_memset_chk',
>> >> inlined from 'restore_user_regs.part.0' at arch/powerpc/kernel/signal_32.c:539:3:
>> >> >> include/linux/fortify-string.h:195:4: error: call to '__write_overflow_field' declared with attribute warning: detected write beyond size of field (1st parameter); maybe use struct_group()? [-Werror=attribute-warning]
>> >> 195 | __write_overflow_field();
>> >> | ^~~~~~~~~~~~~~~~~~~~~~~~
>> >>
>> >> Reported-by: kernel test robot <lkp@intel.com>
>> >> Signed-off-by: Kees Cook <keescook@chromium.org>
>> >
>> > Reviewed-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>>
>> Acked-by: Michael Ellerman <mpe@ellerman.id.au>
>
> Thanks! Should I take this via my tree, or do you want to take it via
> ppc?
I don't mind. If it's easier for you to take it as part of an existing
series then do that, otherwise I can pick it up.
cheers
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] powerpc/signal32: Use struct_group() to zero spe regs
@ 2021-11-24 0:08 ` Michael Ellerman
0 siblings, 0 replies; 19+ messages in thread
From: Michael Ellerman @ 2021-11-24 0:08 UTC (permalink / raw)
To: Kees Cook
Cc: Aneesh Kumar K.V, kernel test robot, Peter Zijlstra,
linux-kernel, linux-hardening, Paul Mackerras, Nicholas Piggin,
Sudeep Holla, linuxppc-dev, Eric W. Biederman
Kees Cook <keescook@chromium.org> writes:
> On Mon, Nov 22, 2021 at 04:43:36PM +1100, Michael Ellerman wrote:
>> LEROY Christophe <christophe.leroy@csgroup.eu> writes:
>> > Le 18/11/2021 à 21:36, Kees Cook a écrit :
>> >> In preparation for FORTIFY_SOURCE performing compile-time and run-time
>> >> field bounds checking for memset(), avoid intentionally writing across
>> >> neighboring fields.
>> >>
>> >> Add a struct_group() for the spe registers so that memset() can correctly reason
>> >> about the size:
>> >>
>> >> In function 'fortify_memset_chk',
>> >> inlined from 'restore_user_regs.part.0' at arch/powerpc/kernel/signal_32.c:539:3:
>> >> >> include/linux/fortify-string.h:195:4: error: call to '__write_overflow_field' declared with attribute warning: detected write beyond size of field (1st parameter); maybe use struct_group()? [-Werror=attribute-warning]
>> >> 195 | __write_overflow_field();
>> >> | ^~~~~~~~~~~~~~~~~~~~~~~~
>> >>
>> >> Reported-by: kernel test robot <lkp@intel.com>
>> >> Signed-off-by: Kees Cook <keescook@chromium.org>
>> >
>> > Reviewed-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>>
>> Acked-by: Michael Ellerman <mpe@ellerman.id.au>
>
> Thanks! Should I take this via my tree, or do you want to take it via
> ppc?
I don't mind. If it's easier for you to take it as part of an existing
series then do that, otherwise I can pick it up.
cheers
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] powerpc/signal32: Use struct_group() to zero spe regs
2021-11-24 0:08 ` Michael Ellerman
@ 2021-12-01 18:55 ` Kees Cook
-1 siblings, 0 replies; 19+ messages in thread
From: Kees Cook @ 2021-12-01 18:55 UTC (permalink / raw)
To: Michael Ellerman
Cc: LEROY Christophe, kernel test robot, Benjamin Herrenschmidt,
Paul Mackerras, Sudeep Holla, Peter Zijlstra, Nicholas Piggin,
Aneesh Kumar K.V, Eric W. Biederman, linux-kernel, linuxppc-dev,
linux-hardening
On Wed, Nov 24, 2021 at 11:08:25AM +1100, Michael Ellerman wrote:
> Kees Cook <keescook@chromium.org> writes:
> > On Mon, Nov 22, 2021 at 04:43:36PM +1100, Michael Ellerman wrote:
> >> LEROY Christophe <christophe.leroy@csgroup.eu> writes:
> >> > Le 18/11/2021 à 21:36, Kees Cook a écrit :
> >> >> In preparation for FORTIFY_SOURCE performing compile-time and run-time
> >> >> field bounds checking for memset(), avoid intentionally writing across
> >> >> neighboring fields.
> >> >>
> >> >> Add a struct_group() for the spe registers so that memset() can correctly reason
> >> >> about the size:
> >> >>
> >> >> In function 'fortify_memset_chk',
> >> >> inlined from 'restore_user_regs.part.0' at arch/powerpc/kernel/signal_32.c:539:3:
> >> >> >> include/linux/fortify-string.h:195:4: error: call to '__write_overflow_field' declared with attribute warning: detected write beyond size of field (1st parameter); maybe use struct_group()? [-Werror=attribute-warning]
> >> >> 195 | __write_overflow_field();
> >> >> | ^~~~~~~~~~~~~~~~~~~~~~~~
> >> >>
> >> >> Reported-by: kernel test robot <lkp@intel.com>
> >> >> Signed-off-by: Kees Cook <keescook@chromium.org>
> >> >
> >> > Reviewed-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> >>
> >> Acked-by: Michael Ellerman <mpe@ellerman.id.au>
> >
> > Thanks! Should I take this via my tree, or do you want to take it via
> > ppc?
>
> I don't mind. If it's easier for you to take it as part of an existing
> series then do that, otherwise I can pick it up.
Most of the other patches are going via other maintainers, so I'd love
it if ppc could snag this one too. :)
Thanks!
-Kees
--
Kees Cook
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] powerpc/signal32: Use struct_group() to zero spe regs
@ 2021-12-01 18:55 ` Kees Cook
0 siblings, 0 replies; 19+ messages in thread
From: Kees Cook @ 2021-12-01 18:55 UTC (permalink / raw)
To: Michael Ellerman
Cc: Aneesh Kumar K.V, kernel test robot, Peter Zijlstra,
linux-kernel, linux-hardening, Paul Mackerras, Nicholas Piggin,
Sudeep Holla, linuxppc-dev, Eric W. Biederman
On Wed, Nov 24, 2021 at 11:08:25AM +1100, Michael Ellerman wrote:
> Kees Cook <keescook@chromium.org> writes:
> > On Mon, Nov 22, 2021 at 04:43:36PM +1100, Michael Ellerman wrote:
> >> LEROY Christophe <christophe.leroy@csgroup.eu> writes:
> >> > Le 18/11/2021 à 21:36, Kees Cook a écrit :
> >> >> In preparation for FORTIFY_SOURCE performing compile-time and run-time
> >> >> field bounds checking for memset(), avoid intentionally writing across
> >> >> neighboring fields.
> >> >>
> >> >> Add a struct_group() for the spe registers so that memset() can correctly reason
> >> >> about the size:
> >> >>
> >> >> In function 'fortify_memset_chk',
> >> >> inlined from 'restore_user_regs.part.0' at arch/powerpc/kernel/signal_32.c:539:3:
> >> >> >> include/linux/fortify-string.h:195:4: error: call to '__write_overflow_field' declared with attribute warning: detected write beyond size of field (1st parameter); maybe use struct_group()? [-Werror=attribute-warning]
> >> >> 195 | __write_overflow_field();
> >> >> | ^~~~~~~~~~~~~~~~~~~~~~~~
> >> >>
> >> >> Reported-by: kernel test robot <lkp@intel.com>
> >> >> Signed-off-by: Kees Cook <keescook@chromium.org>
> >> >
> >> > Reviewed-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> >>
> >> Acked-by: Michael Ellerman <mpe@ellerman.id.au>
> >
> > Thanks! Should I take this via my tree, or do you want to take it via
> > ppc?
>
> I don't mind. If it's easier for you to take it as part of an existing
> series then do that, otherwise I can pick it up.
Most of the other patches are going via other maintainers, so I'd love
it if ppc could snag this one too. :)
Thanks!
-Kees
--
Kees Cook
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] powerpc/signal32: Use struct_group() to zero spe regs
2021-11-18 20:36 ` Kees Cook
@ 2021-12-07 13:27 ` Michael Ellerman
-1 siblings, 0 replies; 19+ messages in thread
From: Michael Ellerman @ 2021-12-07 13:27 UTC (permalink / raw)
To: Michael Ellerman, Kees Cook
Cc: Nicholas Piggin, Aneesh Kumar K.V, linux-hardening,
Eric W. Biederman, Sudeep Holla, Paul Mackerras,
kernel test robot, Peter Zijlstra, Benjamin Herrenschmidt,
linuxppc-dev, Christophe Leroy, linux-kernel
On Thu, 18 Nov 2021 12:36:04 -0800, Kees Cook wrote:
> In preparation for FORTIFY_SOURCE performing compile-time and run-time
> field bounds checking for memset(), avoid intentionally writing across
> neighboring fields.
>
> Add a struct_group() for the spe registers so that memset() can correctly reason
> about the size:
>
> [...]
Applied to powerpc/next.
[1/1] powerpc/signal32: Use struct_group() to zero spe regs
https://git.kernel.org/powerpc/c/62ea67e31981bca95ec16c37e2a1fba68f3dd8c5
cheers
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] powerpc/signal32: Use struct_group() to zero spe regs
@ 2021-12-07 13:27 ` Michael Ellerman
0 siblings, 0 replies; 19+ messages in thread
From: Michael Ellerman @ 2021-12-07 13:27 UTC (permalink / raw)
To: Michael Ellerman, Kees Cook
Cc: kernel test robot, Peter Zijlstra, Aneesh Kumar K.V,
linux-kernel, Nicholas Piggin, Paul Mackerras, linux-hardening,
Sudeep Holla, linuxppc-dev, Eric W. Biederman
On Thu, 18 Nov 2021 12:36:04 -0800, Kees Cook wrote:
> In preparation for FORTIFY_SOURCE performing compile-time and run-time
> field bounds checking for memset(), avoid intentionally writing across
> neighboring fields.
>
> Add a struct_group() for the spe registers so that memset() can correctly reason
> about the size:
>
> [...]
Applied to powerpc/next.
[1/1] powerpc/signal32: Use struct_group() to zero spe regs
https://git.kernel.org/powerpc/c/62ea67e31981bca95ec16c37e2a1fba68f3dd8c5
cheers
^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2021-12-07 13:32 UTC | newest]
Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-18 20:36 [PATCH] powerpc/signal32: Use struct_group() to zero spe regs Kees Cook
2021-11-18 20:36 ` Kees Cook
2021-11-19 8:46 ` LEROY Christophe
2021-11-19 16:28 ` Kees Cook
2021-11-19 16:28 ` Kees Cook
2021-11-19 16:35 ` Christophe Leroy
2021-11-19 16:35 ` Christophe Leroy
2021-11-19 16:42 ` Kees Cook
2021-11-19 16:42 ` Kees Cook
2021-11-22 5:43 ` Michael Ellerman
2021-11-22 5:43 ` Michael Ellerman
2021-11-22 20:47 ` Kees Cook
2021-11-22 20:47 ` Kees Cook
2021-11-24 0:08 ` Michael Ellerman
2021-11-24 0:08 ` Michael Ellerman
2021-12-01 18:55 ` Kees Cook
2021-12-01 18:55 ` Kees Cook
2021-12-07 13:27 ` Michael Ellerman
2021-12-07 13:27 ` Michael Ellerman
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.