* Re: [PATCH 03/11] fs: don't allow splice read/write without explicit ops
From: Christoph Hellwig @ 2020-08-18 20:07 UTC (permalink / raw)
To: Kees Cook
Cc: linux-arch, x86, linux-kernel, Al Viro, linux-fsdevel,
linuxppc-dev, Christoph Hellwig
In-Reply-To: <202008181256.CABD56782@keescook>
On Tue, Aug 18, 2020 at 12:58:07PM -0700, Kees Cook wrote:
> On Tue, Aug 18, 2020 at 09:54:46PM +0200, Christoph Hellwig wrote:
> > On Tue, Aug 18, 2020 at 12:39:34PM -0700, Kees Cook wrote:
> > > On Mon, Aug 17, 2020 at 09:32:04AM +0200, Christoph Hellwig wrote:
> > > > default_file_splice_write is the last piece of generic code that uses
> > > > set_fs to make the uaccess routines operate on kernel pointers. It
> > > > implements a "fallback loop" for splicing from files that do not actually
> > > > provide a proper splice_read method. The usual file systems and other
> > > > high bandwith instances all provide a ->splice_read, so this just removes
> > > > support for various device drivers and procfs/debugfs files. If splice
> > > > support for any of those turns out to be important it can be added back
> > > > by switching them to the iter ops and using generic_file_splice_read.
> > > >
> > > > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > >
> > > This seems a bit disruptive? I feel like this is going to make fuzzers
> > > really noisy (e.g. trinity likes to splice random stuff out of /sys and
> > > /proc).
> >
> > Noisy in the sence of triggering the pr_debug or because they can't
> > handle -EINVAL?
>
> Well, maybe both? I doubt much _expects_ to be using splice, so I'm fine
> with that, but it seems weird not to have a fall-back, especially if
> something would like to splice a file out of there. But, I'm not opposed
> to the change, it just seems like it might cause pain down the road.
The problem is that without pretending a buffer is in user space when
it actually isn't, we can't have a generic fallback. So we'll have to
have specific support - I wrote generic support for seq_file, and
willy did for /proc/sys, but at least the first caused a few problems
and a fair amount of churn, so I'd rather see first if we can get
away without it.
>
> --
> Kees Cook
---end quoted text---
^ permalink raw reply
* Re: [PATCH 08/11] x86: make TASK_SIZE_MAX usable from assembly code
From: Christoph Hellwig @ 2020-08-18 20:00 UTC (permalink / raw)
To: Kees Cook
Cc: linux-arch, x86, linux-kernel, Al Viro, linux-fsdevel,
linuxppc-dev, Christoph Hellwig
In-Reply-To: <202008181258.CEC4B8B3@keescook>
On Tue, Aug 18, 2020 at 12:59:05PM -0700, Kees Cook wrote:
> > I didn't see a problem bisecting, do you have something particular in
> > mind?
>
> Oh, I misunderstood this patch to be a fix for compilation. Is this just
> a correctness fix?
It prepares for using the definition from assembly, which is done in
the next patch.
^ permalink raw reply
* Re: [PATCH 08/11] x86: make TASK_SIZE_MAX usable from assembly code
From: Kees Cook @ 2020-08-18 19:59 UTC (permalink / raw)
To: Christoph Hellwig
Cc: linux-arch, x86, linux-kernel, Al Viro, linux-fsdevel,
linuxppc-dev
In-Reply-To: <20200818195539.GB32691@lst.de>
On Tue, Aug 18, 2020 at 09:55:39PM +0200, Christoph Hellwig wrote:
> On Tue, Aug 18, 2020 at 12:44:49PM -0700, Kees Cook wrote:
> > On Mon, Aug 17, 2020 at 09:32:09AM +0200, Christoph Hellwig wrote:
> > > For 64-bit the only hing missing was a strategic _AC, and for 32-bit we
> >
> > typo: thing
> >
> > > need to use __PAGE_OFFSET instead of PAGE_OFFSET in the TASK_SIZE
> > > definition to escape the explicit unsigned long cast. This just works
> > > because __PAGE_OFFSET is defined using _AC itself and thus never needs
> > > the cast anyway.
> >
> > Shouldn't this be folded into the prior patch so there's no bisection
> > problem?
>
> I didn't see a problem bisecting, do you have something particular in
> mind?
Oh, I misunderstood this patch to be a fix for compilation. Is this just
a correctness fix?
--
Kees Cook
^ permalink raw reply
* Re: [PATCH 03/11] fs: don't allow splice read/write without explicit ops
From: Kees Cook @ 2020-08-18 19:58 UTC (permalink / raw)
To: Christoph Hellwig
Cc: linux-arch, x86, linux-kernel, Al Viro, linux-fsdevel,
linuxppc-dev
In-Reply-To: <20200818195446.GA32691@lst.de>
On Tue, Aug 18, 2020 at 09:54:46PM +0200, Christoph Hellwig wrote:
> On Tue, Aug 18, 2020 at 12:39:34PM -0700, Kees Cook wrote:
> > On Mon, Aug 17, 2020 at 09:32:04AM +0200, Christoph Hellwig wrote:
> > > default_file_splice_write is the last piece of generic code that uses
> > > set_fs to make the uaccess routines operate on kernel pointers. It
> > > implements a "fallback loop" for splicing from files that do not actually
> > > provide a proper splice_read method. The usual file systems and other
> > > high bandwith instances all provide a ->splice_read, so this just removes
> > > support for various device drivers and procfs/debugfs files. If splice
> > > support for any of those turns out to be important it can be added back
> > > by switching them to the iter ops and using generic_file_splice_read.
> > >
> > > Signed-off-by: Christoph Hellwig <hch@lst.de>
> >
> > This seems a bit disruptive? I feel like this is going to make fuzzers
> > really noisy (e.g. trinity likes to splice random stuff out of /sys and
> > /proc).
>
> Noisy in the sence of triggering the pr_debug or because they can't
> handle -EINVAL?
Well, maybe both? I doubt much _expects_ to be using splice, so I'm fine
with that, but it seems weird not to have a fall-back, especially if
something would like to splice a file out of there. But, I'm not opposed
to the change, it just seems like it might cause pain down the road.
--
Kees Cook
^ permalink raw reply
* Re: [PATCH 08/11] x86: make TASK_SIZE_MAX usable from assembly code
From: Christoph Hellwig @ 2020-08-18 19:55 UTC (permalink / raw)
To: Kees Cook
Cc: linux-arch, x86, linux-kernel, Al Viro, linux-fsdevel,
linuxppc-dev, Christoph Hellwig
In-Reply-To: <202008181244.BBDA7DAB@keescook>
On Tue, Aug 18, 2020 at 12:44:49PM -0700, Kees Cook wrote:
> On Mon, Aug 17, 2020 at 09:32:09AM +0200, Christoph Hellwig wrote:
> > For 64-bit the only hing missing was a strategic _AC, and for 32-bit we
>
> typo: thing
>
> > need to use __PAGE_OFFSET instead of PAGE_OFFSET in the TASK_SIZE
> > definition to escape the explicit unsigned long cast. This just works
> > because __PAGE_OFFSET is defined using _AC itself and thus never needs
> > the cast anyway.
>
> Shouldn't this be folded into the prior patch so there's no bisection
> problem?
I didn't see a problem bisecting, do you have something particular in
mind?
^ permalink raw reply
* Re: [PATCH 03/11] fs: don't allow splice read/write without explicit ops
From: Christoph Hellwig @ 2020-08-18 19:54 UTC (permalink / raw)
To: Kees Cook
Cc: linux-arch, x86, linux-kernel, Al Viro, linux-fsdevel,
linuxppc-dev, Christoph Hellwig
In-Reply-To: <202008181239.E51B80265@keescook>
On Tue, Aug 18, 2020 at 12:39:34PM -0700, Kees Cook wrote:
> On Mon, Aug 17, 2020 at 09:32:04AM +0200, Christoph Hellwig wrote:
> > default_file_splice_write is the last piece of generic code that uses
> > set_fs to make the uaccess routines operate on kernel pointers. It
> > implements a "fallback loop" for splicing from files that do not actually
> > provide a proper splice_read method. The usual file systems and other
> > high bandwith instances all provide a ->splice_read, so this just removes
> > support for various device drivers and procfs/debugfs files. If splice
> > support for any of those turns out to be important it can be added back
> > by switching them to the iter ops and using generic_file_splice_read.
> >
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
>
> This seems a bit disruptive? I feel like this is going to make fuzzers
> really noisy (e.g. trinity likes to splice random stuff out of /sys and
> /proc).
Noisy in the sence of triggering the pr_debug or because they can't
handle -EINVAL?
^ permalink raw reply
* Re: [PATCH v2] powerpc/pseries/svm: Allocate SWIOTLB buffer anywhere in memory
From: Thiago Jung Bauermann @ 2020-08-18 19:51 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Konrad Rzeszutek Wilk, linuxppc-dev, Ram Pai, linux-kernel, iommu,
Satheesh Rajendran, Robin Murphy, Marek Szyprowski
In-Reply-To: <20200818065911.GA2324@lst.de>
Christoph Hellwig <hch@lst.de> writes:
> On Mon, Aug 17, 2020 at 06:46:58PM -0300, Thiago Jung Bauermann wrote:
>> POWER secure guests (i.e., guests which use the Protection Execution
>> Facility) need to use SWIOTLB to be able to do I/O with the hypervisor, but
>> they don't need the SWIOTLB memory to be in low addresses since the
>> hypervisor doesn't have any addressing limitation.
>>
>> This solves a SWIOTLB initialization problem we are seeing in secure guests
>> with 128 GB of RAM: they are configured with 4 GB of crashkernel reserved
>> memory, which leaves no space for SWIOTLB in low addresses.
>>
>> To do this, we use mostly the same code as swiotlb_init(), but allocate the
>> buffer using memblock_alloc() instead of memblock_alloc_low().
>>
>> We also need to add swiotlb_set_no_iotlb_memory() in order to set the
>> no_iotlb_memory flag if initialization fails.
>
> Do you really need the helper? As far as I can tell the secure guests
> very much rely on swiotlb for all I/O, so you might as well panic if
> you fail to allocate it.
That is true. Ok, I will do that.
--
Thiago Jung Bauermann
IBM Linux Technology Center
^ permalink raw reply
* Re: [PATCH 09/11] x86: remove address space overrides using set_fs()
From: Kees Cook @ 2020-08-18 19:46 UTC (permalink / raw)
To: Christoph Hellwig
Cc: linux-arch, x86, linux-kernel, Al Viro, linux-fsdevel,
linuxppc-dev
In-Reply-To: <20200817073212.830069-10-hch@lst.de>
On Mon, Aug 17, 2020 at 09:32:10AM +0200, Christoph Hellwig wrote:
> Stop providing the possibility to override the address space using
> set_fs() now that there is no need for that any more. To properly
> handle the TASK_SIZE_MAX checking for 4 vs 5-level page tables on
> x86 a new alternative is introduced, which just like the one in
> entry_64.S has to use the hardcoded virtual address bits to escape
> the fact that TASK_SIZE_MAX isn't actually a constant when 5-level
> page tables are enabled.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
Awesome. :)
Reviewed-by: Kees Cook <keescook@chromium.org>
--
Kees Cook
^ permalink raw reply
* Re: [PATCH 08/11] x86: make TASK_SIZE_MAX usable from assembly code
From: Kees Cook @ 2020-08-18 19:44 UTC (permalink / raw)
To: Christoph Hellwig
Cc: linux-arch, x86, linux-kernel, Al Viro, linux-fsdevel,
linuxppc-dev
In-Reply-To: <20200817073212.830069-9-hch@lst.de>
On Mon, Aug 17, 2020 at 09:32:09AM +0200, Christoph Hellwig wrote:
> For 64-bit the only hing missing was a strategic _AC, and for 32-bit we
typo: thing
> need to use __PAGE_OFFSET instead of PAGE_OFFSET in the TASK_SIZE
> definition to escape the explicit unsigned long cast. This just works
> because __PAGE_OFFSET is defined using _AC itself and thus never needs
> the cast anyway.
Shouldn't this be folded into the prior patch so there's no bisection
problem?
-Kees
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
> arch/x86/include/asm/page_32_types.h | 4 ++--
> arch/x86/include/asm/page_64_types.h | 2 +-
> 2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/include/asm/page_32_types.h b/arch/x86/include/asm/page_32_types.h
> index 26236925fb2c36..f462895a33e452 100644
> --- a/arch/x86/include/asm/page_32_types.h
> +++ b/arch/x86/include/asm/page_32_types.h
> @@ -44,8 +44,8 @@
> /*
> * User space process size: 3GB (default).
> */
> -#define IA32_PAGE_OFFSET PAGE_OFFSET
> -#define TASK_SIZE PAGE_OFFSET
> +#define IA32_PAGE_OFFSET __PAGE_OFFSET
> +#define TASK_SIZE __PAGE_OFFSET
> #define TASK_SIZE_LOW TASK_SIZE
> #define TASK_SIZE_MAX TASK_SIZE
> #define DEFAULT_MAP_WINDOW TASK_SIZE
> diff --git a/arch/x86/include/asm/page_64_types.h b/arch/x86/include/asm/page_64_types.h
> index 996595c9897e0a..838515daf87b36 100644
> --- a/arch/x86/include/asm/page_64_types.h
> +++ b/arch/x86/include/asm/page_64_types.h
> @@ -76,7 +76,7 @@
> *
> * With page table isolation enabled, we map the LDT in ... [stay tuned]
> */
> -#define TASK_SIZE_MAX ((1UL << __VIRTUAL_MASK_SHIFT) - PAGE_SIZE)
> +#define TASK_SIZE_MAX ((_AC(1,UL) << __VIRTUAL_MASK_SHIFT) - PAGE_SIZE)
>
> #define DEFAULT_MAP_WINDOW ((1UL << 47) - PAGE_SIZE)
>
> --
> 2.28.0
>
--
Kees Cook
^ permalink raw reply
* Re: [PATCH 05/11] test_bitmap: skip user bitmap tests for !CONFIG_SET_FS
From: Kees Cook @ 2020-08-18 19:43 UTC (permalink / raw)
To: Christoph Hellwig
Cc: linux-arch, x86, linux-kernel, Al Viro, linux-fsdevel,
linuxppc-dev
In-Reply-To: <20200817073212.830069-6-hch@lst.de>
On Mon, Aug 17, 2020 at 09:32:06AM +0200, Christoph Hellwig wrote:
> We can't run the tests for userspace bitmap parsing if set_fs() doesn't
> exist.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Kees Cook <keescook@chromium.org>
--
Kees Cook
^ permalink raw reply
* Re: [PATCH 04/11] uaccess: add infrastructure for kernel builds with set_fs()
From: Kees Cook @ 2020-08-18 19:40 UTC (permalink / raw)
To: Christoph Hellwig
Cc: linux-arch, x86, linux-kernel, Al Viro, linux-fsdevel,
linuxppc-dev
In-Reply-To: <20200817073212.830069-5-hch@lst.de>
On Mon, Aug 17, 2020 at 09:32:05AM +0200, Christoph Hellwig wrote:
> Add a CONFIG_SET_FS option that is selected by architecturess that
> implement set_fs, which is all of them initially. If the option is not
> set stubs for routines related to overriding the address space are
> provided so that architectures can start to opt out of providing set_fs.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Kees Cook <keescook@chromium.org>
--
Kees Cook
^ permalink raw reply
* Re: [PATCH 03/11] fs: don't allow splice read/write without explicit ops
From: Kees Cook @ 2020-08-18 19:39 UTC (permalink / raw)
To: Christoph Hellwig
Cc: linux-arch, x86, linux-kernel, Al Viro, linux-fsdevel,
linuxppc-dev
In-Reply-To: <20200817073212.830069-4-hch@lst.de>
On Mon, Aug 17, 2020 at 09:32:04AM +0200, Christoph Hellwig wrote:
> default_file_splice_write is the last piece of generic code that uses
> set_fs to make the uaccess routines operate on kernel pointers. It
> implements a "fallback loop" for splicing from files that do not actually
> provide a proper splice_read method. The usual file systems and other
> high bandwith instances all provide a ->splice_read, so this just removes
> support for various device drivers and procfs/debugfs files. If splice
> support for any of those turns out to be important it can be added back
> by switching them to the iter ops and using generic_file_splice_read.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
This seems a bit disruptive? I feel like this is going to make fuzzers
really noisy (e.g. trinity likes to splice random stuff out of /sys and
/proc).
Conceptually, though:
Reviewed-by: Kees Cook <keescook@chromium.org>
--
Kees Cook
^ permalink raw reply
* Re: [PATCH] powerpc/pseries: Do not initiate shutdown when system is running on UPS
From: Tyrel Datwyler @ 2020-08-18 19:35 UTC (permalink / raw)
To: Vasant Hegde, linuxppc-dev; +Cc: stable
In-Reply-To: <20200818105424.234108-1-hegdevasant@linux.vnet.ibm.com>
On 8/18/20 3:54 AM, Vasant Hegde wrote:
> As per PAPR specification whenever system is running on UPS we have to
> wait for predefined time (default 10mins) before initiating shutdown.
The wording in PAPR seems a little unclear. It states for an
EPOW_SYSTEM_SHUTDOWN action code that an EPOW error should be logged followed by
scheduling a shutdown to begin after an OS defined delay interval (with 10
minutes the suggested default).
However, the modifier code descriptions seems to imply that a normal shutdown is
the only one that should happen with no additional delay.
For EPOW sensor value = 3 (EPOW_SYSTEM_SHUTDOWN)
0x01 = Normal system shutdown with no additional delay
0x02 = Loss of utility power, system is running on UPS/Battery
0x03 = Loss of system critical functions, system should be shutdown
0x04 = Ambient temperature too high
For 0x03-0x04 we also do an orderly_poweroff().
Not sure if it really matters, but I was curious and this is just what I gleaned
from glancing at PAPR.
-Tyrel
>
> We have user space tool (rtas_errd) to monitor for EPOW events and
> initiate shutdown after predefined time. Hence do not initiate shutdown
> whenever we get EPOW_SHUTDOWN_ON_UPS event.
>
> Fixes: 79872e35 (powerpc/pseries: All events of EPOW_SYSTEM_SHUTDOWN must initiate shutdown)
> Cc: stable@vger.kernel.org # v4.0+
> Cc: Michael Ellerman <mpe@ellerman.id.au>
> Signed-off-by: Vasant Hegde <hegdevasant@linux.vnet.ibm.com>
> ---
> arch/powerpc/platforms/pseries/ras.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/arch/powerpc/platforms/pseries/ras.c b/arch/powerpc/platforms/pseries/ras.c
> index f3736fcd98fc..13c86a292c6d 100644
> --- a/arch/powerpc/platforms/pseries/ras.c
> +++ b/arch/powerpc/platforms/pseries/ras.c
> @@ -184,7 +184,6 @@ static void handle_system_shutdown(char event_modifier)
> case EPOW_SHUTDOWN_ON_UPS:
> pr_emerg("Loss of system power detected. System is running on"
> " UPS/battery. Check RTAS error log for details\n");
> - orderly_poweroff(true);
> break;
>
> case EPOW_SHUTDOWN_LOSS_OF_CRITICAL_FUNCTIONS:
>
^ permalink raw reply
* Re: [PATCH 02/11] fs: don't allow kernel reads and writes without iter ops
From: Kees Cook @ 2020-08-18 19:34 UTC (permalink / raw)
To: Christoph Hellwig
Cc: linux-arch, x86, linux-kernel, Al Viro, linux-fsdevel,
linuxppc-dev
In-Reply-To: <20200817073212.830069-3-hch@lst.de>
On Mon, Aug 17, 2020 at 09:32:03AM +0200, Christoph Hellwig wrote:
> Don't allow calling ->read or ->write with set_fs as a preparation for
> killing off set_fs. All the instances that we use kernel_read/write on
> are using the iter ops already.
>
> If a file has both the regular ->read/->write methods and the iter
> variants those could have different semantics for messed up enough
> drivers. Also fails the kernel access to them in that case.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Kees Cook <keescook@chromium.org>
--
Kees Cook
^ permalink raw reply
* Re: [PATCH 01/11] mem: remove duplicate ops for /dev/zero and /dev/null
From: Kees Cook @ 2020-08-18 19:33 UTC (permalink / raw)
To: Christoph Hellwig
Cc: linux-arch, x86, linux-kernel, Al Viro, linux-fsdevel,
linuxppc-dev
In-Reply-To: <20200817073212.830069-2-hch@lst.de>
On Mon, Aug 17, 2020 at 09:32:02AM +0200, Christoph Hellwig wrote:
> There is no good reason to implement both the traditional ->read and
> ->write as well as the iter based ops. So implement just the iter
> based ones.
>
> Suggested-by: Al Viro <viro@zeniv.linux.org.uk>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Kees Cook <keescook@chromium.org>
--
Kees Cook
^ permalink raw reply
* Re: [PATCH 06/11] lkdtm: disable set_fs-based tests for !CONFIG_SET_FS
From: Kees Cook @ 2020-08-18 19:32 UTC (permalink / raw)
To: Christoph Hellwig
Cc: linux-arch, x86, linux-kernel, Al Viro, linux-fsdevel,
linuxppc-dev
In-Reply-To: <20200817073212.830069-7-hch@lst.de>
On Mon, Aug 17, 2020 at 09:32:07AM +0200, Christoph Hellwig wrote:
> Once we can't manipulate the address limit, we also can't test what
> happens when the manipulation is abused.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
> drivers/misc/lkdtm/bugs.c | 2 ++
> drivers/misc/lkdtm/core.c | 4 ++++
> drivers/misc/lkdtm/usercopy.c | 2 ++
> 3 files changed, 8 insertions(+)
>
> diff --git a/drivers/misc/lkdtm/bugs.c b/drivers/misc/lkdtm/bugs.c
> index 4dfbfd51bdf774..66f1800b1cb82d 100644
> --- a/drivers/misc/lkdtm/bugs.c
> +++ b/drivers/misc/lkdtm/bugs.c
> @@ -312,6 +312,7 @@ void lkdtm_CORRUPT_LIST_DEL(void)
> pr_err("list_del() corruption not detected!\n");
> }
>
> +#ifdef CONFIG_SET_FS
> /* Test if unbalanced set_fs(KERNEL_DS)/set_fs(USER_DS) check exists. */
> void lkdtm_CORRUPT_USER_DS(void)
> {
> @@ -321,6 +322,7 @@ void lkdtm_CORRUPT_USER_DS(void)
> /* Make sure we do not keep running with a KERNEL_DS! */
> force_sig(SIGKILL);
> }
> +#endif
Please let the test defined, but it should XFAIL with a message about
the CONFIG (see similar ifdefs in lkdtm).
> /* Test that VMAP_STACK is actually allocating with a leading guard page */
> void lkdtm_STACK_GUARD_PAGE_LEADING(void)
> diff --git a/drivers/misc/lkdtm/core.c b/drivers/misc/lkdtm/core.c
> index a5e344df916632..aae08b33a7ee2a 100644
> --- a/drivers/misc/lkdtm/core.c
> +++ b/drivers/misc/lkdtm/core.c
> @@ -112,7 +112,9 @@ static const struct crashtype crashtypes[] = {
> CRASHTYPE(CORRUPT_STACK_STRONG),
> CRASHTYPE(CORRUPT_LIST_ADD),
> CRASHTYPE(CORRUPT_LIST_DEL),
> +#ifdef CONFIG_SET_FS
> CRASHTYPE(CORRUPT_USER_DS),
> +#endif
> CRASHTYPE(STACK_GUARD_PAGE_LEADING),
> CRASHTYPE(STACK_GUARD_PAGE_TRAILING),
> CRASHTYPE(UNSET_SMEP),
> @@ -172,7 +174,9 @@ static const struct crashtype crashtypes[] = {
> CRASHTYPE(USERCOPY_STACK_FRAME_FROM),
> CRASHTYPE(USERCOPY_STACK_BEYOND),
> CRASHTYPE(USERCOPY_KERNEL),
> +#ifdef CONFIG_SET_FS
> CRASHTYPE(USERCOPY_KERNEL_DS),
> +#endif
> CRASHTYPE(STACKLEAK_ERASING),
> CRASHTYPE(CFI_FORWARD_PROTO),
Then none of these are needed.
> #ifdef CONFIG_X86_32
Hmpf, this ifdef was missed in ae56942c1474 ("lkdtm: Make arch-specific
tests always available"). I will fix that.
> diff --git a/drivers/misc/lkdtm/usercopy.c b/drivers/misc/lkdtm/usercopy.c
> index b833367a45d053..4b632fe79ab6bb 100644
> --- a/drivers/misc/lkdtm/usercopy.c
> +++ b/drivers/misc/lkdtm/usercopy.c
> @@ -325,6 +325,7 @@ void lkdtm_USERCOPY_KERNEL(void)
> vm_munmap(user_addr, PAGE_SIZE);
> }
>
> +#ifdef CONFIG_SET_FS
> void lkdtm_USERCOPY_KERNEL_DS(void)
> {
> char __user *user_ptr =
> @@ -339,6 +340,7 @@ void lkdtm_USERCOPY_KERNEL_DS(void)
> pr_err("copy_to_user() to noncanonical address succeeded!?\n");
> set_fs(old_fs);
> }
> +#endif
(Same here, please.)
>
> void __init lkdtm_usercopy_init(void)
> {
> --
> 2.28.0
>
--
Kees Cook
^ permalink raw reply
* Re: [PATCH 07/11] x86: move PAGE_OFFSET, TASK_SIZE & friends to page_{32,64}_types.h
From: Kees Cook @ 2020-08-18 19:27 UTC (permalink / raw)
To: Christoph Hellwig
Cc: linux-arch, x86, linux-kernel, Al Viro, linux-fsdevel,
linuxppc-dev
In-Reply-To: <20200817073212.830069-8-hch@lst.de>
On Mon, Aug 17, 2020 at 09:32:08AM +0200, Christoph Hellwig wrote:
> At least for 64-bit this moves them closer to some of the defines
> they are based on, and it prepares for using the TASK_SIZE_MAX
> definition from assembly.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Kees Cook <keescook@chromium.org>
--
Kees Cook
^ permalink raw reply
* Re: remove the last set_fs() in common code, and remove it for x86 and powerpc
From: Christophe Leroy @ 2020-08-18 18:23 UTC (permalink / raw)
To: Christoph Hellwig
Cc: linux-arch, Kees Cook, x86, linux-kernel, Al Viro, linux-fsdevel,
linuxppc-dev
In-Reply-To: <20200818180555.GA29185@lst.de>
Le 18/08/2020 à 20:05, Christoph Hellwig a écrit :
> On Tue, Aug 18, 2020 at 07:46:22PM +0200, Christophe Leroy wrote:
>> I gave it a go on my powerpc mpc832x. I tested it on top of my newest
>> series that reworks the 32 bits signal handlers (see
>> https://patchwork.ozlabs.org/project/linuxppc-dev/list/?series=196278) with
>> the microbenchmark test used is that series.
>>
>> With KUAP activated, on top of signal32 rework, performance is boosted as
>> system time for the microbenchmark goes from 1.73s down to 1.56s, that is
>> 10% quicker
>>
>> Surprisingly, with the kernel as is today without my signal's series, your
>> series degrades performance slightly (from 2.55s to 2.64s ie 3.5% slower).
>>
>>
>> I also observe, in both cases, a degradation on
>>
>> dd if=/dev/zero of=/dev/null count=1M
>>
>> Without your series, it runs in 5.29 seconds.
>> With your series, it runs in 5.82 seconds, that is 10% more time.
>
> That's pretty strage, I wonder if some kernel text cache line
> effects come into play here?
>
> The kernel access side is only used in slow path code, so it should
> not make a difference, and the uaccess code is simplified and should be
> (marginally) faster.
>
> Btw, was this with the __{get,put}_user_allowed cockup that you noticed
> fixed?
>
Yes it is with the __get_user_size() replaced by __get_user_size_allowed().
Christophe
^ permalink raw reply
* Re: remove the last set_fs() in common code, and remove it for x86 and powerpc
From: Christoph Hellwig @ 2020-08-18 18:05 UTC (permalink / raw)
To: Christophe Leroy
Cc: linux-arch, Kees Cook, x86, linux-kernel, Al Viro, linux-fsdevel,
linuxppc-dev, Christoph Hellwig
In-Reply-To: <319d15b1-cb4a-a7b4-3082-12bb30eb5143@csgroup.eu>
On Tue, Aug 18, 2020 at 07:46:22PM +0200, Christophe Leroy wrote:
> I gave it a go on my powerpc mpc832x. I tested it on top of my newest
> series that reworks the 32 bits signal handlers (see
> https://patchwork.ozlabs.org/project/linuxppc-dev/list/?series=196278) with
> the microbenchmark test used is that series.
>
> With KUAP activated, on top of signal32 rework, performance is boosted as
> system time for the microbenchmark goes from 1.73s down to 1.56s, that is
> 10% quicker
>
> Surprisingly, with the kernel as is today without my signal's series, your
> series degrades performance slightly (from 2.55s to 2.64s ie 3.5% slower).
>
>
> I also observe, in both cases, a degradation on
>
> dd if=/dev/zero of=/dev/null count=1M
>
> Without your series, it runs in 5.29 seconds.
> With your series, it runs in 5.82 seconds, that is 10% more time.
That's pretty strage, I wonder if some kernel text cache line
effects come into play here?
The kernel access side is only used in slow path code, so it should
not make a difference, and the uaccess code is simplified and should be
(marginally) faster.
Btw, was this with the __{get,put}_user_allowed cockup that you noticed
fixed?
^ permalink raw reply
* Re: remove the last set_fs() in common code, and remove it for x86 and powerpc
From: Christophe Leroy @ 2020-08-18 17:46 UTC (permalink / raw)
To: Christoph Hellwig, Al Viro, Michael Ellerman, x86
Cc: linux-fsdevel, linux-arch, linuxppc-dev, Kees Cook, linux-kernel
In-Reply-To: <20200817073212.830069-1-hch@lst.de>
Le 17/08/2020 à 09:32, Christoph Hellwig a écrit :
> Hi all,
>
> this series removes the last set_fs() used to force a kernel address
> space for the uaccess code in the kernel read/write/splice code, and then
> stops implementing the address space overrides entirely for x86 and
> powerpc.
>
> The file system part has been posted a few times, and the read/write side
> has been pretty much unchanced. For splice this series drops the
> conversion of the seq_file and sysctl code to the iter ops, and thus loses
> the splice support for them. The reasons for that is that it caused a lot
> of churn for not much use - splice for these small files really isn't much
> of a win, even if existing userspace uses it. All callers I found do the
> proper fallback, but if this turns out to be an issue the conversion can
> be resurrected.
I like this series.
I gave it a go on my powerpc mpc832x. I tested it on top of my newest
series that reworks the 32 bits signal handlers (see
https://patchwork.ozlabs.org/project/linuxppc-dev/list/?series=196278)
with the microbenchmark test used is that series.
With KUAP activated, on top of signal32 rework, performance is boosted
as system time for the microbenchmark goes from 1.73s down to 1.56s,
that is 10% quicker
Surprisingly, with the kernel as is today without my signal's series,
your series degrades performance slightly (from 2.55s to 2.64s ie 3.5%
slower).
I also observe, in both cases, a degradation on
dd if=/dev/zero of=/dev/null count=1M
Without your series, it runs in 5.29 seconds.
With your series, it runs in 5.82 seconds, that is 10% more time.
Christophe
>
> Besides x86 and powerpc I plan to eventually convert all other
> architectures, although this will be a slow process, starting with the
> easier ones once the infrastructure is merged. The process to convert
> architectures is roughtly:
>
> - ensure there is no set_fs(KERNEL_DS) left in arch specific code
> - implement __get_kernel_nofault and __put_kernel_nofault
> - remove the arch specific address limitation functionality
>
> Diffstat:
> arch/Kconfig | 3
> arch/alpha/Kconfig | 1
> arch/arc/Kconfig | 1
> arch/arm/Kconfig | 1
> arch/arm64/Kconfig | 1
> arch/c6x/Kconfig | 1
> arch/csky/Kconfig | 1
> arch/h8300/Kconfig | 1
> arch/hexagon/Kconfig | 1
> arch/ia64/Kconfig | 1
> arch/m68k/Kconfig | 1
> arch/microblaze/Kconfig | 1
> arch/mips/Kconfig | 1
> arch/nds32/Kconfig | 1
> arch/nios2/Kconfig | 1
> arch/openrisc/Kconfig | 1
> arch/parisc/Kconfig | 1
> arch/powerpc/include/asm/processor.h | 7 -
> arch/powerpc/include/asm/thread_info.h | 5 -
> arch/powerpc/include/asm/uaccess.h | 78 ++++++++-----------
> arch/powerpc/kernel/signal.c | 3
> arch/powerpc/lib/sstep.c | 6 -
> arch/riscv/Kconfig | 1
> arch/s390/Kconfig | 1
> arch/sh/Kconfig | 1
> arch/sparc/Kconfig | 1
> arch/um/Kconfig | 1
> arch/x86/ia32/ia32_aout.c | 1
> arch/x86/include/asm/page_32_types.h | 11 ++
> arch/x86/include/asm/page_64_types.h | 38 +++++++++
> arch/x86/include/asm/processor.h | 60 ---------------
> arch/x86/include/asm/thread_info.h | 2
> arch/x86/include/asm/uaccess.h | 26 ------
> arch/x86/kernel/asm-offsets.c | 3
> arch/x86/lib/getuser.S | 28 ++++---
> arch/x86/lib/putuser.S | 21 +++--
> arch/xtensa/Kconfig | 1
> drivers/char/mem.c | 16 ----
> drivers/misc/lkdtm/bugs.c | 2
> drivers/misc/lkdtm/core.c | 4 +
> drivers/misc/lkdtm/usercopy.c | 2
> fs/read_write.c | 69 ++++++++++-------
> fs/splice.c | 130 +++------------------------------
> include/linux/fs.h | 2
> include/linux/uaccess.h | 18 ++++
> lib/test_bitmap.c | 10 ++
> 46 files changed, 235 insertions(+), 332 deletions(-)
>
^ permalink raw reply
* [PATCH v2 25/25] powerpc/signal32: Transform save_user_regs() and save_tm_user_regs() in 'unsafe' version
From: Christophe Leroy @ 2020-08-18 17:19 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <cover.1597770847.git.christophe.leroy@csgroup.eu>
Change those two functions to be used within a user access block.
For that, change save_general_regs() to and unsafe_save_general_regs(),
then replace all user accesses by unsafe_ versions.
This series leads to a reduction from 2.55s to 1.73s of
the system CPU time with the following microbench app
on an mpc832x with KUAP (approx 32%)
Without KUAP, the difference is in the noise.
void sigusr1(int sig) { }
int main(int argc, char **argv)
{
int i = 100000;
signal(SIGUSR1, sigusr1);
for (;i--;)
raise(SIGUSR1);
exit(0);
}
An additional 0.10s reduction is achieved by removing
CONFIG_PPC_FPU, as the mpc832x has no FPU.
A bit less spectacular on an 8xx as KUAP is less heavy, prior to
the series (with KUAP) it ran in 8.10 ms. Once applies the removal
of FPU regs handling, we get 7.05s. With the full series, we get 6.9s.
If artificially re-activating FPU regs handling with the full series,
we get 7.6s.
So for the 8xx, the removal of the FPU regs copy is what makes the
difference, but the rework of handle_signal also have a benefit.
Same as above, without KUAP the difference is in the noise.
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
arch/powerpc/kernel/signal_32.c | 224 ++++++++++++++++----------------
1 file changed, 111 insertions(+), 113 deletions(-)
diff --git a/arch/powerpc/kernel/signal_32.c b/arch/powerpc/kernel/signal_32.c
index 86539a4e0514..f795fe0240a1 100644
--- a/arch/powerpc/kernel/signal_32.c
+++ b/arch/powerpc/kernel/signal_32.c
@@ -93,8 +93,8 @@ static inline int get_sigset_t(sigset_t *set,
#define to_user_ptr(p) ptr_to_compat(p)
#define from_user_ptr(p) compat_ptr(p)
-static inline int save_general_regs(struct pt_regs *regs,
- struct mcontext __user *frame)
+static __always_inline int
+save_general_regs_unsafe(struct pt_regs *regs, struct mcontext __user *frame)
{
elf_greg_t64 *gregs = (elf_greg_t64 *)regs;
int val, i;
@@ -108,10 +108,12 @@ static inline int save_general_regs(struct pt_regs *regs,
else
val = gregs[i];
- if (__put_user(val, &frame->mc_gregs[i]))
- return -EFAULT;
+ unsafe_put_user(val, &frame->mc_gregs[i], failed);
}
return 0;
+
+failed:
+ return 1;
}
static inline int restore_general_regs(struct pt_regs *regs,
@@ -148,11 +150,15 @@ static inline int get_sigset_t(sigset_t *set, const sigset_t __user *uset)
#define to_user_ptr(p) ((unsigned long)(p))
#define from_user_ptr(p) ((void __user *)(p))
-static inline int save_general_regs(struct pt_regs *regs,
- struct mcontext __user *frame)
+static __always_inline int
+save_general_regs_unsafe(struct pt_regs *regs, struct mcontext __user *frame)
{
WARN_ON(!FULL_REGS(regs));
- return __copy_to_user(&frame->mc_gregs, regs, GP_REGS_SIZE);
+ unsafe_copy_to_user(&frame->mc_gregs, regs, GP_REGS_SIZE, failed);
+ return 0;
+
+failed:
+ return 1;
}
static inline int restore_general_regs(struct pt_regs *regs,
@@ -170,6 +176,11 @@ static inline int restore_general_regs(struct pt_regs *regs,
}
#endif
+#define unsafe_save_general_regs(regs, frame, label) do { \
+ if (save_general_regs_unsafe(regs, frame)) \
+ goto label; \
+} while (0)
+
/*
* When we have signals to deliver, we set up on the
* user stack, going down from the original stack pointer:
@@ -249,21 +260,19 @@ static void prepare_save_user_regs(int ctx_has_vsx_region)
#endif
}
-static int save_user_regs(struct pt_regs *regs, struct mcontext __user *frame,
- struct mcontext __user *tm_frame, int ctx_has_vsx_region)
+static int save_user_regs_unsafe(struct pt_regs *regs, struct mcontext __user *frame,
+ struct mcontext __user *tm_frame, int ctx_has_vsx_region)
{
unsigned long msr = regs->msr;
/* save general registers */
- if (save_general_regs(regs, frame))
- return 1;
+ unsafe_save_general_regs(regs, frame, failed);
#ifdef CONFIG_ALTIVEC
/* save altivec registers */
if (current->thread.used_vr) {
- if (__copy_to_user(&frame->mc_vregs, ¤t->thread.vr_state,
- ELF_NVRREG * sizeof(vector128)))
- return 1;
+ unsafe_copy_to_user(&frame->mc_vregs, ¤t->thread.vr_state,
+ ELF_NVRREG * sizeof(vector128), failed);
/* set MSR_VEC in the saved MSR value to indicate that
frame->mc_vregs contains valid data */
msr |= MSR_VEC;
@@ -276,11 +285,10 @@ static int save_user_regs(struct pt_regs *regs, struct mcontext __user *frame,
* most significant bits of that same vector. --BenH
* Note that the current VRSAVE value is in the SPR at this point.
*/
- if (__put_user(current->thread.vrsave, (u32 __user *)&frame->mc_vregs[32]))
- return 1;
+ unsafe_put_user(current->thread.vrsave, (u32 __user *)&frame->mc_vregs[32],
+ failed);
#endif /* CONFIG_ALTIVEC */
- if (copy_fpr_to_user(&frame->mc_fregs, current))
- return 1;
+ unsafe_copy_fpr_to_user(&frame->mc_fregs, current, failed);
/*
* Clear the MSR VSX bit to indicate there is no valid state attached
@@ -295,17 +303,15 @@ static int save_user_regs(struct pt_regs *regs, struct mcontext __user *frame,
* contains valid data
*/
if (current->thread.used_vsr && ctx_has_vsx_region) {
- if (copy_vsx_to_user(&frame->mc_vsregs, current))
- return 1;
+ unsafe_copy_vsx_to_user(&frame->mc_vsregs, current, failed);
msr |= MSR_VSX;
}
#endif /* CONFIG_VSX */
#ifdef CONFIG_SPE
/* save spe registers */
if (current->thread.used_spe) {
- if (__copy_to_user(&frame->mc_vregs, current->thread.evr,
- ELF_NEVRREG * sizeof(u32)))
- return 1;
+ unsafe_copy_to_user(&frame->mc_vregs, current->thread.evr,
+ ELF_NEVRREG * sizeof(u32)), failed);
/* set MSR_SPE in the saved MSR value to indicate that
frame->mc_vregs contains valid data */
msr |= MSR_SPE;
@@ -313,21 +319,29 @@ static int save_user_regs(struct pt_regs *regs, struct mcontext __user *frame,
/* else assert((regs->msr & MSR_SPE) == 0) */
/* We always copy to/from spefscr */
- if (__put_user(current->thread.spefscr, (u32 __user *)&frame->mc_vregs + ELF_NEVRREG))
- return 1;
+ unsafe_put_user(current->thread.spefscr,
+ (u32 __user *)&frame->mc_vregs + ELF_NEVRREG, failed);
#endif /* CONFIG_SPE */
- if (__put_user(msr, &frame->mc_gregs[PT_MSR]))
- return 1;
+ unsafe_put_user(msr, &frame->mc_gregs[PT_MSR], failed);
+
/* We need to write 0 the MSR top 32 bits in the tm frame so that we
* can check it on the restore to see if TM is active
*/
- if (tm_frame && __put_user(0, &tm_frame->mc_gregs[PT_MSR]))
- return 1;
+ if (tm_frame)
+ unsafe_put_user(0, &tm_frame->mc_gregs[PT_MSR], failed);
return 0;
+
+failed:
+ return 1;
}
+#define unsafe_save_user_regs(regs, frame, tm_frame, has_vsx, label) do { \
+ if (save_user_regs_unsafe(regs, frame, tm_frame, has_vsx)) \
+ goto label; \
+} while (0)
+
#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
/*
* Save the current user registers on the user stack.
@@ -336,7 +350,7 @@ static int save_user_regs(struct pt_regs *regs, struct mcontext __user *frame,
* We also save the transactional registers to a second ucontext in the
* frame.
*
- * See save_user_regs() and signal_64.c:setup_tm_sigcontexts().
+ * See save_user_regs_unsafe() and signal_64.c:setup_tm_sigcontexts().
*/
static void prepare_save_tm_user_regs(void)
{
@@ -352,13 +366,12 @@ static void prepare_save_tm_user_regs(void)
#endif
}
-static int save_tm_user_regs(struct pt_regs *regs, struct mcontext __user *frame,
- struct mcontext __user *tm_frame, unsigned long msr)
+static int save_tm_user_regs_unsafe(struct pt_regs *regs, struct mcontext __user *frame,
+ struct mcontext __user *tm_frame, unsigned long msr)
{
/* Save both sets of general registers */
- if (save_general_regs(¤t->thread.ckpt_regs, frame)
- || save_general_regs(regs, tm_frame))
- return 1;
+ unsafe_save_general_regs(¤t->thread.ckpt_regs, frame, failed);
+ unsafe_save_general_regs(regs, tm_frame, failed);
/* Stash the top half of the 64bit MSR into the 32bit MSR word
* of the transactional mcontext. This way we have a backward-compatible
@@ -366,26 +379,21 @@ static int save_tm_user_regs(struct pt_regs *regs, struct mcontext __user *frame
* also look at what type of transaction (T or S) was active at the
* time of the signal.
*/
- if (__put_user((msr >> 32), &tm_frame->mc_gregs[PT_MSR]))
- return 1;
+ unsafe_put_user((msr >> 32), &tm_frame->mc_gregs[PT_MSR], failed);
#ifdef CONFIG_ALTIVEC
/* save altivec registers */
if (current->thread.used_vr) {
- if (__copy_to_user(&frame->mc_vregs, ¤t->thread.ckvr_state,
- ELF_NVRREG * sizeof(vector128)))
- return 1;
- if (msr & MSR_VEC) {
- if (__copy_to_user(&tm_frame->mc_vregs,
- ¤t->thread.vr_state,
- ELF_NVRREG * sizeof(vector128)))
- return 1;
- } else {
- if (__copy_to_user(&tm_frame->mc_vregs,
- ¤t->thread.ckvr_state,
- ELF_NVRREG * sizeof(vector128)))
- return 1;
- }
+ unsafe_copy_to_user(&frame->mc_vregs, ¤t->thread.ckvr_state,
+ ELF_NVRREG * sizeof(vector128), failed);
+ if (msr & MSR_VEC)
+ unsafe_copy_to_user(&tm_frame->mc_vregs,
+ ¤t->thread.vr_state,
+ ELF_NVRREG * sizeof(vector128), failed);
+ else
+ unsafe_copy_to_user(&tm_frame->mc_vregs,
+ ¤t->thread.ckvr_state,
+ ELF_NVRREG * sizeof(vector128), failed);
/* set MSR_VEC in the saved MSR value to indicate that
* frame->mc_vregs contains valid data
@@ -398,29 +406,21 @@ static int save_tm_user_regs(struct pt_regs *regs, struct mcontext __user *frame
* significant bits of a vector, we "cheat" and stuff VRSAVE in the
* most significant bits of that same vector. --BenH
*/
- if (__put_user(current->thread.ckvrsave,
- (u32 __user *)&frame->mc_vregs[32]))
- return 1;
- if (msr & MSR_VEC) {
- if (__put_user(current->thread.vrsave,
- (u32 __user *)&tm_frame->mc_vregs[32]))
- return 1;
- } else {
- if (__put_user(current->thread.ckvrsave,
- (u32 __user *)&tm_frame->mc_vregs[32]))
- return 1;
- }
+ unsafe_put_user(current->thread.ckvrsave,
+ (u32 __user *)&frame->mc_vregs[32], failed);
+ if (msr & MSR_VEC)
+ unsafe_put_user(current->thread.vrsave,
+ (u32 __user *)&tm_frame->mc_vregs[32], failed);
+ else
+ unsafe_put_user(current->thread.ckvrsave,
+ (u32 __user *)&tm_frame->mc_vregs[32], failed);
#endif /* CONFIG_ALTIVEC */
- if (copy_ckfpr_to_user(&frame->mc_fregs, current))
- return 1;
- if (msr & MSR_FP) {
- if (copy_fpr_to_user(&tm_frame->mc_fregs, current))
- return 1;
- } else {
- if (copy_ckfpr_to_user(&tm_frame->mc_fregs, current))
- return 1;
- }
+ unsafe_copy_ckfpr_to_user(&frame->mc_fregs, current, failed);
+ if (msr & MSR_FP)
+ unsafe_copy_fpr_to_user(&tm_frame->mc_fregs, current, failed);
+ else
+ unsafe_copy_ckfpr_to_user(&tm_frame->mc_fregs, current, failed);
#ifdef CONFIG_VSX
/*
@@ -430,53 +430,54 @@ static int save_tm_user_regs(struct pt_regs *regs, struct mcontext __user *frame
* contains valid data
*/
if (current->thread.used_vsr) {
- if (copy_ckvsx_to_user(&frame->mc_vsregs, current))
- return 1;
- if (msr & MSR_VSX) {
- if (copy_vsx_to_user(&tm_frame->mc_vsregs,
- current))
- return 1;
- } else {
- if (copy_ckvsx_to_user(&tm_frame->mc_vsregs, current))
- return 1;
- }
+ unsafe_copy_ckvsx_to_user(&frame->mc_vsregs, current, failed);
+ if (msr & MSR_VSX)
+ unsafe_copy_vsx_to_user(&tm_frame->mc_vsregs, current, failed);
+ else
+ unsafe_copy_ckvsx_to_user(&tm_frame->mc_vsregs, current, failed);
msr |= MSR_VSX;
}
#endif /* CONFIG_VSX */
#ifdef CONFIG_SPE
/* SPE regs are not checkpointed with TM, so this section is
- * simply the same as in save_user_regs().
+ * simply the same as in save_user_regs_unsafe().
*/
if (current->thread.used_spe) {
- if (__copy_to_user(&frame->mc_vregs, current->thread.evr,
- ELF_NEVRREG * sizeof(u32)))
- return 1;
+ unsafe_copy_to_user(&frame->mc_vregs, current->thread.evr,
+ ELF_NEVRREG * sizeof(u32), failed);
/* set MSR_SPE in the saved MSR value to indicate that
* frame->mc_vregs contains valid data */
msr |= MSR_SPE;
}
/* We always copy to/from spefscr */
- if (__put_user(current->thread.spefscr, (u32 __user *)&frame->mc_vregs + ELF_NEVRREG))
- return 1;
+ unsafe_put_user(current->thread.spefscr,
+ (u32 __user *)&frame->mc_vregs + ELF_NEVRREG, failed);
#endif /* CONFIG_SPE */
- if (__put_user(msr, &frame->mc_gregs[PT_MSR]))
- return 1;
+ unsafe_put_user(msr, &frame->mc_gregs[PT_MSR], failed);
return 0;
+
+failed:
+ return 1;
}
#else
static void prepare_save_tm_user_regs(void) { }
-static int save_tm_user_regs(struct pt_regs *regs, struct mcontext __user *frame,
- struct mcontext __user *tm_frame, unsigned long msr)
+static int save_tm_user_regs_unsafe(struct pt_regs *regs, struct mcontext __user *frame,
+ struct mcontext __user *tm_frame, unsigned long msr)
{
return 0;
}
#endif
+#define unsafe_save_tm_user_regs(regs, frame, tm_frame, msr, label) do { \
+ if (save_tm_user_regs_unsafe(regs, frame, tm_frame, msr)) \
+ goto label; \
+} while (0)
+
/*
* Restore the current user register values from the user stack,
* (except for MSR).
@@ -769,6 +770,11 @@ int handle_rt_signal32(struct ksignal *ksig, sigset_t *oldset,
#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
tm_mctx = &frame->uc_transact.uc_mcontext;
#endif
+ if (MSR_TM_ACTIVE(msr))
+ prepare_save_tm_user_regs();
+ else
+ prepare_save_user_regs(1);
+
if (!user_write_access_begin(frame, sizeof(*frame)))
goto badframe;
@@ -788,8 +794,10 @@ int handle_rt_signal32(struct ksignal *ksig, sigset_t *oldset,
unsafe_put_user((unsigned long)tm_mctx,
&frame->uc_transact.uc_regs, failed);
#endif
+ unsafe_save_tm_user_regs(regs, mctx, tm_mctx, msr, failed);
} else {
unsafe_put_user(0, &frame->uc.uc_link, failed);
+ unsafe_save_user_regs(regs, mctx, tm_mctx, 1, failed);
}
/* Save user registers on the stack */
@@ -812,15 +820,6 @@ int handle_rt_signal32(struct ksignal *ksig, sigset_t *oldset,
if (tramp == (unsigned long)mctx->mc_pad)
flush_icache_range(tramp, tramp + 2 * sizeof(unsigned long));
- if (MSR_TM_ACTIVE(msr)) {
- prepare_save_tm_user_regs();
- if (save_tm_user_regs(regs, mctx, tm_mctx, msr))
- goto badframe;
- } else {
- prepare_save_user_regs(1);
- if (save_user_regs(regs, mctx, tm_mctx, 1))
- goto badframe;
- }
regs->link = tramp;
#ifdef CONFIG_PPC_FPU_REGS
@@ -875,6 +874,11 @@ int handle_signal32(struct ksignal *ksig, sigset_t *oldset,
#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
tm_mctx = &frame->mctx_transact;
#endif
+ if (MSR_TM_ACTIVE(msr))
+ prepare_save_tm_user_regs();
+ else
+ prepare_save_user_regs(1);
+
if (!user_write_access_begin(frame, sizeof(*frame)))
goto badframe;
sc = (struct sigcontext __user *) &frame->sctx;
@@ -892,6 +896,11 @@ int handle_signal32(struct ksignal *ksig, sigset_t *oldset,
unsafe_put_user(to_user_ptr(mctx), &sc->regs, failed);
unsafe_put_user(ksig->sig, &sc->signal, failed);
+ if (MSR_TM_ACTIVE(msr))
+ unsafe_save_tm_user_regs(regs, mctx, tm_mctx, msr, failed);
+ else
+ unsafe_save_user_regs(regs, mctx, tm_mctx, 1, failed);
+
if (vdso32_sigtramp && tsk->mm->context.vdso_base) {
tramp = tsk->mm->context.vdso_base + vdso32_sigtramp;
} else {
@@ -905,16 +914,6 @@ int handle_signal32(struct ksignal *ksig, sigset_t *oldset,
if (tramp == (unsigned long)mctx->mc_pad)
flush_icache_range(tramp, tramp + 2 * sizeof(unsigned long));
- if (MSR_TM_ACTIVE(msr)) {
- prepare_save_tm_user_regs();
- if (save_tm_user_regs(regs, mctx, tm_mctx, msr))
- goto badframe;
- } else {
- prepare_save_user_regs(1);
- if (save_user_regs(regs, mctx, tm_mctx, 1))
- goto badframe;
- }
-
regs->link = tramp;
#ifdef CONFIG_PPC_FPU_REGS
@@ -1066,10 +1065,9 @@ SYSCALL_DEFINE3(swapcontext, struct ucontext __user *, old_ctx,
mctx = (struct mcontext __user *)
((unsigned long) &old_ctx->uc_mcontext & ~0xfUL);
prepare_save_user_regs(ctx_has_vsx_region);
- if (save_user_regs(regs, mctx, NULL, ctx_has_vsx_region))
- return -EFAULT;
if (!user_write_access_begin(old_ctx, ctx_size))
return -EFAULT;
+ unsafe_save_user_regs(regs, mctx, NULL, ctx_has_vsx_region, failed);
unsafe_put_sigset_t(&old_ctx->uc_sigmask, ¤t->blocked, failed);
unsafe_put_user(to_user_ptr(mctx), &old_ctx->uc_regs, failed);
user_write_access_end();
--
2.25.0
^ permalink raw reply related
* [PATCH v2 24/25] powerpc/signal32: Isolate non-copy actions in save_user_regs() and save_tm_user_regs()
From: Christophe Leroy @ 2020-08-18 17:19 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <cover.1597770847.git.christophe.leroy@csgroup.eu>
Reorder actions in save_user_regs() and save_tm_user_regs() to
regroup copies together in order to switch to user_access_begin()
logic in a later patch.
Move non-copy actions into new functions called
prepare_save_user_regs() and prepare_save_tm_user_regs().
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
arch/powerpc/kernel/signal_32.c | 54 +++++++++++++++++++++++++--------
1 file changed, 41 insertions(+), 13 deletions(-)
diff --git a/arch/powerpc/kernel/signal_32.c b/arch/powerpc/kernel/signal_32.c
index 5b8a4ede142c..86539a4e0514 100644
--- a/arch/powerpc/kernel/signal_32.c
+++ b/arch/powerpc/kernel/signal_32.c
@@ -229,14 +229,31 @@ struct rt_sigframe {
* We only save the altivec/spe registers if the process has used
* altivec/spe instructions at some point.
*/
+static void prepare_save_user_regs(int ctx_has_vsx_region)
+{
+ /* Make sure floating point registers are stored in regs */
+ flush_fp_to_thread(current);
+#ifdef CONFIG_ALTIVEC
+ if (current->thread.used_vr)
+ flush_altivec_to_thread(current);
+ if (cpu_has_feature(CPU_FTR_ALTIVEC))
+ current->thread.vrsave = mfspr(SPRN_VRSAVE);
+#endif
+#ifdef CONFIG_VSX
+ if (current->thread.used_vsr && ctx_has_vsx_region)
+ flush_vsx_to_thread(current);
+#endif
+#ifdef CONFIG_SPE
+ if (current->thread.used_spe)
+ flush_spe_to_thread(current);
+#endif
+}
+
static int save_user_regs(struct pt_regs *regs, struct mcontext __user *frame,
struct mcontext __user *tm_frame, int ctx_has_vsx_region)
{
unsigned long msr = regs->msr;
- /* Make sure floating point registers are stored in regs */
- flush_fp_to_thread(current);
-
/* save general registers */
if (save_general_regs(regs, frame))
return 1;
@@ -244,7 +261,6 @@ static int save_user_regs(struct pt_regs *regs, struct mcontext __user *frame,
#ifdef CONFIG_ALTIVEC
/* save altivec registers */
if (current->thread.used_vr) {
- flush_altivec_to_thread(current);
if (__copy_to_user(&frame->mc_vregs, ¤t->thread.vr_state,
ELF_NVRREG * sizeof(vector128)))
return 1;
@@ -260,8 +276,6 @@ static int save_user_regs(struct pt_regs *regs, struct mcontext __user *frame,
* most significant bits of that same vector. --BenH
* Note that the current VRSAVE value is in the SPR at this point.
*/
- if (cpu_has_feature(CPU_FTR_ALTIVEC))
- current->thread.vrsave = mfspr(SPRN_VRSAVE);
if (__put_user(current->thread.vrsave, (u32 __user *)&frame->mc_vregs[32]))
return 1;
#endif /* CONFIG_ALTIVEC */
@@ -281,7 +295,6 @@ static int save_user_regs(struct pt_regs *regs, struct mcontext __user *frame,
* contains valid data
*/
if (current->thread.used_vsr && ctx_has_vsx_region) {
- flush_vsx_to_thread(current);
if (copy_vsx_to_user(&frame->mc_vsregs, current))
return 1;
msr |= MSR_VSX;
@@ -290,7 +303,6 @@ static int save_user_regs(struct pt_regs *regs, struct mcontext __user *frame,
#ifdef CONFIG_SPE
/* save spe registers */
if (current->thread.used_spe) {
- flush_spe_to_thread(current);
if (__copy_to_user(&frame->mc_vregs, current->thread.evr,
ELF_NEVRREG * sizeof(u32)))
return 1;
@@ -326,11 +338,23 @@ static int save_user_regs(struct pt_regs *regs, struct mcontext __user *frame,
*
* See save_user_regs() and signal_64.c:setup_tm_sigcontexts().
*/
-static int save_tm_user_regs(struct pt_regs *regs, struct mcontext __user *frame,
- struct mcontext __user *tm_frame, unsigned long msr)
+static void prepare_save_tm_user_regs(void)
{
WARN_ON(tm_suspend_disabled);
+#ifdef CONFIG_ALTIVEC
+ if (cpu_has_feature(CPU_FTR_ALTIVEC))
+ current->thread.ckvrsave = mfspr(SPRN_VRSAVE);
+#endif
+#ifdef CONFIG_SPE
+ if (current->thread.used_spe)
+ flush_spe_to_thread(current);
+#endif
+}
+
+static int save_tm_user_regs(struct pt_regs *regs, struct mcontext __user *frame,
+ struct mcontext __user *tm_frame, unsigned long msr)
+{
/* Save both sets of general registers */
if (save_general_regs(¤t->thread.ckpt_regs, frame)
|| save_general_regs(regs, tm_frame))
@@ -374,8 +398,6 @@ static int save_tm_user_regs(struct pt_regs *regs, struct mcontext __user *frame
* significant bits of a vector, we "cheat" and stuff VRSAVE in the
* most significant bits of that same vector. --BenH
*/
- if (cpu_has_feature(CPU_FTR_ALTIVEC))
- current->thread.ckvrsave = mfspr(SPRN_VRSAVE);
if (__put_user(current->thread.ckvrsave,
(u32 __user *)&frame->mc_vregs[32]))
return 1;
@@ -427,7 +449,6 @@ static int save_tm_user_regs(struct pt_regs *regs, struct mcontext __user *frame
* simply the same as in save_user_regs().
*/
if (current->thread.used_spe) {
- flush_spe_to_thread(current);
if (__copy_to_user(&frame->mc_vregs, current->thread.evr,
ELF_NEVRREG * sizeof(u32)))
return 1;
@@ -447,6 +468,8 @@ static int save_tm_user_regs(struct pt_regs *regs, struct mcontext __user *frame
return 0;
}
#else
+static void prepare_save_tm_user_regs(void) { }
+
static int save_tm_user_regs(struct pt_regs *regs, struct mcontext __user *frame,
struct mcontext __user *tm_frame, unsigned long msr)
{
@@ -790,9 +813,11 @@ int handle_rt_signal32(struct ksignal *ksig, sigset_t *oldset,
flush_icache_range(tramp, tramp + 2 * sizeof(unsigned long));
if (MSR_TM_ACTIVE(msr)) {
+ prepare_save_tm_user_regs();
if (save_tm_user_regs(regs, mctx, tm_mctx, msr))
goto badframe;
} else {
+ prepare_save_user_regs(1);
if (save_user_regs(regs, mctx, tm_mctx, 1))
goto badframe;
}
@@ -881,9 +906,11 @@ int handle_signal32(struct ksignal *ksig, sigset_t *oldset,
flush_icache_range(tramp, tramp + 2 * sizeof(unsigned long));
if (MSR_TM_ACTIVE(msr)) {
+ prepare_save_tm_user_regs();
if (save_tm_user_regs(regs, mctx, tm_mctx, msr))
goto badframe;
} else {
+ prepare_save_user_regs(1);
if (save_user_regs(regs, mctx, tm_mctx, 1))
goto badframe;
}
@@ -1038,6 +1065,7 @@ SYSCALL_DEFINE3(swapcontext, struct ucontext __user *, old_ctx,
*/
mctx = (struct mcontext __user *)
((unsigned long) &old_ctx->uc_mcontext & ~0xfUL);
+ prepare_save_user_regs(ctx_has_vsx_region);
if (save_user_regs(regs, mctx, NULL, ctx_has_vsx_region))
return -EFAULT;
if (!user_write_access_begin(old_ctx, ctx_size))
--
2.25.0
^ permalink raw reply related
* [PATCH v2 23/25] powerpc/signal: Create 'unsafe' versions of copy_[ck][fpr/vsx]_to_user()
From: Christophe Leroy @ 2020-08-18 17:19 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <cover.1597770847.git.christophe.leroy@csgroup.eu>
For the non VSX version, that's trivial. Just use unsafe_copy_to_user()
instead of __copy_to_user().
For the VSX version, remove the intermediate step through a buffer and
use unsafe_put_user() directly. This generates a far smaller code which
is acceptable to inline, see below:
Standard VSX version:
0000000000000000 <.copy_fpr_to_user>:
0: 7c 08 02 a6 mflr r0
4: fb e1 ff f8 std r31,-8(r1)
8: 39 00 00 20 li r8,32
c: 39 24 0b 80 addi r9,r4,2944
10: 7d 09 03 a6 mtctr r8
14: f8 01 00 10 std r0,16(r1)
18: f8 21 fe 71 stdu r1,-400(r1)
1c: 39 41 00 68 addi r10,r1,104
20: e9 09 00 00 ld r8,0(r9)
24: 39 4a 00 08 addi r10,r10,8
28: 39 29 00 10 addi r9,r9,16
2c: f9 0a 00 00 std r8,0(r10)
30: 42 00 ff f0 bdnz 20 <.copy_fpr_to_user+0x20>
34: e9 24 0d 80 ld r9,3456(r4)
38: 3d 42 00 00 addis r10,r2,0
3a: R_PPC64_TOC16_HA .toc
3c: eb ea 00 00 ld r31,0(r10)
3e: R_PPC64_TOC16_LO_DS .toc
40: f9 21 01 70 std r9,368(r1)
44: e9 3f 00 00 ld r9,0(r31)
48: 81 29 00 20 lwz r9,32(r9)
4c: 2f 89 00 00 cmpwi cr7,r9,0
50: 40 9c 00 18 bge cr7,68 <.copy_fpr_to_user+0x68>
54: 4c 00 01 2c isync
58: 3d 20 40 00 lis r9,16384
5c: 79 29 07 c6 rldicr r9,r9,32,31
60: 7d 3d 03 a6 mtspr 29,r9
64: 4c 00 01 2c isync
68: 38 a0 01 08 li r5,264
6c: 38 81 00 70 addi r4,r1,112
70: 48 00 00 01 bl 70 <.copy_fpr_to_user+0x70>
70: R_PPC64_REL24 .__copy_tofrom_user
74: 60 00 00 00 nop
78: e9 3f 00 00 ld r9,0(r31)
7c: 81 29 00 20 lwz r9,32(r9)
80: 2f 89 00 00 cmpwi cr7,r9,0
84: 40 9c 00 18 bge cr7,9c <.copy_fpr_to_user+0x9c>
88: 4c 00 01 2c isync
8c: 39 20 ff ff li r9,-1
90: 79 29 00 44 rldicr r9,r9,0,1
94: 7d 3d 03 a6 mtspr 29,r9
98: 4c 00 01 2c isync
9c: 38 21 01 90 addi r1,r1,400
a0: e8 01 00 10 ld r0,16(r1)
a4: eb e1 ff f8 ld r31,-8(r1)
a8: 7c 08 03 a6 mtlr r0
ac: 4e 80 00 20 blr
'unsafe' simulated VSX version (The ... are only nops) using
unsafe_copy_fpr_to_user() macro:
unsigned long copy_fpr_to_user(void __user *to,
struct task_struct *task)
{
unsafe_copy_fpr_to_user(to, task, failed);
return 0;
failed:
return 1;
}
0000000000000000 <.copy_fpr_to_user>:
0: 39 00 00 20 li r8,32
4: 39 44 0b 80 addi r10,r4,2944
8: 7d 09 03 a6 mtctr r8
c: 7c 69 1b 78 mr r9,r3
...
20: e9 0a 00 00 ld r8,0(r10)
24: f9 09 00 00 std r8,0(r9)
28: 39 4a 00 10 addi r10,r10,16
2c: 39 29 00 08 addi r9,r9,8
30: 42 00 ff f0 bdnz 20 <.copy_fpr_to_user+0x20>
34: e9 24 0d 80 ld r9,3456(r4)
38: f9 23 01 00 std r9,256(r3)
3c: 38 60 00 00 li r3,0
40: 4e 80 00 20 blr
...
50: 38 60 00 01 li r3,1
54: 4e 80 00 20 blr
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
arch/powerpc/kernel/signal.h | 53 ++++++++++++++++++++++++++++++++++++
1 file changed, 53 insertions(+)
diff --git a/arch/powerpc/kernel/signal.h b/arch/powerpc/kernel/signal.h
index f610cfafa478..2559a681536e 100644
--- a/arch/powerpc/kernel/signal.h
+++ b/arch/powerpc/kernel/signal.h
@@ -32,7 +32,54 @@ unsigned long copy_fpr_to_user(void __user *to, struct task_struct *task);
unsigned long copy_ckfpr_to_user(void __user *to, struct task_struct *task);
unsigned long copy_fpr_from_user(struct task_struct *task, void __user *from);
unsigned long copy_ckfpr_from_user(struct task_struct *task, void __user *from);
+
+#define unsafe_copy_fpr_to_user(to, task, label) do { \
+ struct task_struct *__t = task; \
+ u64 __user *buf = (u64 __user *)to; \
+ int i; \
+ \
+ for (i = 0; i < ELF_NFPREG - 1 ; i++) \
+ unsafe_put_user(__t->thread.TS_FPR(i), &buf[i], label); \
+ unsafe_put_user(__t->thread.fp_state.fpscr, &buf[i], label); \
+} while (0)
+
+#define unsafe_copy_vsx_to_user(to, task, label) do { \
+ struct task_struct *__t = task; \
+ u64 __user *buf = (u64 __user *)to; \
+ int i; \
+ \
+ for (i = 0; i < ELF_NVSRHALFREG ; i++) \
+ unsafe_put_user(__t->thread.fp_state.fpr[i][TS_VSRLOWOFFSET], \
+ &buf[i], label);\
+} while (0)
+
+#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
+#define unsafe_copy_ckfpr_to_user(to, task, label) do { \
+ struct task_struct *__t = task; \
+ u64 __user *buf = (u64 __user *)to; \
+ int i; \
+ \
+ for (i = 0; i < ELF_NFPREG - 1 ; i++) \
+ unsafe_put_user(__t->thread.TS_CKFPR(i), &buf[i], label);\
+ unsafe_put_user(__t->thread.ckfp_state.fpscr, &buf[i], label); \
+} while (0)
+
+#define unsafe_copy_ckvsx_to_user(to, task, label) do { \
+ struct task_struct *__t = task; \
+ u64 __user *buf = (u64 __user *)to; \
+ int i; \
+ \
+ for (i = 0; i < ELF_NVSRHALFREG ; i++) \
+ unsafe_put_user(__t->thread.ckfp_state.fpr[i][TS_VSRLOWOFFSET], \
+ &buf[i], label);\
+} while (0)
+#endif
#elif defined(CONFIG_PPC_FPU_REGS)
+
+#define unsafe_copy_fpr_to_user(to, task, label) \
+ unsafe_copy_to_user(to, (task)->thread.fp_state.fpr, \
+ ELF_NFPREG * sizeof(double), label)
+
static inline unsigned long
copy_fpr_to_user(void __user *to, struct task_struct *task)
{
@@ -48,6 +95,10 @@ copy_fpr_from_user(struct task_struct *task, void __user *from)
}
#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
+#define unsafe_copy_ckfpr_to_user(to, task, label) \
+ unsafe_copy_to_user(to, (task)->thread.ckfp_state.fpr, \
+ ELF_NFPREG * sizeof(double), label)
+
inline unsigned long copy_ckfpr_to_user(void __user *to, struct task_struct *task)
{
return __copy_to_user(to, task->thread.ckfp_state.fpr,
@@ -62,6 +113,8 @@ copy_ckfpr_from_user(struct task_struct *task, void __user *from)
}
#endif /* CONFIG_PPC_TRANSACTIONAL_MEM */
#else
+#define unsafe_copy_fpr_to_user(to, task, label) do { } while (0)
+
static inline unsigned long
copy_fpr_to_user(void __user *to, struct task_struct *task)
{
--
2.25.0
^ permalink raw reply related
* [PATCH v2 22/25] powerpc/signal32: Switch swap_context() to user_access_begin() logic
From: Christophe Leroy @ 2020-08-18 17:19 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <cover.1597770847.git.christophe.leroy@csgroup.eu>
As this was the last user of put_sigset_t(), remove it as well.
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
arch/powerpc/kernel/signal_32.c | 24 ++++++++++--------------
1 file changed, 10 insertions(+), 14 deletions(-)
diff --git a/arch/powerpc/kernel/signal_32.c b/arch/powerpc/kernel/signal_32.c
index 3f9f315dd036..5b8a4ede142c 100644
--- a/arch/powerpc/kernel/signal_32.c
+++ b/arch/powerpc/kernel/signal_32.c
@@ -82,11 +82,6 @@
* Functions for flipping sigsets (thanks to brain dead generic
* implementation that makes things simple for little endian only)
*/
-static inline int put_sigset_t(compat_sigset_t __user *uset, sigset_t *set)
-{
- return put_compat_sigset(uset, set, sizeof(*uset));
-}
-
#define unsafe_put_sigset_t unsafe_put_compat_sigset
static inline int get_sigset_t(sigset_t *set,
@@ -138,11 +133,6 @@ static inline int restore_general_regs(struct pt_regs *regs,
#define GP_REGS_SIZE min(sizeof(elf_gregset_t), sizeof(struct pt_regs))
-static inline int put_sigset_t(sigset_t __user *uset, sigset_t *set)
-{
- return copy_to_user(uset, set, sizeof(*uset));
-}
-
#define unsafe_put_sigset_t(uset, set, label) do { \
sigset_t __user *__us = uset ; \
const sigset_t *__s = set; \
@@ -1048,11 +1038,13 @@ SYSCALL_DEFINE3(swapcontext, struct ucontext __user *, old_ctx,
*/
mctx = (struct mcontext __user *)
((unsigned long) &old_ctx->uc_mcontext & ~0xfUL);
- if (!access_ok(old_ctx, ctx_size)
- || save_user_regs(regs, mctx, NULL, ctx_has_vsx_region)
- || put_sigset_t(&old_ctx->uc_sigmask, ¤t->blocked)
- || __put_user(to_user_ptr(mctx), &old_ctx->uc_regs))
+ if (save_user_regs(regs, mctx, NULL, ctx_has_vsx_region))
+ return -EFAULT;
+ if (!user_write_access_begin(old_ctx, ctx_size))
return -EFAULT;
+ unsafe_put_sigset_t(&old_ctx->uc_sigmask, ¤t->blocked, failed);
+ unsafe_put_user(to_user_ptr(mctx), &old_ctx->uc_regs, failed);
+ user_write_access_end();
}
if (new_ctx == NULL)
return 0;
@@ -1076,6 +1068,10 @@ SYSCALL_DEFINE3(swapcontext, struct ucontext __user *, old_ctx,
set_thread_flag(TIF_RESTOREALL);
return 0;
+
+failed:
+ user_write_access_end();
+ return -EFAULT;
}
#ifdef CONFIG_PPC64
--
2.25.0
^ permalink raw reply related
* [PATCH v2 21/25] powerpc/signal32: Add and use unsafe_put_sigset_t()
From: Christophe Leroy @ 2020-08-18 17:19 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <cover.1597770847.git.christophe.leroy@csgroup.eu>
put_sigset_t() calls copy_to_user() for copying two words.
This is terribly inefficient for copying two words.
By switching to unsafe_put_user(), we end up with something as
simple as:
3cc: 81 3d 00 00 lwz r9,0(r29)
3d0: 91 26 00 b4 stw r9,180(r6)
3d4: 81 3d 00 04 lwz r9,4(r29)
3d8: 91 26 00 b8 stw r9,184(r6)
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
arch/powerpc/kernel/signal_32.c | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/kernel/signal_32.c b/arch/powerpc/kernel/signal_32.c
index 310d3b8d9ad5..3f9f315dd036 100644
--- a/arch/powerpc/kernel/signal_32.c
+++ b/arch/powerpc/kernel/signal_32.c
@@ -87,6 +87,8 @@ static inline int put_sigset_t(compat_sigset_t __user *uset, sigset_t *set)
return put_compat_sigset(uset, set, sizeof(*uset));
}
+#define unsafe_put_sigset_t unsafe_put_compat_sigset
+
static inline int get_sigset_t(sigset_t *set,
const compat_sigset_t __user *uset)
{
@@ -141,6 +143,13 @@ static inline int put_sigset_t(sigset_t __user *uset, sigset_t *set)
return copy_to_user(uset, set, sizeof(*uset));
}
+#define unsafe_put_sigset_t(uset, set, label) do { \
+ sigset_t __user *__us = uset ; \
+ const sigset_t *__s = set; \
+ \
+ unsafe_copy_to_user(__us, __s, sizeof(*__us), label); \
+} while (0)
+
static inline int get_sigset_t(sigset_t *set, const sigset_t __user *uset)
{
return copy_from_user(set, uset, sizeof(*uset));
@@ -780,10 +789,10 @@ int handle_rt_signal32(struct ksignal *ksig, sigset_t *oldset,
failed);
unsafe_put_user(PPC_INST_SC, &mctx->mc_pad[1], failed);
}
+ unsafe_put_sigset_t(&frame->uc.uc_sigmask, oldset, failed);
+
user_write_access_end();
- if (put_sigset_t(&frame->uc.uc_sigmask, oldset))
- goto badframe;
if (copy_siginfo_to_user(&frame->info, &ksig->info))
goto badframe;
--
2.25.0
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox