* [PATCH] libnvdimm/of_pmem: Fix leaking bus_desc.provider_name in some paths
From: Vaibhav Jain @ 2020-01-23 3:18 UTC (permalink / raw)
To: linuxppc-dev, linux-nvdimm
Cc: Vishal Verma, stable, Oliver O'Halloran, Aneesh Kumar K . V,
Vaibhav Jain, Dan Williams
String 'bus_desc.provider_name' allocated inside
of_pmem_region_probe() will leak in case call to nvdimm_bus_register()
fails or when of_pmem_region_remove() is called.
This minor patch ensures that 'bus_desc.provider_name' is freed in
error path for of_pmem_region_probe() as well as in
of_pmem_region_remove().
Cc: stable@vger.kernel.org
Fixes:49bddc73d15c2("libnvdimm/of_pmem: Provide a unique name for bus provider")
Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>
---
drivers/nvdimm/of_pmem.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/nvdimm/of_pmem.c b/drivers/nvdimm/of_pmem.c
index 8224d1431ea9..9cb76f9837ad 100644
--- a/drivers/nvdimm/of_pmem.c
+++ b/drivers/nvdimm/of_pmem.c
@@ -36,6 +36,7 @@ static int of_pmem_region_probe(struct platform_device *pdev)
priv->bus = bus = nvdimm_bus_register(&pdev->dev, &priv->bus_desc);
if (!bus) {
+ kfree(priv->bus_desc.provider_name);
kfree(priv);
return -ENODEV;
}
@@ -81,6 +82,7 @@ static int of_pmem_region_remove(struct platform_device *pdev)
struct of_pmem_private *priv = platform_get_drvdata(pdev);
nvdimm_bus_unregister(priv->bus);
+ kfree(priv->bus_desc.provider_name);
kfree(priv);
return 0;
--
2.24.1
^ permalink raw reply related
* Re: [PATCH v2 1/6] fs/readdir: Fix filldir() and filldir64() use of user_access_begin()
From: Christophe Leroy @ 2020-01-23 6:27 UTC (permalink / raw)
To: Linus Torvalds
Cc: Linux Kernel Mailing List, Linux-MM, Paul Mackerras,
Alexander Viro, linux-fsdevel, Andrew Morton, linuxppc-dev
In-Reply-To: <CAHk-=whzabmci2b7ras3RcMpimvzNAk4QHDcYf=irvwXnunS8w@mail.gmail.com>
Le 22/01/2020 à 21:37, Linus Torvalds a écrit :
> [ Talking to myself ]
>
> On Wed, Jan 22, 2020 at 12:00 PM Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
>>
>> COMPLETELY UNTESTED! It compiles for me. The generated assembly looks
>> ok from a quick look.
>
>
> So here's a slightly updated patch that does exactly that, and avoids
> the objtool warning.
>
> It actually generates better code than the last one too, because now
> we don't duplicate the user_access_end() for the EINTR case.
>
> So test this one instead, please.
This patch works on my ppc board, thanks
Christophe
^ permalink raw reply
* [PATCH] lib: Reduce user_access_begin() boundaries in strncpy_from_user() and strnlen_user()
From: Christophe Leroy @ 2020-01-23 8:34 UTC (permalink / raw)
To: Linus Torvalds, Andrew Morton; +Cc: linux-mm, linuxppc-dev, linux-kernel
The range passed to user_access_begin() by strncpy_from_user() and
strnlen_user() starts at 'src' and goes up to the limit of userspace
allthough reads will be limited by the 'count' param.
On 32 bits powerpc (book3s/32) access has to be granted for each 256Mbytes
segment and the cost increases with the number of segments to unlock.
Limit the range with 'count' param.
Fixes: 594cc251fdd0 ("make 'user_access_begin()' do 'access_ok()'")
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
lib/strncpy_from_user.c | 14 +++++++-------
lib/strnlen_user.c | 14 +++++++-------
2 files changed, 14 insertions(+), 14 deletions(-)
diff --git a/lib/strncpy_from_user.c b/lib/strncpy_from_user.c
index dccb95af6003..706020b06617 100644
--- a/lib/strncpy_from_user.c
+++ b/lib/strncpy_from_user.c
@@ -30,13 +30,6 @@ static inline long do_strncpy_from_user(char *dst, const char __user *src,
const struct word_at_a_time constants = WORD_AT_A_TIME_CONSTANTS;
unsigned long res = 0;
- /*
- * Truncate 'max' to the user-specified limit, so that
- * we only have one limit we need to check in the loop
- */
- if (max > count)
- max = count;
-
if (IS_UNALIGNED(src, dst))
goto byte_at_a_time;
@@ -114,6 +107,13 @@ long strncpy_from_user(char *dst, const char __user *src, long count)
unsigned long max = max_addr - src_addr;
long retval;
+ /*
+ * Truncate 'max' to the user-specified limit, so that
+ * we only have one limit we need to check in the loop
+ */
+ if (max > count)
+ max = count;
+
kasan_check_write(dst, count);
check_object_size(dst, count, false);
if (user_access_begin(src, max)) {
diff --git a/lib/strnlen_user.c b/lib/strnlen_user.c
index 6c0005d5dd5c..41670d4a5816 100644
--- a/lib/strnlen_user.c
+++ b/lib/strnlen_user.c
@@ -26,13 +26,6 @@ static inline long do_strnlen_user(const char __user *src, unsigned long count,
unsigned long align, res = 0;
unsigned long c;
- /*
- * Truncate 'max' to the user-specified limit, so that
- * we only have one limit we need to check in the loop
- */
- if (max > count)
- max = count;
-
/*
* Do everything aligned. But that means that we
* need to also expand the maximum..
@@ -109,6 +102,13 @@ long strnlen_user(const char __user *str, long count)
unsigned long max = max_addr - src_addr;
long retval;
+ /*
+ * Truncate 'max' to the user-specified limit, so that
+ * we only have one limit we need to check in the loop
+ */
+ if (max > count)
+ max = count;
+
if (user_access_begin(str, max)) {
retval = do_strnlen_user(str, count, max);
user_access_end();
--
2.25.0
^ permalink raw reply related
* Re: [PATCH kernel RFC 0/4] powerpc/powenv/ioda: Allow huge DMA window at 4GB
From: Alexey Kardashevskiy @ 2020-01-23 8:42 UTC (permalink / raw)
To: David Gibson
Cc: Alistair Popple, Alex Williamson, Oliver O'Halloran,
linuxppc-dev, kvm
In-Reply-To: <20200123011730.GL2347@umbus.fritz.box>
On 23/01/2020 12:17, David Gibson wrote:
> On Thu, Jan 23, 2020 at 11:53:32AM +1100, Alexey Kardashevskiy wrote:
>> Anyone, ping?
>
> Sorry, I've totally lost track of this one. I think you'll need to
> repost.
It has not changed and still applies, and the question is more about how
we proceed with this feature that the patches themselves. Or it is just
not in your mailbox anymore so you cannot reply? :)
>
>
>>
>>
>> On 10/01/2020 15:18, Alexey Kardashevskiy wrote:
>>>
>>>
>>> On 02/12/2019 12:59, Alexey Kardashevskiy wrote:
>>>> Here is an attempt to support bigger DMA space for devices
>>>> supporting DMA masks less than 59 bits (GPUs come into mind
>>>> first). POWER9 PHBs have an option to map 2 windows at 0
>>>> and select a windows based on DMA address being below or above
>>>> 4GB.
>>>>
>>>> This adds the "iommu=iommu_bypass" kernel parameter and
>>>> supports VFIO+pseries machine - current this requires telling
>>>> upstream+unmodified QEMU about this via
>>>> -global spapr-pci-host-bridge.dma64_win_addr=0x100000000
>>>> or per-phb property. 4/4 advertises the new option but
>>>> there is no automation around it in QEMU (should it be?).
>>>>
>>>> For now it is either 1<<59 or 4GB mode; dynamic switching is
>>>> not supported (could be via sysfs).
>>>>
>>>> This is based on sha1
>>>> a6ed68d6468b Linus Torvalds "Merge tag 'drm-next-2019-11-27' of git://anongit.freedesktop.org/drm/drm".
>>>>
>>>> Please comment. Thanks.
>>>
>>>
>>> David, Alistair, ping? Thanks,
>>
>>
>>>
>>>
>>>>
>>>>
>>>>
>>>> Alexey Kardashevskiy (4):
>>>> powerpc/powernv/ioda: Rework for huge DMA window at 4GB
>>>> powerpc/powernv/ioda: Allow smaller TCE table levels
>>>> powerpc/powernv/phb4: Add 4GB IOMMU bypass mode
>>>> vfio/spapr_tce: Advertise and allow a huge DMA windows at 4GB
>>>>
>>>> arch/powerpc/include/asm/iommu.h | 1 +
>>>> arch/powerpc/include/asm/opal-api.h | 11 +-
>>>> arch/powerpc/include/asm/opal.h | 2 +
>>>> arch/powerpc/platforms/powernv/pci.h | 1 +
>>>> include/uapi/linux/vfio.h | 2 +
>>>> arch/powerpc/platforms/powernv/opal-call.c | 2 +
>>>> arch/powerpc/platforms/powernv/pci-ioda-tce.c | 4 +-
>>>> arch/powerpc/platforms/powernv/pci-ioda.c | 219 ++++++++++++++----
>>>> drivers/vfio/vfio_iommu_spapr_tce.c | 10 +-
>>>> 9 files changed, 202 insertions(+), 50 deletions(-)
>>>>
>>>
>>
>
--
Alexey
^ permalink raw reply
* Re: [PATCH v2 net-next] net: convert suitable drivers to use phy_do_ioctl_running
From: David Miller @ 2020-01-23 9:49 UTC (permalink / raw)
To: hkallweit1
Cc: andrew, f.fainelli, michal.simek, wens, bcm-kernel-feedback-list,
slemieux.tyco, yisen.zhuang, steve.glendinning, mripard, vz,
opendmb, linux-arm-kernel, salil.mehta, timur, sergei.shtylyov,
netdev, linux-usb, woojung.huh, UNGLinuxDriver, linux-renesas-soc,
linuxppc-dev
In-Reply-To: <2db5d899-a550-456d-a725-f7cf009f53a3@gmail.com>
From: Heiner Kallweit <hkallweit1@gmail.com>
Date: Tue, 21 Jan 2020 22:09:33 +0100
> Convert suitable drivers to use new helper phy_do_ioctl_running.
>
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
> v2: I forgot the netdev mailing list
Applied to net-next.
^ permalink raw reply
* [PATCH v2] powerpc/mm/hash: Fix sharing context ids between kernel & userspace
From: Michael Ellerman @ 2020-01-23 10:25 UTC (permalink / raw)
To: linuxppc-dev; +Cc: aneesh.kumar, marillat, debian-powerpc, romain, npiggin
From: "Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com>
Commit 0034d395f89d ("powerpc/mm/hash64: Map all the kernel regions in
the same 0xc range") has a bug in the definition of MIN_USER_CONTEXT.
The result is that the context id used for the vmemmap and the lowest
context id handed out to userspace are the same. The context id is
essentially the process identifier as far as the first stage of the
MMU translation is concerned.
This can result in multiple SLB entries with the same VSID (Virtual
Segment ID), accessible to the kernel and some random userspace
process that happens to get the overlapping id, which is not expected
eg:
07 c00c000008000000 40066bdea7000500 1T ESID= c00c00 VSID= 66bdea7 LLP:100
12 0002000008000000 40066bdea7000d80 1T ESID= 200 VSID= 66bdea7 LLP:100
Even though the user process and the kernel use the same VSID, the
permissions in the hash page table prevent the user process from
reading or writing to any kernel mappings.
It can also lead to SLB entries with different base page size
encodings (LLP), eg:
05 c00c000008000000 00006bde0053b500 256M ESID=c00c00000 VSID= 6bde0053b LLP:100
09 0000000008000000 00006bde0053bc80 256M ESID= 0 VSID= 6bde0053b LLP: 0
Such SLB entries can result in machine checks, eg. as seen on a G5:
Oops: Machine check, sig: 7 [#1]
BE PAGE SIZE=64K MU-Hash SMP NR_CPUS=4 NUMA Power Mac
NIP: c00000000026f248 LR: c000000000295e58 CTR: 0000000000000000
REGS: c0000000erfd3d70 TRAP: 0200 Tainted: G M (5.5.0-rcl-gcc-8.2.0-00010-g228b667d8ea1)
MSR: 9000000000109032 <SF,HV,EE,ME,IR,DR,RI> CR: 24282048 XER: 00000000
DAR: c00c000000612c80 DSISR: 00000400 IRQMASK: 0
...
NIP [c00000000026f248] .kmem_cache_free+0x58/0x140
LR [c088000008295e58] .putname 8x88/0xa
Call Trace:
.putname+0xB8/0xa
.filename_lookup.part.76+0xbe/0x160
.do_faccessat+0xe0/0x380
system_call+0x5c/ex68
This happens with 256MB segments and 64K pages, as the duplicate VSID
is hit with the first vmemmap segment and the first user segment, and
older 32-bit userspace maps things in the first user segment.
On other CPUs a machine check is not seen. Instead the userspace
process can get stuck continuously faulting, with the fault never
properly serviced, due to the kernel not understanding that there is
already a HPTE for the address but with inaccessible permissions.
On machines with 1T segments we've not seen the bug hit other than by
deliberately exercising it. That seems to be just a matter of luck
though, due to the typical layout of the user virtual address space
and the ranges of vmemmap that are typically populated.
To fix it we add 2 to MIN_USER_CONTEXT. This ensures the lowest
context given to userspace doesn't overlap with the VMEMMAP context,
or with the context for INVALID_REGION_ID.
Fixes: 0034d395f89d ("powerpc/mm/hash64: Map all the kernel regions in the same 0xc range")
Cc: stable@vger.kernel.org # v5.2+
Reported-by: Christian Marillat <marillat@debian.org>
Reported-by: Romain Dolbeau <romain@dolbeau.org>
Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
[mpe: Account for INVALID_REGION_ID, mostly rewrite change log]
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
arch/powerpc/include/asm/book3s/64/mmu-hash.h | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/arch/powerpc/include/asm/book3s/64/mmu-hash.h b/arch/powerpc/include/asm/book3s/64/mmu-hash.h
index 15b75005bc34..3fa1b962dc27 100644
--- a/arch/powerpc/include/asm/book3s/64/mmu-hash.h
+++ b/arch/powerpc/include/asm/book3s/64/mmu-hash.h
@@ -600,8 +600,11 @@ extern void slb_set_size(u16 size);
*
*/
#define MAX_USER_CONTEXT ((ASM_CONST(1) << CONTEXT_BITS) - 2)
+
+// The + 2 accounts for INVALID_REGION and 1 more to avoid overlap with kernel
#define MIN_USER_CONTEXT (MAX_KERNEL_CTX_CNT + MAX_VMALLOC_CTX_CNT + \
- MAX_IO_CTX_CNT + MAX_VMEMMAP_CTX_CNT)
+ MAX_IO_CTX_CNT + MAX_VMEMMAP_CTX_CNT + 2)
+
/*
* For platforms that support on 65bit VA we limit the context bits
*/
--
2.21.1
^ permalink raw reply related
* Re: [PATCH v2 5/6] powerpc/32s: prepare prevent_user_access() for user_access_end()
From: Michael Ellerman @ 2020-01-23 10:59 UTC (permalink / raw)
To: Christophe Leroy, Benjamin Herrenschmidt, Paul Mackerras
Cc: linux-fsdevel, linux-mm, linuxppc-dev, linux-kernel
In-Reply-To: <824b69f5452d1d41d12c4dbd306d4b8f32d493fc.1579715466.git.christophe.leroy@c-s.fr>
Christophe Leroy <christophe.leroy@c-s.fr> writes:
> In preparation of implementing user_access_begin and friends
> on powerpc, the book3s/32 version of prevent_user_access() need
> to be prepared for user_access_end().
>
> user_access_end() doesn't provide the address and size which
> were passed to user_access_begin(), required by prevent_user_access()
> to know which segment to modify.
>
> The list of segments which where unprotected by allow_user_access()
> are available in current->kuap. But we don't want prevent_user_access()
> to read this all the time, especially everytime it is 0 (for instance
> because the access was not a write access).
>
> Implement a special direction case named KUAP_SELF. In this case only,
> the addr and end are retrieved from current->kuap.
Can we call it KUAP_CURRENT?
ie. "use the KUAP state in current"
cheers
^ permalink raw reply
* [PATCH] powerpc/fsl_booke: avoid creating duplicate tlb1 entry
From: Laurentiu Tudor @ 2020-01-23 11:19 UTC (permalink / raw)
To: oss@buserror.net, linuxppc-dev@lists.ozlabs.org,
mpe@ellerman.id.au
Cc: Diana Madalina Craciun, linux-kernel@vger.kernel.org,
stable@vger.kernel.org, Laurentiu Tudor
In the current implementation, the call to loadcam_multi() is wrapped
between switch_to_as1() and restore_to_as0() calls so, when it tries
to create its own temporary AS=1 TLB1 entry, it ends up duplicating the
existing one created by switch_to_as1(). Add a check to skip creating
the temporary entry if already running in AS=1.
Fixes: d9e1831a4202 ("powerpc/85xx: Load all early TLB entries at once")
Signed-off-by: Laurentiu Tudor <laurentiu.tudor@nxp.com>
Cc: stable@vger.kernel.org
---
arch/powerpc/mm/nohash/tlb_low.S | 12 +++++++++++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/arch/powerpc/mm/nohash/tlb_low.S b/arch/powerpc/mm/nohash/tlb_low.S
index 2ca407cedbe7..eaeee402f96e 100644
--- a/arch/powerpc/mm/nohash/tlb_low.S
+++ b/arch/powerpc/mm/nohash/tlb_low.S
@@ -397,7 +397,7 @@ _GLOBAL(set_context)
* extern void loadcam_entry(unsigned int index)
*
* Load TLBCAM[index] entry in to the L2 CAM MMU
- * Must preserve r7, r8, r9, and r10
+ * Must preserve r7, r8, r9, r10 and r11
*/
_GLOBAL(loadcam_entry)
mflr r5
@@ -433,6 +433,10 @@ END_MMU_FTR_SECTION_IFSET(MMU_FTR_BIG_PHYS)
*/
_GLOBAL(loadcam_multi)
mflr r8
+ /* Don't switch to AS=1 if already there */
+ mfmsr r11
+ andi. r11,r11,MSR_IS
+ bne 10f
/*
* Set up temporary TLB entry that is the same as what we're
@@ -458,6 +462,7 @@ _GLOBAL(loadcam_multi)
mtmsr r6
isync
+10:
mr r9,r3
add r10,r3,r4
2: bl loadcam_entry
@@ -466,6 +471,10 @@ _GLOBAL(loadcam_multi)
mr r3,r9
blt 2b
+ /* Don't return to AS=0 if we were in AS=1 at function start */
+ andi. r11,r11,MSR_IS
+ bne 3f
+
/* Return to AS=0 and clear the temporary entry */
mfmsr r6
rlwinm. r6,r6,0,~(MSR_IS|MSR_DS)
@@ -481,6 +490,7 @@ _GLOBAL(loadcam_multi)
tlbwe
isync
+3:
mtlr r8
blr
#endif
--
2.17.1
^ permalink raw reply related
* Re: [PATCH v2 1/6] fs/readdir: Fix filldir() and filldir64() use of user_access_begin()
From: Michael Ellerman @ 2020-01-23 11:56 UTC (permalink / raw)
To: Christophe Leroy, Benjamin Herrenschmidt, Paul Mackerras,
Linus Torvalds, Alexander Viro, Andrew Morton
Cc: linux-fsdevel, linux-mm, linuxppc-dev, linux-kernel
In-Reply-To: <12a4be679e43de1eca6e5e2173163f27e2f25236.1579715466.git.christophe.leroy@c-s.fr>
Hi Christophe,
This patch is independent of the rest of the series AFAICS, and it looks
like Linus has modified it quite a bit down thread.
So I'll take patches 2-6 via powerpc and assume this patch will go via
Linus or Al or elsewhere.
Also a couple of minor spelling fixes below.
cheers
Christophe Leroy <christophe.leroy@c-s.fr> writes:
> Some architectures grand full access to userspace regardless of the
^
grant
> address/len passed to user_access_begin(), but other architectures
> only grand access to the requested area.
^
grant
>
> For exemple, on 32 bits powerpc (book3s/32), access is granted by
^
example
> segments of 256 Mbytes.
>
> Modify filldir() and filldir64() to request the real area they need
> to get access to, i.e. the area covering the parent dirent (if any)
> and the contiguous current dirent.
>
> Fixes: 9f79b78ef744 ("Convert filldir[64]() from __put_user() to unsafe_put_user()")
> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
> ---
> v2: have user_access_begin() cover both parent dirent (if any) and current dirent
> ---
> fs/readdir.c | 50 ++++++++++++++++++++++++++++----------------------
> 1 file changed, 28 insertions(+), 22 deletions(-)
>
> diff --git a/fs/readdir.c b/fs/readdir.c
> index d26d5ea4de7b..3f9b4488d9b7 100644
> --- a/fs/readdir.c
> +++ b/fs/readdir.c
> @@ -214,7 +214,7 @@ struct getdents_callback {
> static int filldir(struct dir_context *ctx, const char *name, int namlen,
> loff_t offset, u64 ino, unsigned int d_type)
> {
> - struct linux_dirent __user * dirent;
> + struct linux_dirent __user * dirent, *dirent0;
> struct getdents_callback *buf =
> container_of(ctx, struct getdents_callback, ctx);
> unsigned long d_ino;
> @@ -232,19 +232,22 @@ static int filldir(struct dir_context *ctx, const char *name, int namlen,
> buf->error = -EOVERFLOW;
> return -EOVERFLOW;
> }
> - dirent = buf->previous;
> - if (dirent && signal_pending(current))
> + dirent0 = buf->previous;
> + if (dirent0 && signal_pending(current))
> return -EINTR;
>
> - /*
> - * Note! This range-checks 'previous' (which may be NULL).
> - * The real range was checked in getdents
> - */
> - if (!user_access_begin(dirent, sizeof(*dirent)))
> - goto efault;
> - if (dirent)
> - unsafe_put_user(offset, &dirent->d_off, efault_end);
> dirent = buf->current_dir;
> + if (dirent0) {
> + int sz = (void __user *)dirent + reclen -
> + (void __user *)dirent0;
> +
> + if (!user_access_begin(dirent0, sz))
> + goto efault;
> + unsafe_put_user(offset, &dirent0->d_off, efault_end);
> + } else {
> + if (!user_access_begin(dirent, reclen))
> + goto efault;
> + }
> unsafe_put_user(d_ino, &dirent->d_ino, efault_end);
> unsafe_put_user(reclen, &dirent->d_reclen, efault_end);
> unsafe_put_user(d_type, (char __user *) dirent + reclen - 1, efault_end);
> @@ -307,7 +310,7 @@ struct getdents_callback64 {
> static int filldir64(struct dir_context *ctx, const char *name, int namlen,
> loff_t offset, u64 ino, unsigned int d_type)
> {
> - struct linux_dirent64 __user *dirent;
> + struct linux_dirent64 __user *dirent, *dirent0;
> struct getdents_callback64 *buf =
> container_of(ctx, struct getdents_callback64, ctx);
> int reclen = ALIGN(offsetof(struct linux_dirent64, d_name) + namlen + 1,
> @@ -319,19 +322,22 @@ static int filldir64(struct dir_context *ctx, const char *name, int namlen,
> buf->error = -EINVAL; /* only used if we fail.. */
> if (reclen > buf->count)
> return -EINVAL;
> - dirent = buf->previous;
> - if (dirent && signal_pending(current))
> + dirent0 = buf->previous;
> + if (dirent0 && signal_pending(current))
> return -EINTR;
>
> - /*
> - * Note! This range-checks 'previous' (which may be NULL).
> - * The real range was checked in getdents
> - */
> - if (!user_access_begin(dirent, sizeof(*dirent)))
> - goto efault;
> - if (dirent)
> - unsafe_put_user(offset, &dirent->d_off, efault_end);
> dirent = buf->current_dir;
> + if (dirent0) {
> + int sz = (void __user *)dirent + reclen -
> + (void __user *)dirent0;
> +
> + if (!user_access_begin(dirent0, sz))
> + goto efault;
> + unsafe_put_user(offset, &dirent0->d_off, efault_end);
> + } else {
> + if (!user_access_begin(dirent, reclen))
> + goto efault;
> + }
> unsafe_put_user(ino, &dirent->d_ino, efault_end);
> unsafe_put_user(reclen, &dirent->d_reclen, efault_end);
> unsafe_put_user(d_type, &dirent->d_type, efault_end);
> --
> 2.25.0
^ permalink raw reply
* Re: [PATCH v2 1/6] fs/readdir: Fix filldir() and filldir64() use of user_access_begin()
From: Michael Ellerman @ 2020-01-23 12:00 UTC (permalink / raw)
To: Christophe Leroy, Benjamin Herrenschmidt, Paul Mackerras,
Linus Torvalds, Alexander Viro, Andrew Morton
Cc: linux-fsdevel, linux-mm, linuxppc-dev, linux-kernel
In-Reply-To: <87muaeidyc.fsf@mpe.ellerman.id.au>
Michael Ellerman <mpe@ellerman.id.au> writes:
> Hi Christophe,
>
> This patch is independent of the rest of the series AFAICS
And of course having hit send I immediately realise that's not true.
> So I'll take patches 2-6 via powerpc and assume this patch will go via
> Linus or Al or elsewhere.
So I guess I'll wait and see what happens with patch 1.
cheers
^ permalink raw reply
* Re: [PATCH v2 6/6] powerpc: Implement user_access_begin and friends
From: Michael Ellerman @ 2020-01-23 12:05 UTC (permalink / raw)
To: Christophe Leroy, Benjamin Herrenschmidt, Paul Mackerras
Cc: linux-fsdevel, linux-mm, linuxppc-dev, linux-kernel
In-Reply-To: <2a20d19776faba4d85dbe51ae00a5f6ac5ac0969.1579715466.git.christophe.leroy@c-s.fr>
Christophe Leroy <christophe.leroy@c-s.fr> writes:
> Today, when a function like strncpy_from_user() is called,
> the userspace access protection is de-activated and re-activated
> for every word read.
>
> By implementing user_access_begin and friends, the protection
> is de-activated at the beginning of the copy and re-activated at the
> end.
>
> Implement user_access_begin(), user_access_end() and
> unsafe_get_user(), unsafe_put_user() and unsafe_copy_to_user()
>
> For the time being, we keep user_access_save() and
> user_access_restore() as nops.
That means we will run with user access enabled in a few more places, but
it's only used sparingly AFAICS:
kernel/trace/trace_branch.c: unsigned long flags = user_access_save();
lib/ubsan.c: unsigned long flags = user_access_save();
lib/ubsan.c: unsigned long ua_flags = user_access_save();
mm/kasan/common.c: unsigned long flags = user_access_save();
And we don't have objtool checking that user access enablement isn't
leaking in the first place, so I guess it's OK for us not to implement
these to begin with?
cheers
> diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
> index cafad1960e76..ea67bbd56bd4 100644
> --- a/arch/powerpc/include/asm/uaccess.h
> +++ b/arch/powerpc/include/asm/uaccess.h
> @@ -91,9 +91,14 @@ static inline int __access_ok(unsigned long addr, unsigned long size,
> __put_user_check((__typeof__(*(ptr)))(x), (ptr), sizeof(*(ptr)))
>
> #define __get_user(x, ptr) \
> - __get_user_nocheck((x), (ptr), sizeof(*(ptr)))
> + __get_user_nocheck((x), (ptr), sizeof(*(ptr)), true)
> #define __put_user(x, ptr) \
> - __put_user_nocheck((__typeof__(*(ptr)))(x), (ptr), sizeof(*(ptr)))
> + __put_user_nocheck((__typeof__(*(ptr)))(x), (ptr), sizeof(*(ptr)), true)
> +
> +#define __get_user_allowed(x, ptr) \
> + __get_user_nocheck((x), (ptr), sizeof(*(ptr)), false)
> +#define __put_user_allowed(x, ptr) \
> + __put_user_nocheck((__typeof__(*(ptr)))(x), (ptr), sizeof(*(ptr)), false)
>
> #define __get_user_inatomic(x, ptr) \
> __get_user_nosleep((x), (ptr), sizeof(*(ptr)))
> @@ -138,10 +143,9 @@ extern long __put_user_bad(void);
> : "r" (x), "b" (addr), "i" (-EFAULT), "0" (err))
> #endif /* __powerpc64__ */
>
> -#define __put_user_size(x, ptr, size, retval) \
> +#define __put_user_size_allowed(x, ptr, size, retval) \
> do { \
> retval = 0; \
> - allow_write_to_user(ptr, size); \
> switch (size) { \
> case 1: __put_user_asm(x, ptr, retval, "stb"); break; \
> case 2: __put_user_asm(x, ptr, retval, "sth"); break; \
> @@ -149,17 +153,26 @@ do { \
> case 8: __put_user_asm2(x, ptr, retval); break; \
> default: __put_user_bad(); \
> } \
> +} while (0)
> +
> +#define __put_user_size(x, ptr, size, retval) \
> +do { \
> + allow_write_to_user(ptr, size); \
> + __put_user_size_allowed(x, ptr, size, retval); \
> prevent_write_to_user(ptr, size); \
> } while (0)
>
> -#define __put_user_nocheck(x, ptr, size) \
> +#define __put_user_nocheck(x, ptr, size, allow) \
> ({ \
> long __pu_err; \
> __typeof__(*(ptr)) __user *__pu_addr = (ptr); \
> if (!is_kernel_addr((unsigned long)__pu_addr)) \
> might_fault(); \
> __chk_user_ptr(ptr); \
> - __put_user_size((x), __pu_addr, (size), __pu_err); \
> + if (allow) \
> + __put_user_size((x), __pu_addr, (size), __pu_err); \
> + else \
> + __put_user_size_allowed((x), __pu_addr, (size), __pu_err); \
> __pu_err; \
> })
>
> @@ -236,13 +249,12 @@ extern long __get_user_bad(void);
> : "b" (addr), "i" (-EFAULT), "0" (err))
> #endif /* __powerpc64__ */
>
> -#define __get_user_size(x, ptr, size, retval) \
> +#define __get_user_size_allowed(x, ptr, size, retval) \
> do { \
> retval = 0; \
> __chk_user_ptr(ptr); \
> if (size > sizeof(x)) \
> (x) = __get_user_bad(); \
> - allow_read_from_user(ptr, size); \
> switch (size) { \
> case 1: __get_user_asm(x, ptr, retval, "lbz"); break; \
> case 2: __get_user_asm(x, ptr, retval, "lhz"); break; \
> @@ -250,6 +262,12 @@ do { \
> case 8: __get_user_asm2(x, ptr, retval); break; \
> default: (x) = __get_user_bad(); \
> } \
> +} while (0)
> +
> +#define __get_user_size(x, ptr, size, retval) \
> +do { \
> + allow_read_from_user(ptr, size); \
> + __get_user_size_allowed(x, ptr, size, retval); \
> prevent_read_from_user(ptr, size); \
> } while (0)
>
> @@ -260,7 +278,7 @@ do { \
> #define __long_type(x) \
> __typeof__(__builtin_choose_expr(sizeof(x) > sizeof(0UL), 0ULL, 0UL))
>
> -#define __get_user_nocheck(x, ptr, size) \
> +#define __get_user_nocheck(x, ptr, size, allow) \
> ({ \
> long __gu_err; \
> __long_type(*(ptr)) __gu_val; \
> @@ -269,7 +287,10 @@ do { \
> if (!is_kernel_addr((unsigned long)__gu_addr)) \
> might_fault(); \
> barrier_nospec(); \
> - __get_user_size(__gu_val, __gu_addr, (size), __gu_err); \
> + if (allow) \
> + __get_user_size(__gu_val, __gu_addr, (size), __gu_err); \
> + else \
> + __get_user_size_allowed(__gu_val, __gu_addr, (size), __gu_err); \
> (x) = (__typeof__(*(ptr)))__gu_val; \
> __gu_err; \
> })
> @@ -387,6 +408,34 @@ static inline unsigned long raw_copy_to_user(void __user *to,
> return ret;
> }
>
> +static inline unsigned long
> +raw_copy_to_user_allowed(void __user *to, const void *from, unsigned long n)
> +{
> + unsigned long ret;
> + if (__builtin_constant_p(n) && (n) <= 8) {
> + ret = 1;
> +
> + switch (n) {
> + case 1:
> + __put_user_size_allowed(*(u8 *)from, (u8 __user *)to, 1, ret);
> + break;
> + case 2:
> + __put_user_size_allowed(*(u16 *)from, (u16 __user *)to, 2, ret);
> + break;
> + case 4:
> + __put_user_size_allowed(*(u32 *)from, (u32 __user *)to, 4, ret);
> + break;
> + case 8:
> + __put_user_size_allowed(*(u64 *)from, (u64 __user *)to, 8, ret);
> + break;
> + }
> + if (ret == 0)
> + return 0;
> + }
> +
> + return __copy_tofrom_user(to, (__force const void __user *)from, n);
> +}
> +
> static __always_inline unsigned long __must_check
> copy_to_user_mcsafe(void __user *to, const void *from, unsigned long n)
> {
> @@ -428,4 +477,27 @@ extern long __copy_from_user_flushcache(void *dst, const void __user *src,
> extern void memcpy_page_flushcache(char *to, struct page *page, size_t offset,
> size_t len);
>
> +static __must_check inline bool user_access_begin(const void __user *ptr, size_t len)
> +{
> + if (unlikely(!access_ok(ptr, len)))
> + return false;
> + allow_read_write_user((void __user *)ptr, ptr, len);
> + return true;
> +}
> +#define user_access_begin user_access_begin
> +
> +static inline void user_access_end(void)
> +{
> + prevent_user_access(NULL, NULL, ~0UL, KUAP_SELF);
> +}
> +#define user_access_end user_access_end
> +
> +static inline unsigned long user_access_save(void) { return 0UL; }
> +static inline void user_access_restore(unsigned long flags) { }
> +
> +#define unsafe_op_wrap(op, err) do { if (unlikely(op)) goto err; } while (0)
> +#define unsafe_get_user(x,p,e) unsafe_op_wrap(__get_user_allowed(x,p),e)
> +#define unsafe_put_user(x,p,e) unsafe_op_wrap(__put_user_allowed(x,p),e)
> +#define unsafe_copy_to_user(d,s,l,e) unsafe_op_wrap(raw_copy_to_user_allowed(d,s,l),e)
> +
> #endif /* _ARCH_POWERPC_UACCESS_H */
> --
> 2.25.0
^ permalink raw reply
* Re: [PATCH v2 6/6] powerpc: Implement user_access_begin and friends
From: Michael Ellerman @ 2020-01-23 12:31 UTC (permalink / raw)
To: Christophe Leroy, Benjamin Herrenschmidt, Paul Mackerras
Cc: linux-fsdevel, linux-mm, linuxppc-dev, linux-kernel
In-Reply-To: <87iml2idi9.fsf@mpe.ellerman.id.au>
Michael Ellerman <mpe@ellerman.id.au> writes:
> Christophe Leroy <christophe.leroy@c-s.fr> writes:
>> Today, when a function like strncpy_from_user() is called,
>> the userspace access protection is de-activated and re-activated
>> for every word read.
>>
>> By implementing user_access_begin and friends, the protection
>> is de-activated at the beginning of the copy and re-activated at the
>> end.
>>
>> Implement user_access_begin(), user_access_end() and
>> unsafe_get_user(), unsafe_put_user() and unsafe_copy_to_user()
>>
>> For the time being, we keep user_access_save() and
>> user_access_restore() as nops.
>
> That means we will run with user access enabled in a few more places, but
> it's only used sparingly AFAICS:
>
> kernel/trace/trace_branch.c: unsigned long flags = user_access_save();
> lib/ubsan.c: unsigned long flags = user_access_save();
> lib/ubsan.c: unsigned long ua_flags = user_access_save();
> mm/kasan/common.c: unsigned long flags = user_access_save();
>
> And we don't have objtool checking that user access enablement isn't
> leaking in the first place, so I guess it's OK for us not to implement
> these to begin with?
It looks like we can implement them on on all three KUAP
implementations.
For radix and 8xx we just return/set the relevant SPR.
For book3s/32/kup.h I think we'd just need to add a KUAP_CURRENT case to
allow_user_access()?
cheers
^ permalink raw reply
* Re: [PATCH v2 1/6] fs/readdir: Fix filldir() and filldir64() use of user_access_begin()
From: Christophe Leroy @ 2020-01-23 12:43 UTC (permalink / raw)
To: Michael Ellerman, Benjamin Herrenschmidt, Paul Mackerras,
Linus Torvalds, Alexander Viro, Andrew Morton
Cc: linux-fsdevel, linux-mm, linuxppc-dev, linux-kernel
In-Reply-To: <87k15iidrq.fsf@mpe.ellerman.id.au>
Le 23/01/2020 à 13:00, Michael Ellerman a écrit :
> Michael Ellerman <mpe@ellerman.id.au> writes:
>> Hi Christophe,
>>
>> This patch is independent of the rest of the series AFAICS
>
> And of course having hit send I immediately realise that's not true.
Without this, book3s/32 fails booting. (And without patch 2, it even
hangs, looping forever in do_page_fault()).
>
>> So I'll take patches 2-6 via powerpc and assume this patch will go via
>> Linus or Al or elsewhere.
>
> So I guess I'll wait and see what happens with patch 1.
We could eventually opt out user_access_begin() for
CONFIG_PPC_BOOK3S_32, then you could take patches 3 and 6. That's enough
to have user_access_begin() and stuff for 8xx and RADIX.
Patch 2 should be taken as well as a fix, and can be kept independant of
the series (once we have patch 1, we normally don't hit the problem
fixed by patch 2).
Won't don't need patch 4 until we want user_access_begin() supported by
book3s/32
However, I'm about to send out a v3 with a different approach. It
modifies the core part where user_access_begin() is returning an opaque
value used by user_access_end(). And it also tells user_access_begin()
whether it's a read or a write, so that we can limit unlocking to write
acccesses on book3s/32, and fine grain rights on book3s/64.
Maybe you would prefer this change on top of first step, in which case
I'll be able to make a v4 rebasing all this on top of patch 3 and 6 of
v3 series. Tell me what you prefer.
Christophe
^ permalink raw reply
* [PATCH v3 5/7] powerpc/32s: Drop NULL addr verification
From: Christophe Leroy @ 2020-01-23 12:59 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman
Cc: linux-fsdevel, linux-mm, linuxppc-dev, linux-kernel
In-Reply-To: <fed4f49349913cb6739dac647ba6a61d56b989d2.1579783936.git.christophe.leroy@c-s.fr>
NULL addr is a user address. Don't waste time checking it. If
someone tries to access it, it will SIGFAULT the same way as for
address 1, so no need to make it special.
The special case is when not doing a write, in that case we want
to drop the entire function. This is now handled by 'dir' param
and not by the nulity of 'to' anymore.
Also make beginning of prevent_user_access() similar
to beginning of allow_user_access(), and tell the compiler
that writing in kernel space or with a 0 length is unlikely
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
v2: no change
v3: no change
---
arch/powerpc/include/asm/book3s/32/kup.h | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/arch/powerpc/include/asm/book3s/32/kup.h b/arch/powerpc/include/asm/book3s/32/kup.h
index d765515bd1c1..3c1798e56b55 100644
--- a/arch/powerpc/include/asm/book3s/32/kup.h
+++ b/arch/powerpc/include/asm/book3s/32/kup.h
@@ -113,7 +113,7 @@ static __always_inline void allow_user_access(void __user *to, const void __user
addr = (__force u32)to;
- if (!addr || addr >= TASK_SIZE || !size)
+ if (unlikely(addr >= TASK_SIZE || !size))
return;
end = min(addr + size, TASK_SIZE);
@@ -124,16 +124,18 @@ static __always_inline void allow_user_access(void __user *to, const void __user
static __always_inline void prevent_user_access(void __user *to, const void __user *from,
u32 size, unsigned long dir)
{
- u32 addr = (__force u32)to;
- u32 end = min(addr + size, TASK_SIZE);
+ u32 addr, end;
BUILD_BUG_ON(!__builtin_constant_p(dir));
if (!(dir & KUAP_W))
return;
- if (!addr || addr >= TASK_SIZE || !size)
+ addr = (__force u32)to;
+
+ if (unlikely(addr >= TASK_SIZE || !size))
return;
+ end = min(addr + size, TASK_SIZE);
current->thread.kuap = 0;
kuap_update_sr(mfsrin(addr) | SR_KS, addr, end); /* set Ks */
}
--
2.25.0
^ permalink raw reply related
* [PATCH v3 1/7] fs/readdir: Fix filldir() and filldir64() use of user_access_begin()
From: Christophe Leroy @ 2020-01-23 12:59 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
Linus Torvalds, Alexander Viro, Andrew Morton
Cc: linux-fsdevel, linux-mm, linuxppc-dev, linux-kernel
Some architectures grant full access to userspace regardless of the
address/len passed to user_access_begin(), but other architectures
only grant access to the requested area.
For example, on 32 bits powerpc (book3s/32), access is granted by
segments of 256 Mbytes.
Modify filldir() and filldir64() to request the real area they need
to get access to, i.e. the area covering the parent dirent (if any)
and the contiguous current dirent.
Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Fixes: 9f79b78ef744 ("Convert filldir[64]() from __put_user() to unsafe_put_user()")
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
v2: have user_access_begin() cover both parent dirent (if any) and current dirent
v3: replaced by patch from Linus
---
fs/readdir.c | 70 +++++++++++++++++++++++++---------------------------
1 file changed, 34 insertions(+), 36 deletions(-)
diff --git a/fs/readdir.c b/fs/readdir.c
index d26d5ea4de7b..4b466cbb0f3a 100644
--- a/fs/readdir.c
+++ b/fs/readdir.c
@@ -206,7 +206,7 @@ struct linux_dirent {
struct getdents_callback {
struct dir_context ctx;
struct linux_dirent __user * current_dir;
- struct linux_dirent __user * previous;
+ int prev_reclen;
int count;
int error;
};
@@ -214,12 +214,13 @@ struct getdents_callback {
static int filldir(struct dir_context *ctx, const char *name, int namlen,
loff_t offset, u64 ino, unsigned int d_type)
{
- struct linux_dirent __user * dirent;
+ struct linux_dirent __user *dirent, *prev;
struct getdents_callback *buf =
container_of(ctx, struct getdents_callback, ctx);
unsigned long d_ino;
int reclen = ALIGN(offsetof(struct linux_dirent, d_name) + namlen + 2,
sizeof(long));
+ int prev_reclen;
buf->error = verify_dirent_name(name, namlen);
if (unlikely(buf->error))
@@ -232,28 +233,24 @@ static int filldir(struct dir_context *ctx, const char *name, int namlen,
buf->error = -EOVERFLOW;
return -EOVERFLOW;
}
- dirent = buf->previous;
- if (dirent && signal_pending(current))
+ prev_reclen = buf->prev_reclen;
+ if (prev_reclen && signal_pending(current))
return -EINTR;
-
- /*
- * Note! This range-checks 'previous' (which may be NULL).
- * The real range was checked in getdents
- */
- if (!user_access_begin(dirent, sizeof(*dirent)))
- goto efault;
- if (dirent)
- unsafe_put_user(offset, &dirent->d_off, efault_end);
dirent = buf->current_dir;
+ prev = (void __user *)dirent - prev_reclen;
+ if (!user_access_begin(prev, reclen + prev_reclen))
+ goto efault;
+
+ /* This might be 'dirent->d_off', but if so it will get overwritten */
+ unsafe_put_user(offset, &prev->d_off, efault_end);
unsafe_put_user(d_ino, &dirent->d_ino, efault_end);
unsafe_put_user(reclen, &dirent->d_reclen, efault_end);
unsafe_put_user(d_type, (char __user *) dirent + reclen - 1, efault_end);
unsafe_copy_dirent_name(dirent->d_name, name, namlen, efault_end);
user_access_end();
- buf->previous = dirent;
- dirent = (void __user *)dirent + reclen;
- buf->current_dir = dirent;
+ buf->current_dir = (void __user *)dirent + reclen;
+ buf->prev_reclen = reclen;
buf->count -= reclen;
return 0;
efault_end:
@@ -267,7 +264,6 @@ SYSCALL_DEFINE3(getdents, unsigned int, fd,
struct linux_dirent __user *, dirent, unsigned int, count)
{
struct fd f;
- struct linux_dirent __user * lastdirent;
struct getdents_callback buf = {
.ctx.actor = filldir,
.count = count,
@@ -285,8 +281,10 @@ SYSCALL_DEFINE3(getdents, unsigned int, fd,
error = iterate_dir(f.file, &buf.ctx);
if (error >= 0)
error = buf.error;
- lastdirent = buf.previous;
- if (lastdirent) {
+ if (buf.prev_reclen) {
+ struct linux_dirent __user *lastdirent;
+ lastdirent = (void __user *)buf.current_dir - buf.prev_reclen;
+
if (put_user(buf.ctx.pos, &lastdirent->d_off))
error = -EFAULT;
else
@@ -299,7 +297,7 @@ SYSCALL_DEFINE3(getdents, unsigned int, fd,
struct getdents_callback64 {
struct dir_context ctx;
struct linux_dirent64 __user * current_dir;
- struct linux_dirent64 __user * previous;
+ int prev_reclen;
int count;
int error;
};
@@ -307,11 +305,12 @@ struct getdents_callback64 {
static int filldir64(struct dir_context *ctx, const char *name, int namlen,
loff_t offset, u64 ino, unsigned int d_type)
{
- struct linux_dirent64 __user *dirent;
+ struct linux_dirent64 __user *dirent, *prev;
struct getdents_callback64 *buf =
container_of(ctx, struct getdents_callback64, ctx);
int reclen = ALIGN(offsetof(struct linux_dirent64, d_name) + namlen + 1,
sizeof(u64));
+ int prev_reclen;
buf->error = verify_dirent_name(name, namlen);
if (unlikely(buf->error))
@@ -319,30 +318,28 @@ static int filldir64(struct dir_context *ctx, const char *name, int namlen,
buf->error = -EINVAL; /* only used if we fail.. */
if (reclen > buf->count)
return -EINVAL;
- dirent = buf->previous;
- if (dirent && signal_pending(current))
+ prev_reclen = buf->prev_reclen;
+ if (prev_reclen && signal_pending(current))
return -EINTR;
-
- /*
- * Note! This range-checks 'previous' (which may be NULL).
- * The real range was checked in getdents
- */
- if (!user_access_begin(dirent, sizeof(*dirent)))
- goto efault;
- if (dirent)
- unsafe_put_user(offset, &dirent->d_off, efault_end);
dirent = buf->current_dir;
+ prev = (void __user *)dirent - prev_reclen;
+ if (!user_access_begin(prev, reclen + prev_reclen))
+ goto efault;
+
+ /* This might be 'dirent->d_off', but if so it will get overwritten */
+ unsafe_put_user(offset, &prev->d_off, efault_end);
unsafe_put_user(ino, &dirent->d_ino, efault_end);
unsafe_put_user(reclen, &dirent->d_reclen, efault_end);
unsafe_put_user(d_type, &dirent->d_type, efault_end);
unsafe_copy_dirent_name(dirent->d_name, name, namlen, efault_end);
user_access_end();
- buf->previous = dirent;
+ buf->prev_reclen = reclen;
dirent = (void __user *)dirent + reclen;
buf->current_dir = dirent;
buf->count -= reclen;
return 0;
+
efault_end:
user_access_end();
efault:
@@ -354,7 +351,6 @@ int ksys_getdents64(unsigned int fd, struct linux_dirent64 __user *dirent,
unsigned int count)
{
struct fd f;
- struct linux_dirent64 __user * lastdirent;
struct getdents_callback64 buf = {
.ctx.actor = filldir64,
.count = count,
@@ -372,9 +368,11 @@ int ksys_getdents64(unsigned int fd, struct linux_dirent64 __user *dirent,
error = iterate_dir(f.file, &buf.ctx);
if (error >= 0)
error = buf.error;
- lastdirent = buf.previous;
- if (lastdirent) {
+ if (buf.prev_reclen) {
+ struct linux_dirent64 __user *lastdirent;
typeof(lastdirent->d_off) d_off = buf.ctx.pos;
+
+ lastdirent = (void __user *)buf.current_dir - buf.prev_reclen;
if (__put_user(d_off, &lastdirent->d_off))
error = -EFAULT;
else
--
2.25.0
^ permalink raw reply related
* [PATCH v3 4/7] powerpc/kuap: Fix set direction in allow/prevent_user_access()
From: Christophe Leroy @ 2020-01-23 12:59 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman
Cc: linux-fsdevel, linux-mm, linuxppc-dev, linux-kernel
In-Reply-To: <fed4f49349913cb6739dac647ba6a61d56b989d2.1579783936.git.christophe.leroy@c-s.fr>
__builtin_constant_p() always return 0 for pointers, so on RADIX
we always end up opening both direction (by writing 0 in SPR29):
0000000000000170 <._copy_to_user>:
...
1b0: 4c 00 01 2c isync
1b4: 39 20 00 00 li r9,0
1b8: 7d 3d 03 a6 mtspr 29,r9
1bc: 4c 00 01 2c isync
1c0: 48 00 00 01 bl 1c0 <._copy_to_user+0x50>
1c0: R_PPC64_REL24 .__copy_tofrom_user
...
0000000000000220 <._copy_from_user>:
...
2ac: 4c 00 01 2c isync
2b0: 39 20 00 00 li r9,0
2b4: 7d 3d 03 a6 mtspr 29,r9
2b8: 4c 00 01 2c isync
2bc: 7f c5 f3 78 mr r5,r30
2c0: 7f 83 e3 78 mr r3,r28
2c4: 48 00 00 01 bl 2c4 <._copy_from_user+0xa4>
2c4: R_PPC64_REL24 .__copy_tofrom_user
...
Use an explicit parameter for direction selection, so that GCC
is able to see it is a constant:
00000000000001b0 <._copy_to_user>:
...
1f0: 4c 00 01 2c isync
1f4: 3d 20 40 00 lis r9,16384
1f8: 79 29 07 c6 rldicr r9,r9,32,31
1fc: 7d 3d 03 a6 mtspr 29,r9
200: 4c 00 01 2c isync
204: 48 00 00 01 bl 204 <._copy_to_user+0x54>
204: R_PPC64_REL24 .__copy_tofrom_user
...
0000000000000260 <._copy_from_user>:
...
2ec: 4c 00 01 2c isync
2f0: 39 20 ff ff li r9,-1
2f4: 79 29 00 04 rldicr r9,r9,0,0
2f8: 7d 3d 03 a6 mtspr 29,r9
2fc: 4c 00 01 2c isync
300: 7f c5 f3 78 mr r5,r30
304: 7f 83 e3 78 mr r3,r28
308: 48 00 00 01 bl 308 <._copy_from_user+0xa8>
308: R_PPC64_REL24 .__copy_tofrom_user
...
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
v2: no change
v3: no change
---
arch/powerpc/include/asm/book3s/32/kup.h | 13 ++++++--
.../powerpc/include/asm/book3s/64/kup-radix.h | 11 +++----
arch/powerpc/include/asm/kup.h | 30 ++++++++++++++-----
arch/powerpc/include/asm/nohash/32/kup-8xx.h | 4 +--
arch/powerpc/include/asm/uaccess.h | 4 +--
5 files changed, 43 insertions(+), 19 deletions(-)
diff --git a/arch/powerpc/include/asm/book3s/32/kup.h b/arch/powerpc/include/asm/book3s/32/kup.h
index d88008c8eb85..d765515bd1c1 100644
--- a/arch/powerpc/include/asm/book3s/32/kup.h
+++ b/arch/powerpc/include/asm/book3s/32/kup.h
@@ -102,11 +102,13 @@ static inline void kuap_update_sr(u32 sr, u32 addr, u32 end)
isync(); /* Context sync required after mtsrin() */
}
-static inline void allow_user_access(void __user *to, const void __user *from, u32 size)
+static __always_inline void allow_user_access(void __user *to, const void __user *from,
+ u32 size, unsigned long dir)
{
u32 addr, end;
- if (__builtin_constant_p(to) && to == NULL)
+ BUILD_BUG_ON(!__builtin_constant_p(dir));
+ if (!(dir & KUAP_W))
return;
addr = (__force u32)to;
@@ -119,11 +121,16 @@ static inline void allow_user_access(void __user *to, const void __user *from, u
kuap_update_sr(mfsrin(addr) & ~SR_KS, addr, end); /* Clear Ks */
}
-static inline void prevent_user_access(void __user *to, const void __user *from, u32 size)
+static __always_inline void prevent_user_access(void __user *to, const void __user *from,
+ u32 size, unsigned long dir)
{
u32 addr = (__force u32)to;
u32 end = min(addr + size, TASK_SIZE);
+ BUILD_BUG_ON(!__builtin_constant_p(dir));
+ if (!(dir & KUAP_W))
+ return;
+
if (!addr || addr >= TASK_SIZE || !size)
return;
diff --git a/arch/powerpc/include/asm/book3s/64/kup-radix.h b/arch/powerpc/include/asm/book3s/64/kup-radix.h
index dbbd22cb80f5..f11315306d41 100644
--- a/arch/powerpc/include/asm/book3s/64/kup-radix.h
+++ b/arch/powerpc/include/asm/book3s/64/kup-radix.h
@@ -77,20 +77,21 @@ static inline void set_kuap(unsigned long value)
isync();
}
-static inline void allow_user_access(void __user *to, const void __user *from,
- unsigned long size)
+static __always_inline void allow_user_access(void __user *to, const void __user *from,
+ unsigned long size, unsigned long dir)
{
// This is written so we can resolve to a single case at build time
- if (__builtin_constant_p(to) && to == NULL)
+ BUILD_BUG_ON(!__builtin_constant_p(dir));
+ if (dir == KUAP_R)
set_kuap(AMR_KUAP_BLOCK_WRITE);
- else if (__builtin_constant_p(from) && from == NULL)
+ else if (dir == KUAP_W)
set_kuap(AMR_KUAP_BLOCK_READ);
else
set_kuap(0);
}
static inline void prevent_user_access(void __user *to, const void __user *from,
- unsigned long size)
+ unsigned long size, unsigned long dir)
{
set_kuap(AMR_KUAP_BLOCKED);
}
diff --git a/arch/powerpc/include/asm/kup.h b/arch/powerpc/include/asm/kup.h
index 812e66f31934..ff57bfcb88f7 100644
--- a/arch/powerpc/include/asm/kup.h
+++ b/arch/powerpc/include/asm/kup.h
@@ -2,6 +2,10 @@
#ifndef _ASM_POWERPC_KUP_H_
#define _ASM_POWERPC_KUP_H_
+#define KUAP_R 1
+#define KUAP_W 2
+#define KUAP_RW (KUAP_R | KUAP_W)
+
#ifdef CONFIG_PPC64
#include <asm/book3s/64/kup-radix.h>
#endif
@@ -42,9 +46,9 @@ void setup_kuap(bool disabled);
#else
static inline void setup_kuap(bool disabled) { }
static inline void allow_user_access(void __user *to, const void __user *from,
- unsigned long size) { }
+ unsigned long size, unsigned long dir) { }
static inline void prevent_user_access(void __user *to, const void __user *from,
- unsigned long size) { }
+ unsigned long size, unsigned long dir) { }
static inline bool
bad_kuap_fault(struct pt_regs *regs, unsigned long address, bool is_write)
{
@@ -54,24 +58,36 @@ bad_kuap_fault(struct pt_regs *regs, unsigned long address, bool is_write)
static inline void allow_read_from_user(const void __user *from, unsigned long size)
{
- allow_user_access(NULL, from, size);
+ allow_user_access(NULL, from, size, KUAP_R);
}
static inline void allow_write_to_user(void __user *to, unsigned long size)
{
- allow_user_access(to, NULL, size);
+ allow_user_access(to, NULL, size, KUAP_W);
+}
+
+static inline void allow_read_write_user(void __user *to, const void __user *from,
+ unsigned long size)
+{
+ allow_user_access(to, from, size, KUAP_RW);
}
static inline void prevent_read_from_user(const void __user *from, unsigned long size)
{
- prevent_user_access(NULL, from, size);
+ prevent_user_access(NULL, from, size, KUAP_R);
}
static inline void prevent_write_to_user(void __user *to, unsigned long size)
{
- prevent_user_access(to, NULL, size);
+ prevent_user_access(to, NULL, size, KUAP_W);
+}
+
+static inline void prevent_read_write_user(void __user *to, const void __user *from,
+ unsigned long size)
+{
+ prevent_user_access(to, from, size, KUAP_RW);
}
#endif /* !__ASSEMBLY__ */
-#endif /* _ASM_POWERPC_KUP_H_ */
+#endif /* _ASM_POWERPC_KUAP_H_ */
diff --git a/arch/powerpc/include/asm/nohash/32/kup-8xx.h b/arch/powerpc/include/asm/nohash/32/kup-8xx.h
index f2fea603b929..1d70c80366fd 100644
--- a/arch/powerpc/include/asm/nohash/32/kup-8xx.h
+++ b/arch/powerpc/include/asm/nohash/32/kup-8xx.h
@@ -35,13 +35,13 @@
#include <asm/reg.h>
static inline void allow_user_access(void __user *to, const void __user *from,
- unsigned long size)
+ unsigned long size, unsigned long dir)
{
mtspr(SPRN_MD_AP, MD_APG_INIT);
}
static inline void prevent_user_access(void __user *to, const void __user *from,
- unsigned long size)
+ unsigned long size, unsigned long dir)
{
mtspr(SPRN_MD_AP, MD_APG_KUAP);
}
diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
index c92fe7fe9692..cafad1960e76 100644
--- a/arch/powerpc/include/asm/uaccess.h
+++ b/arch/powerpc/include/asm/uaccess.h
@@ -313,9 +313,9 @@ raw_copy_in_user(void __user *to, const void __user *from, unsigned long n)
unsigned long ret;
barrier_nospec();
- allow_user_access(to, from, n);
+ allow_read_write_user(to, from, n);
ret = __copy_tofrom_user(to, from, n);
- prevent_user_access(to, from, n);
+ prevent_read_write_user(to, from, n);
return ret;
}
#endif /* __powerpc64__ */
--
2.25.0
^ permalink raw reply related
* [PATCH v3 2/7] uaccess: Tell user_access_begin() if it's for a write or not
From: Christophe Leroy @ 2020-01-23 12:59 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
Linus Torvalds, Alexander Viro, Andrew Morton, Thomas Gleixner,
Ingo Molnar, Borislav Petkov, H. Peter Anvin, Jani Nikula
Cc: x86, linux-kernel, dri-devel, linux-mm, linux-fsdevel,
linuxppc-dev
In-Reply-To: <fed4f49349913cb6739dac647ba6a61d56b989d2.1579783936.git.christophe.leroy@c-s.fr>
On 32 bits powerPC (book3s/32), only write accesses to user are
protected and there is no point spending time on unlocking for reads.
On 64 bits powerpc (book3s/64 at least), access can be granted
read only, write only or read/write.
Add an argument to user_access_begin() to tell when it's for write and
return an opaque key that will be used by user_access_end() to know
what was done by user_access_begin().
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
v3: new
---
arch/x86/include/asm/uaccess.h | 5 +++--
drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 15 ++++++++++-----
fs/readdir.c | 16 ++++++++++------
include/linux/uaccess.h | 4 ++--
kernel/compat.c | 16 ++++++++++------
kernel/exit.c | 17 +++++++++++------
lib/strncpy_from_user.c | 6 ++++--
lib/strnlen_user.c | 6 ++++--
lib/usercopy.c | 8 +++++---
9 files changed, 59 insertions(+), 34 deletions(-)
diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h
index 61d93f062a36..05eccdc0366a 100644
--- a/arch/x86/include/asm/uaccess.h
+++ b/arch/x86/include/asm/uaccess.h
@@ -709,7 +709,8 @@ extern struct movsl_mask {
* checking before using them, but you have to surround them with the
* user_access_begin/end() pair.
*/
-static __must_check __always_inline bool user_access_begin(const void __user *ptr, size_t len)
+static __must_check __always_inline unsigned long
+user_access_begin(const void __user *ptr, size_t len, bool write)
{
if (unlikely(!access_ok(ptr,len)))
return 0;
@@ -717,7 +718,7 @@ static __must_check __always_inline bool user_access_begin(const void __user *pt
return 1;
}
#define user_access_begin(a,b) user_access_begin(a,b)
-#define user_access_end() __uaccess_end()
+#define user_access_end(x) __uaccess_end()
#define user_access_save() smap_save()
#define user_access_restore(x) smap_restore(x)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
index bc3a67226163..509bfb6116ac 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
@@ -1615,6 +1615,7 @@ static int eb_copy_relocations(const struct i915_execbuffer *eb)
const unsigned int count = eb->buffer_count;
unsigned int i;
int err;
+ unsigned long key;
for (i = 0; i < count; i++) {
const unsigned int nreloc = eb->exec[i].relocation_count;
@@ -1662,14 +1663,15 @@ static int eb_copy_relocations(const struct i915_execbuffer *eb)
* happened we would make the mistake of assuming that the
* relocations were valid.
*/
- if (!user_access_begin(urelocs, size))
+ key = user_access_begin(urelocs, size, true);
+ if (!key)
goto end;
for (copied = 0; copied < nreloc; copied++)
unsafe_put_user(-1,
&urelocs[copied].presumed_offset,
end_user);
- user_access_end();
+ user_access_end(key);
eb->exec[i].relocs_ptr = (uintptr_t)relocs;
}
@@ -1677,7 +1679,7 @@ static int eb_copy_relocations(const struct i915_execbuffer *eb)
return 0;
end_user:
- user_access_end();
+ user_access_end(key);
end:
kvfree(relocs);
err = -EFAULT;
@@ -2906,6 +2908,7 @@ i915_gem_execbuffer2_ioctl(struct drm_device *dev, void *data,
struct drm_i915_gem_exec_object2 __user *user_exec_list =
u64_to_user_ptr(args->buffers_ptr);
unsigned int i;
+ unsigned long key;
/* Copy the new buffer offsets back to the user's exec list. */
/*
@@ -2915,7 +2918,9 @@ i915_gem_execbuffer2_ioctl(struct drm_device *dev, void *data,
* And this range already got effectively checked earlier
* when we did the "copy_from_user()" above.
*/
- if (!user_access_begin(user_exec_list, count * sizeof(*user_exec_list)))
+ key = user_access_begin(user_exec_list,
+ count * sizeof(*user_exec_list), true);
+ if (!key)
goto end;
for (i = 0; i < args->buffer_count; i++) {
@@ -2929,7 +2934,7 @@ i915_gem_execbuffer2_ioctl(struct drm_device *dev, void *data,
end_user);
}
end_user:
- user_access_end();
+ user_access_end(key);
end:;
}
diff --git a/fs/readdir.c b/fs/readdir.c
index 4b466cbb0f3a..47b9ef97e16e 100644
--- a/fs/readdir.c
+++ b/fs/readdir.c
@@ -221,6 +221,7 @@ static int filldir(struct dir_context *ctx, const char *name, int namlen,
int reclen = ALIGN(offsetof(struct linux_dirent, d_name) + namlen + 2,
sizeof(long));
int prev_reclen;
+ unsigned long key;
buf->error = verify_dirent_name(name, namlen);
if (unlikely(buf->error))
@@ -238,7 +239,8 @@ static int filldir(struct dir_context *ctx, const char *name, int namlen,
return -EINTR;
dirent = buf->current_dir;
prev = (void __user *)dirent - prev_reclen;
- if (!user_access_begin(prev, reclen + prev_reclen))
+ key = user_access_begin(prev, reclen + prev_reclen, true);
+ if (!key)
goto efault;
/* This might be 'dirent->d_off', but if so it will get overwritten */
@@ -247,14 +249,14 @@ static int filldir(struct dir_context *ctx, const char *name, int namlen,
unsafe_put_user(reclen, &dirent->d_reclen, efault_end);
unsafe_put_user(d_type, (char __user *) dirent + reclen - 1, efault_end);
unsafe_copy_dirent_name(dirent->d_name, name, namlen, efault_end);
- user_access_end();
+ user_access_end(key);
buf->current_dir = (void __user *)dirent + reclen;
buf->prev_reclen = reclen;
buf->count -= reclen;
return 0;
efault_end:
- user_access_end();
+ user_access_end(key);
efault:
buf->error = -EFAULT;
return -EFAULT;
@@ -311,6 +313,7 @@ static int filldir64(struct dir_context *ctx, const char *name, int namlen,
int reclen = ALIGN(offsetof(struct linux_dirent64, d_name) + namlen + 1,
sizeof(u64));
int prev_reclen;
+ unsigned long key;
buf->error = verify_dirent_name(name, namlen);
if (unlikely(buf->error))
@@ -323,7 +326,8 @@ static int filldir64(struct dir_context *ctx, const char *name, int namlen,
return -EINTR;
dirent = buf->current_dir;
prev = (void __user *)dirent - prev_reclen;
- if (!user_access_begin(prev, reclen + prev_reclen))
+ key = user_access_begin(prev, reclen + prev_reclen, true);
+ if (!key)
goto efault;
/* This might be 'dirent->d_off', but if so it will get overwritten */
@@ -332,7 +336,7 @@ static int filldir64(struct dir_context *ctx, const char *name, int namlen,
unsafe_put_user(reclen, &dirent->d_reclen, efault_end);
unsafe_put_user(d_type, &dirent->d_type, efault_end);
unsafe_copy_dirent_name(dirent->d_name, name, namlen, efault_end);
- user_access_end();
+ user_access_end(key);
buf->prev_reclen = reclen;
dirent = (void __user *)dirent + reclen;
@@ -341,7 +345,7 @@ static int filldir64(struct dir_context *ctx, const char *name, int namlen,
return 0;
efault_end:
- user_access_end();
+ user_access_end(key);
efault:
buf->error = -EFAULT;
return -EFAULT;
diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h
index 67f016010aad..394f5029a727 100644
--- a/include/linux/uaccess.h
+++ b/include/linux/uaccess.h
@@ -369,8 +369,8 @@ extern long strnlen_unsafe_user(const void __user *unsafe_addr, long count);
probe_kernel_read(&retval, addr, sizeof(retval))
#ifndef user_access_begin
-#define user_access_begin(ptr,len) access_ok(ptr, len)
-#define user_access_end() do { } while (0)
+#define user_access_begin(ptr, len, write) access_ok(ptr, len)
+#define user_access_end(x) do { } while (0)
#define unsafe_op_wrap(op, err) do { if (unlikely(op)) goto err; } while (0)
#define unsafe_get_user(x,p,e) unsafe_op_wrap(__get_user(x,p),e)
#define unsafe_put_user(x,p,e) unsafe_op_wrap(__put_user(x,p),e)
diff --git a/kernel/compat.c b/kernel/compat.c
index 95005f849c68..4bcbe1cd761b 100644
--- a/kernel/compat.c
+++ b/kernel/compat.c
@@ -258,12 +258,14 @@ long compat_get_bitmap(unsigned long *mask, const compat_ulong_t __user *umask,
unsigned long bitmap_size)
{
unsigned long nr_compat_longs;
+ unsigned long key;
/* align bitmap up to nearest compat_long_t boundary */
bitmap_size = ALIGN(bitmap_size, BITS_PER_COMPAT_LONG);
nr_compat_longs = BITS_TO_COMPAT_LONGS(bitmap_size);
- if (!user_access_begin(umask, bitmap_size / 8))
+ key = user_access_begin(umask, bitmap_size / 8, false);
+ if (!key)
return -EFAULT;
while (nr_compat_longs > 1) {
@@ -275,11 +277,11 @@ long compat_get_bitmap(unsigned long *mask, const compat_ulong_t __user *umask,
}
if (nr_compat_longs)
unsafe_get_user(*mask, umask++, Efault);
- user_access_end();
+ user_access_end(key);
return 0;
Efault:
- user_access_end();
+ user_access_end(key);
return -EFAULT;
}
@@ -287,12 +289,14 @@ long compat_put_bitmap(compat_ulong_t __user *umask, unsigned long *mask,
unsigned long bitmap_size)
{
unsigned long nr_compat_longs;
+ unsigned long key;
/* align bitmap up to nearest compat_long_t boundary */
bitmap_size = ALIGN(bitmap_size, BITS_PER_COMPAT_LONG);
nr_compat_longs = BITS_TO_COMPAT_LONGS(bitmap_size);
- if (!user_access_begin(umask, bitmap_size / 8))
+ key = user_access_begin(umask, bitmap_size / 8, true);
+ if (!key)
return -EFAULT;
while (nr_compat_longs > 1) {
@@ -303,10 +307,10 @@ long compat_put_bitmap(compat_ulong_t __user *umask, unsigned long *mask,
}
if (nr_compat_longs)
unsafe_put_user((compat_ulong_t)*mask, umask++, Efault);
- user_access_end();
+ user_access_end(key);
return 0;
Efault:
- user_access_end();
+ user_access_end(key);
return -EFAULT;
}
diff --git a/kernel/exit.c b/kernel/exit.c
index 2833ffb0c211..1cb9c8a879d2 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -1553,6 +1553,7 @@ SYSCALL_DEFINE5(waitid, int, which, pid_t, upid, struct siginfo __user *,
struct waitid_info info = {.status = 0};
long err = kernel_waitid(which, upid, &info, options, ru ? &r : NULL);
int signo = 0;
+ unsigned long key;
if (err > 0) {
signo = SIGCHLD;
@@ -1563,7 +1564,8 @@ SYSCALL_DEFINE5(waitid, int, which, pid_t, upid, struct siginfo __user *,
if (!infop)
return err;
- if (!user_access_begin(infop, sizeof(*infop)))
+ key = user_access_begin(infop, sizeof(*infop), true);
+ if (!key)
return -EFAULT;
unsafe_put_user(signo, &infop->si_signo, Efault);
@@ -1572,10 +1574,10 @@ SYSCALL_DEFINE5(waitid, int, which, pid_t, upid, struct siginfo __user *,
unsafe_put_user(info.pid, &infop->si_pid, Efault);
unsafe_put_user(info.uid, &infop->si_uid, Efault);
unsafe_put_user(info.status, &infop->si_status, Efault);
- user_access_end();
+ user_access_end(key);
return err;
Efault:
- user_access_end();
+ user_access_end(key);
return -EFAULT;
}
@@ -1673,6 +1675,8 @@ COMPAT_SYSCALL_DEFINE5(waitid,
struct waitid_info info = {.status = 0};
long err = kernel_waitid(which, pid, &info, options, uru ? &ru : NULL);
int signo = 0;
+ unsigned long key;
+
if (err > 0) {
signo = SIGCHLD;
err = 0;
@@ -1690,7 +1694,8 @@ COMPAT_SYSCALL_DEFINE5(waitid,
if (!infop)
return err;
- if (!user_access_begin(infop, sizeof(*infop)))
+ key = user_access_begin(infop, sizeof(*infop), true);
+ if (!key)
return -EFAULT;
unsafe_put_user(signo, &infop->si_signo, Efault);
@@ -1699,10 +1704,10 @@ COMPAT_SYSCALL_DEFINE5(waitid,
unsafe_put_user(info.pid, &infop->si_pid, Efault);
unsafe_put_user(info.uid, &infop->si_uid, Efault);
unsafe_put_user(info.status, &infop->si_status, Efault);
- user_access_end();
+ user_access_end(key);
return err;
Efault:
- user_access_end();
+ user_access_end(key);
return -EFAULT;
}
#endif
diff --git a/lib/strncpy_from_user.c b/lib/strncpy_from_user.c
index dccb95af6003..7184fb766439 100644
--- a/lib/strncpy_from_user.c
+++ b/lib/strncpy_from_user.c
@@ -113,12 +113,14 @@ long strncpy_from_user(char *dst, const char __user *src, long count)
if (likely(src_addr < max_addr)) {
unsigned long max = max_addr - src_addr;
long retval;
+ unsigned long key;
kasan_check_write(dst, count);
check_object_size(dst, count, false);
- if (user_access_begin(src, max)) {
+ key = user_access_begin(src, max, false);
+ if (key) {
retval = do_strncpy_from_user(dst, src, count, max);
- user_access_end();
+ user_access_end(key);
return retval;
}
}
diff --git a/lib/strnlen_user.c b/lib/strnlen_user.c
index 6c0005d5dd5c..819e355b8608 100644
--- a/lib/strnlen_user.c
+++ b/lib/strnlen_user.c
@@ -108,10 +108,12 @@ long strnlen_user(const char __user *str, long count)
if (likely(src_addr < max_addr)) {
unsigned long max = max_addr - src_addr;
long retval;
+ unsigned long key;
- if (user_access_begin(str, max)) {
+ key = user_access_begin(str, max, false);
+ if (key) {
retval = do_strnlen_user(str, count, max);
- user_access_end();
+ user_access_end(key);
return retval;
}
}
diff --git a/lib/usercopy.c b/lib/usercopy.c
index cbb4d9ec00f2..9e03ca88ad32 100644
--- a/lib/usercopy.c
+++ b/lib/usercopy.c
@@ -51,6 +51,7 @@ int check_zeroed_user(const void __user *from, size_t size)
{
unsigned long val;
uintptr_t align = (uintptr_t) from % sizeof(unsigned long);
+ unsigned long key;
if (unlikely(size == 0))
return 1;
@@ -58,7 +59,8 @@ int check_zeroed_user(const void __user *from, size_t size)
from -= align;
size += align;
- if (!user_access_begin(from, size))
+ key = user_access_begin(from, size, false);
+ if (!key)
return -EFAULT;
unsafe_get_user(val, (unsigned long __user *) from, err_fault);
@@ -79,10 +81,10 @@ int check_zeroed_user(const void __user *from, size_t size)
val &= aligned_byte_mask(size);
done:
- user_access_end();
+ user_access_end(key);
return (val == 0);
err_fault:
- user_access_end();
+ user_access_end(key);
return -EFAULT;
}
EXPORT_SYMBOL(check_zeroed_user);
--
2.25.0
^ permalink raw reply related
* [PATCH v3 3/7] powerpc/32s: Fix bad_kuap_fault()
From: Christophe Leroy @ 2020-01-23 12:59 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman
Cc: linux-fsdevel, linux-mm, linuxppc-dev, linux-kernel
In-Reply-To: <fed4f49349913cb6739dac647ba6a61d56b989d2.1579783936.git.christophe.leroy@c-s.fr>
At the moment, bad_kuap_fault() reports a fault only if a bad access
to userspace occurred while access to userspace was not granted.
But if a fault occurs for a write outside the allowed userspace
segment(s) that have been unlocked, bad_kuap_fault() fails to
detect it and the kernel loops forever in do_page_fault().
Fix it by checking that the accessed address is within the allowed
range.
Fixes: a68c31fc01ef ("powerpc/32s: Implement Kernel Userspace Access Protection")
Cc: stable@vger.kernel.org
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
v2: added missing address parametre to bad_kuap_fault() in asm/kup.h
v3: no change
---
arch/powerpc/include/asm/book3s/32/kup.h | 9 +++++++--
arch/powerpc/include/asm/book3s/64/kup-radix.h | 3 ++-
arch/powerpc/include/asm/kup.h | 6 +++++-
arch/powerpc/include/asm/nohash/32/kup-8xx.h | 3 ++-
arch/powerpc/mm/fault.c | 2 +-
5 files changed, 17 insertions(+), 6 deletions(-)
diff --git a/arch/powerpc/include/asm/book3s/32/kup.h b/arch/powerpc/include/asm/book3s/32/kup.h
index f9dc597b0b86..d88008c8eb85 100644
--- a/arch/powerpc/include/asm/book3s/32/kup.h
+++ b/arch/powerpc/include/asm/book3s/32/kup.h
@@ -131,12 +131,17 @@ static inline void prevent_user_access(void __user *to, const void __user *from,
kuap_update_sr(mfsrin(addr) | SR_KS, addr, end); /* set Ks */
}
-static inline bool bad_kuap_fault(struct pt_regs *regs, bool is_write)
+static inline bool
+bad_kuap_fault(struct pt_regs *regs, unsigned long address, bool is_write)
{
+ unsigned long begin = regs->kuap & 0xf0000000;
+ unsigned long end = regs->kuap << 28;
+
if (!is_write)
return false;
- return WARN(!regs->kuap, "Bug: write fault blocked by segment registers !");
+ return WARN(address < begin || address >= end,
+ "Bug: write fault blocked by segment registers !");
}
#endif /* CONFIG_PPC_KUAP */
diff --git a/arch/powerpc/include/asm/book3s/64/kup-radix.h b/arch/powerpc/include/asm/book3s/64/kup-radix.h
index f254de956d6a..dbbd22cb80f5 100644
--- a/arch/powerpc/include/asm/book3s/64/kup-radix.h
+++ b/arch/powerpc/include/asm/book3s/64/kup-radix.h
@@ -95,7 +95,8 @@ static inline void prevent_user_access(void __user *to, const void __user *from,
set_kuap(AMR_KUAP_BLOCKED);
}
-static inline bool bad_kuap_fault(struct pt_regs *regs, bool is_write)
+static inline bool
+bad_kuap_fault(struct pt_regs *regs, unsigned long address, bool is_write)
{
return WARN(mmu_has_feature(MMU_FTR_RADIX_KUAP) &&
(regs->kuap & (is_write ? AMR_KUAP_BLOCK_WRITE : AMR_KUAP_BLOCK_READ)),
diff --git a/arch/powerpc/include/asm/kup.h b/arch/powerpc/include/asm/kup.h
index 5b5e39643a27..812e66f31934 100644
--- a/arch/powerpc/include/asm/kup.h
+++ b/arch/powerpc/include/asm/kup.h
@@ -45,7 +45,11 @@ static inline void allow_user_access(void __user *to, const void __user *from,
unsigned long size) { }
static inline void prevent_user_access(void __user *to, const void __user *from,
unsigned long size) { }
-static inline bool bad_kuap_fault(struct pt_regs *regs, bool is_write) { return false; }
+static inline bool
+bad_kuap_fault(struct pt_regs *regs, unsigned long address, bool is_write)
+{
+ return false;
+}
#endif /* CONFIG_PPC_KUAP */
static inline void allow_read_from_user(const void __user *from, unsigned long size)
diff --git a/arch/powerpc/include/asm/nohash/32/kup-8xx.h b/arch/powerpc/include/asm/nohash/32/kup-8xx.h
index 1006a427e99c..f2fea603b929 100644
--- a/arch/powerpc/include/asm/nohash/32/kup-8xx.h
+++ b/arch/powerpc/include/asm/nohash/32/kup-8xx.h
@@ -46,7 +46,8 @@ static inline void prevent_user_access(void __user *to, const void __user *from,
mtspr(SPRN_MD_AP, MD_APG_KUAP);
}
-static inline bool bad_kuap_fault(struct pt_regs *regs, bool is_write)
+static inline bool
+bad_kuap_fault(struct pt_regs *regs, unsigned long address, bool is_write)
{
return WARN(!((regs->kuap ^ MD_APG_KUAP) & 0xf0000000),
"Bug: fault blocked by AP register !");
diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
index b5047f9b5dec..1baeb045f7f4 100644
--- a/arch/powerpc/mm/fault.c
+++ b/arch/powerpc/mm/fault.c
@@ -233,7 +233,7 @@ static bool bad_kernel_fault(struct pt_regs *regs, unsigned long error_code,
// Read/write fault in a valid region (the exception table search passed
// above), but blocked by KUAP is bad, it can never succeed.
- if (bad_kuap_fault(regs, is_write))
+ if (bad_kuap_fault(regs, address, is_write))
return true;
// What's left? Kernel fault on user in well defined regions (extable
--
2.25.0
^ permalink raw reply related
* [PATCH v3 6/7] powerpc/32s: Prepare allow_user_access() for user_access_begin()
From: Christophe Leroy @ 2020-01-23 12:59 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman
Cc: linux-fsdevel, linux-mm, linuxppc-dev, linux-kernel
In-Reply-To: <fed4f49349913cb6739dac647ba6a61d56b989d2.1579783936.git.christophe.leroy@c-s.fr>
In preparation of implementing user_access_begin and friends
on powerpc, allow_user_access() need to be prepared for
user_access_begin()
user_access_end() doesn't provide the address and size which
were passed to user_access_begin(), required by prevent_user_access()
to know which segment to modify. But user_access_end() takes an
opaque value returned by user_access_begin().
Make allow_user_access() return the value it writes to current->kuap.
This will allow user_access_end() to recalculate the segment range
without having to read current->kuap.
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
v3: Replaces the patch "Prepare prevent_user_access() for user_access_end()"
---
arch/powerpc/include/asm/book3s/32/kup.h | 15 ++++++++++-----
arch/powerpc/include/asm/book3s/64/kup-radix.h | 7 +++++--
arch/powerpc/include/asm/kup.h | 8 ++++++--
arch/powerpc/include/asm/nohash/32/kup-8xx.h | 6 ++++--
4 files changed, 25 insertions(+), 11 deletions(-)
diff --git a/arch/powerpc/include/asm/book3s/32/kup.h b/arch/powerpc/include/asm/book3s/32/kup.h
index 3c1798e56b55..128dcbf3a19d 100644
--- a/arch/powerpc/include/asm/book3s/32/kup.h
+++ b/arch/powerpc/include/asm/book3s/32/kup.h
@@ -102,23 +102,28 @@ static inline void kuap_update_sr(u32 sr, u32 addr, u32 end)
isync(); /* Context sync required after mtsrin() */
}
-static __always_inline void allow_user_access(void __user *to, const void __user *from,
- u32 size, unsigned long dir)
+/* Make sure we never return 0. We only use top and bottom 4 bits */
+static __always_inline unsigned long
+allow_user_access(void __user *to, const void __user *from, u32 size, unsigned long dir)
{
u32 addr, end;
+ unsigned long kuap;
BUILD_BUG_ON(!__builtin_constant_p(dir));
if (!(dir & KUAP_W))
- return;
+ return 0x100;
addr = (__force u32)to;
if (unlikely(addr >= TASK_SIZE || !size))
- return;
+ return 0x100;
end = min(addr + size, TASK_SIZE);
- current->thread.kuap = (addr & 0xf0000000) | ((((end - 1) >> 28) + 1) & 0xf);
+ kuap = (addr & 0xf0000000) | ((((end - 1) >> 28) + 1) & 0xf);
+ current->thread.kuap = kuap;
kuap_update_sr(mfsrin(addr) & ~SR_KS, addr, end); /* Clear Ks */
+
+ return kuap;
}
static __always_inline void prevent_user_access(void __user *to, const void __user *from,
diff --git a/arch/powerpc/include/asm/book3s/64/kup-radix.h b/arch/powerpc/include/asm/book3s/64/kup-radix.h
index f11315306d41..183f0c87017b 100644
--- a/arch/powerpc/include/asm/book3s/64/kup-radix.h
+++ b/arch/powerpc/include/asm/book3s/64/kup-radix.h
@@ -77,8 +77,9 @@ static inline void set_kuap(unsigned long value)
isync();
}
-static __always_inline void allow_user_access(void __user *to, const void __user *from,
- unsigned long size, unsigned long dir)
+static __always_inline unsigned long
+allow_user_access(void __user *to, const void __user *from,
+ unsigned long size, unsigned long dir)
{
// This is written so we can resolve to a single case at build time
BUILD_BUG_ON(!__builtin_constant_p(dir));
@@ -88,6 +89,8 @@ static __always_inline void allow_user_access(void __user *to, const void __user
set_kuap(AMR_KUAP_BLOCK_READ);
else
set_kuap(0);
+
+ return 1;
}
static inline void prevent_user_access(void __user *to, const void __user *from,
diff --git a/arch/powerpc/include/asm/kup.h b/arch/powerpc/include/asm/kup.h
index ff57bfcb88f7..691fec5afd4a 100644
--- a/arch/powerpc/include/asm/kup.h
+++ b/arch/powerpc/include/asm/kup.h
@@ -45,8 +45,12 @@ static inline void setup_kuep(bool disabled) { }
void setup_kuap(bool disabled);
#else
static inline void setup_kuap(bool disabled) { }
-static inline void allow_user_access(void __user *to, const void __user *from,
- unsigned long size, unsigned long dir) { }
+static inline unsigned long allow_user_access(void __user *to, const void __user *from,
+ unsigned long size, unsigned long dir)
+{
+ return 1;
+}
+
static inline void prevent_user_access(void __user *to, const void __user *from,
unsigned long size, unsigned long dir) { }
static inline bool
diff --git a/arch/powerpc/include/asm/nohash/32/kup-8xx.h b/arch/powerpc/include/asm/nohash/32/kup-8xx.h
index 1d70c80366fd..ee673d3c0ab6 100644
--- a/arch/powerpc/include/asm/nohash/32/kup-8xx.h
+++ b/arch/powerpc/include/asm/nohash/32/kup-8xx.h
@@ -34,10 +34,12 @@
#include <asm/reg.h>
-static inline void allow_user_access(void __user *to, const void __user *from,
- unsigned long size, unsigned long dir)
+static inline unsigned long allow_user_access(void __user *to, const void __user *from,
+ unsigned long size, unsigned long dir)
{
mtspr(SPRN_MD_AP, MD_APG_INIT);
+
+ return 1;
}
static inline void prevent_user_access(void __user *to, const void __user *from,
--
2.25.0
^ permalink raw reply related
* [PATCH v3 7/7] powerpc: Implement user_access_begin and friends
From: Christophe Leroy @ 2020-01-23 12:59 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman
Cc: linux-fsdevel, linux-mm, linuxppc-dev, linux-kernel
In-Reply-To: <fed4f49349913cb6739dac647ba6a61d56b989d2.1579783936.git.christophe.leroy@c-s.fr>
Today, when a function like strncpy_from_user() is called,
the userspace access protection is de-activated and re-activated
for every word read.
By implementing user_access_begin and friends, the protection
is de-activated at the beginning of the copy and re-activated at the
end.
Implement user_access_begin(), user_access_end() and
unsafe_get_user(), unsafe_put_user() and unsafe_copy_to_user()
For the time being, we keep user_access_save() and
user_access_restore() as nops.
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
v2: no change
v3: adapted to the new format of user_access_begin/end()
---
arch/powerpc/include/asm/uaccess.h | 100 ++++++++++++++++++++++++++---
1 file changed, 90 insertions(+), 10 deletions(-)
diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
index cafad1960e76..30204e80df1b 100644
--- a/arch/powerpc/include/asm/uaccess.h
+++ b/arch/powerpc/include/asm/uaccess.h
@@ -91,9 +91,14 @@ static inline int __access_ok(unsigned long addr, unsigned long size,
__put_user_check((__typeof__(*(ptr)))(x), (ptr), sizeof(*(ptr)))
#define __get_user(x, ptr) \
- __get_user_nocheck((x), (ptr), sizeof(*(ptr)))
+ __get_user_nocheck((x), (ptr), sizeof(*(ptr)), true)
#define __put_user(x, ptr) \
- __put_user_nocheck((__typeof__(*(ptr)))(x), (ptr), sizeof(*(ptr)))
+ __put_user_nocheck((__typeof__(*(ptr)))(x), (ptr), sizeof(*(ptr)), true)
+
+#define __get_user_allowed(x, ptr) \
+ __get_user_nocheck((x), (ptr), sizeof(*(ptr)), false)
+#define __put_user_allowed(x, ptr) \
+ __put_user_nocheck((__typeof__(*(ptr)))(x), (ptr), sizeof(*(ptr)), false)
#define __get_user_inatomic(x, ptr) \
__get_user_nosleep((x), (ptr), sizeof(*(ptr)))
@@ -138,10 +143,9 @@ extern long __put_user_bad(void);
: "r" (x), "b" (addr), "i" (-EFAULT), "0" (err))
#endif /* __powerpc64__ */
-#define __put_user_size(x, ptr, size, retval) \
+#define __put_user_size_allowed(x, ptr, size, retval) \
do { \
retval = 0; \
- allow_write_to_user(ptr, size); \
switch (size) { \
case 1: __put_user_asm(x, ptr, retval, "stb"); break; \
case 2: __put_user_asm(x, ptr, retval, "sth"); break; \
@@ -149,17 +153,26 @@ do { \
case 8: __put_user_asm2(x, ptr, retval); break; \
default: __put_user_bad(); \
} \
+} while (0)
+
+#define __put_user_size(x, ptr, size, retval) \
+do { \
+ allow_write_to_user(ptr, size); \
+ __put_user_size_allowed(x, ptr, size, retval); \
prevent_write_to_user(ptr, size); \
} while (0)
-#define __put_user_nocheck(x, ptr, size) \
+#define __put_user_nocheck(x, ptr, size, allow) \
({ \
long __pu_err; \
__typeof__(*(ptr)) __user *__pu_addr = (ptr); \
if (!is_kernel_addr((unsigned long)__pu_addr)) \
might_fault(); \
__chk_user_ptr(ptr); \
- __put_user_size((x), __pu_addr, (size), __pu_err); \
+ if (allow) \
+ __put_user_size((x), __pu_addr, (size), __pu_err); \
+ else \
+ __put_user_size_allowed((x), __pu_addr, (size), __pu_err); \
__pu_err; \
})
@@ -236,13 +249,12 @@ extern long __get_user_bad(void);
: "b" (addr), "i" (-EFAULT), "0" (err))
#endif /* __powerpc64__ */
-#define __get_user_size(x, ptr, size, retval) \
+#define __get_user_size_allowed(x, ptr, size, retval) \
do { \
retval = 0; \
__chk_user_ptr(ptr); \
if (size > sizeof(x)) \
(x) = __get_user_bad(); \
- allow_read_from_user(ptr, size); \
switch (size) { \
case 1: __get_user_asm(x, ptr, retval, "lbz"); break; \
case 2: __get_user_asm(x, ptr, retval, "lhz"); break; \
@@ -250,6 +262,12 @@ do { \
case 8: __get_user_asm2(x, ptr, retval); break; \
default: (x) = __get_user_bad(); \
} \
+} while (0)
+
+#define __get_user_size(x, ptr, size, retval) \
+do { \
+ allow_read_from_user(ptr, size); \
+ __get_user_size_allowed(x, ptr, size, retval); \
prevent_read_from_user(ptr, size); \
} while (0)
@@ -260,7 +278,7 @@ do { \
#define __long_type(x) \
__typeof__(__builtin_choose_expr(sizeof(x) > sizeof(0UL), 0ULL, 0UL))
-#define __get_user_nocheck(x, ptr, size) \
+#define __get_user_nocheck(x, ptr, size, allow) \
({ \
long __gu_err; \
__long_type(*(ptr)) __gu_val; \
@@ -269,7 +287,10 @@ do { \
if (!is_kernel_addr((unsigned long)__gu_addr)) \
might_fault(); \
barrier_nospec(); \
- __get_user_size(__gu_val, __gu_addr, (size), __gu_err); \
+ if (allow) \
+ __get_user_size(__gu_val, __gu_addr, (size), __gu_err); \
+ else \
+ __get_user_size_allowed(__gu_val, __gu_addr, (size), __gu_err); \
(x) = (__typeof__(*(ptr)))__gu_val; \
__gu_err; \
})
@@ -387,6 +408,34 @@ static inline unsigned long raw_copy_to_user(void __user *to,
return ret;
}
+static inline unsigned long
+raw_copy_to_user_allowed(void __user *to, const void *from, unsigned long n)
+{
+ unsigned long ret;
+ if (__builtin_constant_p(n) && (n) <= 8) {
+ ret = 1;
+
+ switch (n) {
+ case 1:
+ __put_user_size_allowed(*(u8 *)from, (u8 __user *)to, 1, ret);
+ break;
+ case 2:
+ __put_user_size_allowed(*(u16 *)from, (u16 __user *)to, 2, ret);
+ break;
+ case 4:
+ __put_user_size_allowed(*(u32 *)from, (u32 __user *)to, 4, ret);
+ break;
+ case 8:
+ __put_user_size_allowed(*(u64 *)from, (u64 __user *)to, 8, ret);
+ break;
+ }
+ if (ret == 0)
+ return 0;
+ }
+
+ return __copy_tofrom_user(to, (__force const void __user *)from, n);
+}
+
static __always_inline unsigned long __must_check
copy_to_user_mcsafe(void __user *to, const void *from, unsigned long n)
{
@@ -428,4 +477,35 @@ extern long __copy_from_user_flushcache(void *dst, const void __user *src,
extern void memcpy_page_flushcache(char *to, struct page *page, size_t offset,
size_t len);
+static __must_check inline unsigned long
+user_access_begin(const void __user *ptr, size_t len, bool write)
+{
+ if (unlikely(!access_ok(ptr, len)))
+ return 0;
+ return allow_user_access((void __user *)ptr, ptr, len, write ? KUAP_RW : KUAP_R);
+}
+#define user_access_begin user_access_begin
+
+static inline void user_access_end(unsigned long key)
+{
+ if (IS_ENABLED(CONFIG_PPC_BOOK3S_32)) {
+ void __user *ptr = (__force void __user *)(key & 0xf0000000);
+ u32 size = (key << 28) - (key & 0xf0000000);
+
+ prevent_user_access(ptr, ptr, size, key & 0xf000000f ? KUAP_RW : KUAP_R);
+ } else {
+ prevent_user_access(NULL, NULL, ~0UL, KUAP_RW);
+ }
+}
+#define user_access_end user_access_end
+
+static inline unsigned long user_access_save(void) { return 0UL; }
+static inline void user_access_restore(unsigned long flags) { }
+
+#define unsafe_op_wrap(op, err) do { if (unlikely(op)) goto err; } while (0)
+#define unsafe_get_user(x, p, e) unsafe_op_wrap(__get_user_allowed(x, p), e)
+#define unsafe_put_user(x, p, e) unsafe_op_wrap(__put_user_allowed(x, p), e)
+#define unsafe_copy_to_user(d, s, l, e) \
+ unsafe_op_wrap(raw_copy_to_user_allowed(d, s, l), e)
+
#endif /* _ARCH_POWERPC_UACCESS_H */
--
2.25.0
^ permalink raw reply related
* Re: [PATCH v3 2/7] uaccess: Tell user_access_begin() if it's for a write or not
From: Jani Nikula @ 2020-01-23 13:11 UTC (permalink / raw)
To: Christophe Leroy, Benjamin Herrenschmidt, Paul Mackerras,
Michael Ellerman, Linus Torvalds, Alexander Viro, Andrew Morton,
Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin
Cc: x86, linux-kernel, dri-devel, linux-mm, linux-fsdevel,
linuxppc-dev
In-Reply-To: <e11a8f0670251267f87e3114e0bdbacb1eb72980.1579783936.git.christophe.leroy@c-s.fr>
On Thu, 23 Jan 2020, Christophe Leroy <christophe.leroy@c-s.fr> wrote:
> On 32 bits powerPC (book3s/32), only write accesses to user are
> protected and there is no point spending time on unlocking for reads.
>
> On 64 bits powerpc (book3s/64 at least), access can be granted
> read only, write only or read/write.
>
> Add an argument to user_access_begin() to tell when it's for write and
> return an opaque key that will be used by user_access_end() to know
> what was done by user_access_begin().
IMHO an opaque key is a prime example of a case where the use of an
opaque typedef is warranted. Nobody needs to know or care it's
specifically an unsigned long.
BR,
Jani.
--
Jani Nikula, Intel Open Source Graphics Center
^ permalink raw reply
* Re: [PATCH v2 07/10] powerpc/configs/skiroot: Enable security features
From: Joel Stanley @ 2020-01-23 13:38 UTC (permalink / raw)
To: Michael Ellerman; +Cc: linuxppc-dev, Oliver O'Halloran, Daniel Axtens
In-Reply-To: <20200121043000.16212-7-mpe@ellerman.id.au>
On Tue, 21 Jan 2020 at 04:30, Michael Ellerman <mpe@ellerman.id.au> wrote:
>
> From: Joel Stanley <joel@jms.id.au>
>
> This turns on HARDENED_USERCOPY with HARDENED_USERCOPY_PAGESPAN, and
> FORTIFY_SOURCE.
>
> It also enables SECURITY_LOCKDOWN_LSM with _EARLY and
> LOCK_DOWN_KERNEL_FORCE_INTEGRITY options enabled. This still allows
> xmon to be used in read-only mode.
>
> MODULE_SIG is selected by lockdown, so it is still enabled.
>
> Signed-off-by: Joel Stanley <joel@jms.id.au>
> [mpe: Switch to lockdown integrity mode per oohal]
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
I did some testing and with change we break kexec. As it's critical
for this kernel to be able to kexec we need to set KEXEC_FILE=y if
we're setting FORCE_INTEGRITY=y.
I've tested your series with that modification made and userspace was
once again able to kexec (with -s).
Cheers,
Joel
> ---
> arch/powerpc/configs/skiroot_defconfig | 11 ++++++++++-
> 1 file changed, 10 insertions(+), 1 deletion(-)
>
> v2: Switch to lockdown integrity mode rather than confidentiality as noticed by
> dja and discussed with jms and oohal.
>
> diff --git a/arch/powerpc/configs/skiroot_defconfig b/arch/powerpc/configs/skiroot_defconfig
> index 24a210fe0049..93b478436a2b 100644
> --- a/arch/powerpc/configs/skiroot_defconfig
> +++ b/arch/powerpc/configs/skiroot_defconfig
> @@ -49,7 +49,6 @@ CONFIG_JUMP_LABEL=y
> CONFIG_STRICT_KERNEL_RWX=y
> CONFIG_MODULES=y
> CONFIG_MODULE_UNLOAD=y
> -CONFIG_MODULE_SIG=y
> CONFIG_MODULE_SIG_FORCE=y
> CONFIG_MODULE_SIG_SHA512=y
> CONFIG_PARTITION_ADVANCED=y
> @@ -272,6 +271,16 @@ CONFIG_NLS_ASCII=y
> CONFIG_NLS_ISO8859_1=y
> CONFIG_NLS_UTF8=y
> CONFIG_ENCRYPTED_KEYS=y
> +CONFIG_SECURITY=y
> +CONFIG_HARDENED_USERCOPY=y
> +# CONFIG_HARDENED_USERCOPY_FALLBACK is not set
> +CONFIG_HARDENED_USERCOPY_PAGESPAN=y
> +CONFIG_FORTIFY_SOURCE=y
> +CONFIG_SECURITY_LOCKDOWN_LSM=y
> +CONFIG_SECURITY_LOCKDOWN_LSM_EARLY=y
> +CONFIG_LOCK_DOWN_KERNEL_FORCE_INTEGRITY=y
> +# CONFIG_INTEGRITY is not set
> +CONFIG_LSM="yama,loadpin,safesetid,integrity"
> # CONFIG_CRYPTO_HW is not set
> CONFIG_CRC16=y
> CONFIG_CRC_ITU_T=y
> --
> 2.21.1
>
^ permalink raw reply
* Re: [PATCH] powerpc: drmem: avoid NULL pointer dereference when drmem is unavailable
From: Nathan Lynch @ 2020-01-23 15:56 UTC (permalink / raw)
To: Libor Pechacek
Cc: David Hildenbrand, Michal Suchanek, linux-kernel, Paul Mackerras,
Leonardo Bras, Thomas Gleixner, linuxppc-dev, Allison Randal
In-Reply-To: <20200116102758.GC25138@fm.suse.cz>
Hello and thanks for the patch.
Libor Pechacek <lpechacek@suse.cz> writes:
> In KVM guests drmem structure is only zero initialized. Trying to
> manipulate DLPAR parameters results in a crash in this environment.
I think this statement needs qualification. Unless I'm mistaken, this
happens only when you boot a guest without any hotpluggable memory
configured, and then try to add or remove memory.
> diff --git a/arch/powerpc/include/asm/drmem.h b/arch/powerpc/include/asm/drmem.h
> index 3d76e1c388c2..28c3d936fdf3 100644
> --- a/arch/powerpc/include/asm/drmem.h
> +++ b/arch/powerpc/include/asm/drmem.h
> @@ -27,12 +27,12 @@ struct drmem_lmb_info {
> extern struct drmem_lmb_info *drmem_info;
>
> #define for_each_drmem_lmb_in_range(lmb, start, end) \
> - for ((lmb) = (start); (lmb) <= (end); (lmb)++)
> + for ((lmb) = (start); (lmb) < (end); (lmb)++)
>
> #define for_each_drmem_lmb(lmb) \
> for_each_drmem_lmb_in_range((lmb), \
> &drmem_info->lmbs[0], \
> - &drmem_info->lmbs[drmem_info->n_lmbs - 1])
> + &drmem_info->lmbs[drmem_info->n_lmbs])
>
> /*
> * The of_drconf_cell_v1 struct defines the layout of the LMB data
> diff --git a/arch/powerpc/platforms/pseries/hotplug-memory.c b/arch/powerpc/platforms/pseries/hotplug-memory.c
> index c126b94d1943..4ea6af002e27 100644
> --- a/arch/powerpc/platforms/pseries/hotplug-memory.c
> +++ b/arch/powerpc/platforms/pseries/hotplug-memory.c
> @@ -236,9 +236,9 @@ static int get_lmb_range(u32 drc_index, int n_lmbs,
> if (!start)
> return -EINVAL;
>
> - end = &start[n_lmbs - 1];
> + end = &start[n_lmbs];
>
> - last_lmb = &drmem_info->lmbs[drmem_info->n_lmbs - 1];
> + last_lmb = &drmem_info->lmbs[drmem_info->n_lmbs];
> if (end > last_lmb)
> return -EINVAL;
Is this not undefined behavior? I'd rather do this in a way that does
not involve forming out-of-bounds pointers. Even if it's safe, naming
that pointer "last_lmb" now actively hinders understanding of the code;
it should be named "limit" or something.
Anyway, I confess I've had the following patch sitting unsubmitted due
to a combination of perceived low urgency and lack of time. I've
verified it addresses your reported problem but I need a day or two to
check that it doesn't regress memory add/remove on suitably configured
guests:
================================================================
powerpc/drmem: make drmem list traversal safe on non-drmem systems
A user has reported a crash when attempting to remove an LMB on a KVM
guest where the device tree lacks the nodes and properties
(ibm,dynamic-reconfiguration-memory etc) which allow the drmem code to
support dynamic memory removal:
pseries-hotplug-mem: Attempting to hot-remove 1 LMB(s)
Unable to handle kernel paging request for data at address 0x00000010
Faulting instruction address: 0xc0000000000f0a30
Oops: Kernel access of bad area, sig: 11 [#1]
[... regs etc ...]
NIP [c0000000000f0a30] dlpar_memory+0x510/0xb50
LR [c0000000000f09e8] dlpar_memory+0x4c8/0xb50
Call Trace:
[c0000001edf97ba0] [c0000000000f09e8] dlpar_memory+0x4c8/0xb50 (unreliable)
[c0000001edf97c20] [c0000000000e8390] handle_dlpar_errorlog+0x60/0x140
[c0000001edf97c90] [c0000000000e85e0] dlpar_store+0x170/0x490
[c0000001edf97d40] [c000000000cb0c90] kobj_attr_store+0x30/0x50
[c0000001edf97d60] [c000000000591598] sysfs_kf_write+0x68/0xa0
[c0000001edf97d80] [c00000000058fec4] kernfs_fop_write+0x104/0x270
[c0000001edf97dd0] [c0000000004a1078] sys_write+0x128/0x380
[c0000001edf97e30] [c00000000000b388] system_call+0x5c/0x70
Make for_each_drmem_lmb() safe for the case where the list of drmem
LMBs is unpopulated.
Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com>
1 file changed, 36 insertions(+), 7 deletions(-)
arch/powerpc/include/asm/drmem.h | 43 +++++++++++++++++++++++++++++++++-------
modified arch/powerpc/include/asm/drmem.h
@@ -20,19 +20,48 @@ struct drmem_lmb {
struct drmem_lmb_info {
struct drmem_lmb *lmbs;
- int n_lmbs;
+ unsigned int n_lmbs;
u32 lmb_size;
};
extern struct drmem_lmb_info *drmem_info;
-#define for_each_drmem_lmb_in_range(lmb, start, end) \
- for ((lmb) = (start); (lmb) <= (end); (lmb)++)
+static inline bool drmem_present(void)
+{
+ return drmem_info->lmbs != NULL;
+}
+
+static inline struct drmem_lmb *drmem_lmb_index(unsigned int index)
+{
+ if (!drmem_present())
+ return NULL;
-#define for_each_drmem_lmb(lmb) \
- for_each_drmem_lmb_in_range((lmb), \
- &drmem_info->lmbs[0], \
- &drmem_info->lmbs[drmem_info->n_lmbs - 1])
+ if (WARN_ON(index >= drmem_info->n_lmbs))
+ return NULL;
+
+ return &drmem_info->lmbs[index];
+}
+
+static inline struct drmem_lmb *drmem_first_lmb(void)
+{
+ return drmem_lmb_index(0);
+}
+
+static inline struct drmem_lmb *drmem_last_lmb(void)
+{
+ if (!drmem_present())
+ return NULL;
+
+ return drmem_lmb_index(drmem_info->n_lmbs - 1);
+}
+
+#define for_each_drmem_lmb(lmb) \
+ for ((lmb) = drmem_first_lmb(); \
+ (lmb) != NULL && (lmb) <= drmem_last_lmb(); \
+ (lmb)++)
+
+#define for_each_drmem_lmb_in_range(lmb, start, end) \
+ for ((lmb) = (start); (lmb) <= (end); (lmb)++)
/*
* The of_drconf_cell_v1 struct defines the layout of the LMB data
^ permalink raw reply
* Re: [PATCH] powerpc: drmem: avoid NULL pointer dereference when drmem is unavailable
From: Michal Suchánek @ 2020-01-23 16:10 UTC (permalink / raw)
To: Nathan Lynch
Cc: David Hildenbrand, Libor Pechacek, linux-kernel, Paul Mackerras,
Leonardo Bras, Thomas Gleixner, linuxppc-dev, Allison Randal
In-Reply-To: <87o8uudv51.fsf@linux.ibm.com>
On Thu, Jan 23, 2020 at 09:56:10AM -0600, Nathan Lynch wrote:
> Hello and thanks for the patch.
>
> Libor Pechacek <lpechacek@suse.cz> writes:
> > In KVM guests drmem structure is only zero initialized. Trying to
> > manipulate DLPAR parameters results in a crash in this environment.
>
> I think this statement needs qualification. Unless I'm mistaken, this
> happens only when you boot a guest without any hotpluggable memory
> configured, and then try to add or remove memory.
>
>
> > diff --git a/arch/powerpc/include/asm/drmem.h b/arch/powerpc/include/asm/drmem.h
> > index 3d76e1c388c2..28c3d936fdf3 100644
> > --- a/arch/powerpc/include/asm/drmem.h
> > +++ b/arch/powerpc/include/asm/drmem.h
> > @@ -27,12 +27,12 @@ struct drmem_lmb_info {
> > extern struct drmem_lmb_info *drmem_info;
> >
> > #define for_each_drmem_lmb_in_range(lmb, start, end) \
> > - for ((lmb) = (start); (lmb) <= (end); (lmb)++)
> > + for ((lmb) = (start); (lmb) < (end); (lmb)++)
> >
> > #define for_each_drmem_lmb(lmb) \
> > for_each_drmem_lmb_in_range((lmb), \
> > &drmem_info->lmbs[0], \
> > - &drmem_info->lmbs[drmem_info->n_lmbs - 1])
> > + &drmem_info->lmbs[drmem_info->n_lmbs])
> >
> > /*
> > * The of_drconf_cell_v1 struct defines the layout of the LMB data
> > diff --git a/arch/powerpc/platforms/pseries/hotplug-memory.c b/arch/powerpc/platforms/pseries/hotplug-memory.c
> > index c126b94d1943..4ea6af002e27 100644
> > --- a/arch/powerpc/platforms/pseries/hotplug-memory.c
> > +++ b/arch/powerpc/platforms/pseries/hotplug-memory.c
> > @@ -236,9 +236,9 @@ static int get_lmb_range(u32 drc_index, int n_lmbs,
> > if (!start)
> > return -EINVAL;
> >
> > - end = &start[n_lmbs - 1];
> > + end = &start[n_lmbs];
> >
> > - last_lmb = &drmem_info->lmbs[drmem_info->n_lmbs - 1];
> > + last_lmb = &drmem_info->lmbs[drmem_info->n_lmbs];
> > if (end > last_lmb)
> > return -EINVAL;
>
> Is this not undefined behavior? I'd rather do this in a way that does
> not involve forming out-of-bounds pointers. Even if it's safe, naming
> that pointer "last_lmb" now actively hinders understanding of the code;
> it should be named "limit" or something.
Indeed, the name might be misleading now.
However, the loop differes from anything else we have in the kernel.
The standard explicitly allows the pointer to point just after the last
element to allow expressing the iteration limit without danger of
overflow.
Thanks
Michal
^ permalink raw reply
* [PATCH v4] powerpc: use probe_user_read() and probe_user_write()
From: Christophe Leroy @ 2020-01-23 17:30 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman
Cc: linuxppc-dev, linux-kernel
Instead of opencoding, use probe_user_read() to failessly read
a user location and probe_user_write() for writing to user.
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
Link to v3: https://patchwork.ozlabs.org/patch/1026042/
v4: Reviving this patch after one year. Now probe_user_read/write() is in the kernel so patch 1 is gone.
v3: No change
v2: Using probe_user_read() instead of probe_user_address()
---
arch/powerpc/kernel/process.c | 12 +-----------
arch/powerpc/kvm/book3s_64_mmu_radix.c | 6 ++----
arch/powerpc/mm/fault.c | 6 +-----
arch/powerpc/oprofile/backtrace.c | 14 ++------------
arch/powerpc/perf/callchain.c | 20 +++-----------------
arch/powerpc/perf/core-book3s.c | 8 +-------
arch/powerpc/sysdev/fsl_pci.c | 10 ++++------
7 files changed, 14 insertions(+), 62 deletions(-)
diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index 4df94b6e2f32..79f2cb6ecf87 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -1264,16 +1264,6 @@ void show_user_instructions(struct pt_regs *regs)
pc = regs->nip - (NR_INSN_TO_PRINT * 3 / 4 * sizeof(int));
- /*
- * Make sure the NIP points at userspace, not kernel text/data or
- * elsewhere.
- */
- if (!__access_ok(pc, NR_INSN_TO_PRINT * sizeof(int), USER_DS)) {
- pr_info("%s[%d]: Bad NIP, not dumping instructions.\n",
- current->comm, current->pid);
- return;
- }
-
seq_buf_init(&s, buf, sizeof(buf));
while (n) {
@@ -1284,7 +1274,7 @@ void show_user_instructions(struct pt_regs *regs)
for (i = 0; i < 8 && n; i++, n--, pc += sizeof(int)) {
int instr;
- if (probe_kernel_address((const void *)pc, instr)) {
+ if (probe_user_read(&instr, (void __user *)pc, sizeof(instr))) {
seq_buf_printf(&s, "XXXXXXXX ");
continue;
}
diff --git a/arch/powerpc/kvm/book3s_64_mmu_radix.c b/arch/powerpc/kvm/book3s_64_mmu_radix.c
index da857c8ba6e4..231410dc9db4 100644
--- a/arch/powerpc/kvm/book3s_64_mmu_radix.c
+++ b/arch/powerpc/kvm/book3s_64_mmu_radix.c
@@ -63,12 +63,10 @@ unsigned long __kvmhv_copy_tofrom_guest_radix(int lpid, int pid,
}
isync();
- pagefault_disable();
if (is_load)
- ret = raw_copy_from_user(to, from, n);
+ ret = probe_user_read(to, (const void __user *)from, n);
else
- ret = raw_copy_to_user(to, from, n);
- pagefault_enable();
+ ret = probe_user_write((void __user *)to, from, n);
/* switch the pid first to avoid running host with unallocated pid */
if (quadrant == 1 && pid != old_pid)
diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
index b5047f9b5dec..9e119f98a725 100644
--- a/arch/powerpc/mm/fault.c
+++ b/arch/powerpc/mm/fault.c
@@ -279,12 +279,8 @@ static bool bad_stack_expansion(struct pt_regs *regs, unsigned long address,
if ((flags & FAULT_FLAG_WRITE) && (flags & FAULT_FLAG_USER) &&
access_ok(nip, sizeof(*nip))) {
unsigned int inst;
- int res;
- pagefault_disable();
- res = __get_user_inatomic(inst, nip);
- pagefault_enable();
- if (!res)
+ if (!probe_user_read(&inst, nip, sizeof(inst)))
return !store_updates_sp(inst);
*must_retry = true;
}
diff --git a/arch/powerpc/oprofile/backtrace.c b/arch/powerpc/oprofile/backtrace.c
index 43245f4a9bcb..2799b922f780 100644
--- a/arch/powerpc/oprofile/backtrace.c
+++ b/arch/powerpc/oprofile/backtrace.c
@@ -28,15 +28,12 @@ static unsigned int user_getsp32(unsigned int sp, int is_first)
unsigned int stack_frame[2];
void __user *p = compat_ptr(sp);
- if (!access_ok(p, sizeof(stack_frame)))
- return 0;
-
/*
* The most likely reason for this is that we returned -EFAULT,
* which means that we've done all that we can do from
* interrupt context.
*/
- if (__copy_from_user_inatomic(stack_frame, p, sizeof(stack_frame)))
+ if (probe_user_read(stack_frame, (void __user *)p, sizeof(stack_frame)))
return 0;
if (!is_first)
@@ -54,11 +51,7 @@ static unsigned long user_getsp64(unsigned long sp, int is_first)
{
unsigned long stack_frame[3];
- if (!access_ok((void __user *)sp, sizeof(stack_frame)))
- return 0;
-
- if (__copy_from_user_inatomic(stack_frame, (void __user *)sp,
- sizeof(stack_frame)))
+ if (probe_user_read(stack_frame, (void __user *)sp, sizeof(stack_frame)))
return 0;
if (!is_first)
@@ -103,7 +96,6 @@ void op_powerpc_backtrace(struct pt_regs * const regs, unsigned int depth)
first_frame = 0;
}
} else {
- pagefault_disable();
#ifdef CONFIG_PPC64
if (!is_32bit_task()) {
while (depth--) {
@@ -112,7 +104,6 @@ void op_powerpc_backtrace(struct pt_regs * const regs, unsigned int depth)
break;
first_frame = 0;
}
- pagefault_enable();
return;
}
#endif
@@ -123,6 +114,5 @@ void op_powerpc_backtrace(struct pt_regs * const regs, unsigned int depth)
break;
first_frame = 0;
}
- pagefault_enable();
}
}
diff --git a/arch/powerpc/perf/callchain.c b/arch/powerpc/perf/callchain.c
index 35d542515faf..cbc251981209 100644
--- a/arch/powerpc/perf/callchain.c
+++ b/arch/powerpc/perf/callchain.c
@@ -155,12 +155,8 @@ static int read_user_stack_64(unsigned long __user *ptr, unsigned long *ret)
((unsigned long)ptr & 7))
return -EFAULT;
- pagefault_disable();
- if (!__get_user_inatomic(*ret, ptr)) {
- pagefault_enable();
+ if (!probe_user_read(ret, ptr, sizeof(*ret)))
return 0;
- }
- pagefault_enable();
return read_user_stack_slow(ptr, ret, 8);
}
@@ -171,12 +167,8 @@ static int read_user_stack_32(unsigned int __user *ptr, unsigned int *ret)
((unsigned long)ptr & 3))
return -EFAULT;
- pagefault_disable();
- if (!__get_user_inatomic(*ret, ptr)) {
- pagefault_enable();
+ if (!probe_user_read(ret, ptr, sizeof(*ret)))
return 0;
- }
- pagefault_enable();
return read_user_stack_slow(ptr, ret, 4);
}
@@ -293,17 +285,11 @@ static void perf_callchain_user_64(struct perf_callchain_entry_ctx *entry,
*/
static int read_user_stack_32(unsigned int __user *ptr, unsigned int *ret)
{
- int rc;
-
if ((unsigned long)ptr > TASK_SIZE - sizeof(unsigned int) ||
((unsigned long)ptr & 3))
return -EFAULT;
- pagefault_disable();
- rc = __get_user_inatomic(*ret, ptr);
- pagefault_enable();
-
- return rc;
+ return probe_user_read(ret, ptr, sizeof(*ret));
}
static inline void perf_callchain_user_64(struct perf_callchain_entry_ctx *entry,
diff --git a/arch/powerpc/perf/core-book3s.c b/arch/powerpc/perf/core-book3s.c
index 48604625ab31..3086055bf681 100644
--- a/arch/powerpc/perf/core-book3s.c
+++ b/arch/powerpc/perf/core-book3s.c
@@ -415,7 +415,6 @@ static void power_pmu_sched_task(struct perf_event_context *ctx, bool sched_in)
static __u64 power_pmu_bhrb_to(u64 addr)
{
unsigned int instr;
- int ret;
__u64 target;
if (is_kernel_addr(addr)) {
@@ -426,13 +425,8 @@ static __u64 power_pmu_bhrb_to(u64 addr)
}
/* Userspace: need copy instruction here then translate it */
- pagefault_disable();
- ret = __get_user_inatomic(instr, (unsigned int __user *)addr);
- if (ret) {
- pagefault_enable();
+ if (probe_user_read(&instr, (unsigned int __user *)addr, sizeof(instr)))
return 0;
- }
- pagefault_enable();
target = branch_target(&instr);
if ((!target) || (instr & BRANCH_ABSOLUTE))
diff --git a/arch/powerpc/sysdev/fsl_pci.c b/arch/powerpc/sysdev/fsl_pci.c
index 617a443d673d..4a8874bc1057 100644
--- a/arch/powerpc/sysdev/fsl_pci.c
+++ b/arch/powerpc/sysdev/fsl_pci.c
@@ -1065,13 +1065,11 @@ int fsl_pci_mcheck_exception(struct pt_regs *regs)
addr += mfspr(SPRN_MCAR);
if (is_in_pci_mem_space(addr)) {
- if (user_mode(regs)) {
- pagefault_disable();
- ret = get_user(inst, (__u32 __user *)regs->nip);
- pagefault_enable();
- } else {
+ if (user_mode(regs))
+ ret = probe_user_read(&inst, (void __user *)regs->nip,
+ sizeof(inst));
+ else
ret = probe_kernel_address((void *)regs->nip, inst);
- }
if (!ret && mcheck_handle_load(regs, inst)) {
regs->nip += 4;
--
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