Re: [PATCH v2] hwmon: (ibmpowernv) refactor deprecated strncpy
Justin Stitt writes:
> `strncpy` is deprecated for use on NUL-terminated destination strings [1].
>
> A suitable replacement is `memcpy` as we've already precisely calculated
> the number of bytes to copy while `buf` has been explicitly
> zero-initialized:
> | char buf[8] = { 0 };
>
> Link:
> https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings
> [1]
> Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html
> [2]
> Link: https://github.com/KSPP/linux/issues/90
> Cc: linux-hardening@vger.kernel.org
> Signed-off-by: Justin Stitt
> ---
> Changes in v2:
> - prefer memcpy to strscpy (thanks Kees)
> - Link to v1:
> https://lore.kernel.org/r/20230914-strncpy-drivers-hwmon-ibmpowernv-c-v1-1-ba6b7f42c...@google.com
> ---
> drivers/hwmon/ibmpowernv.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Tested-by: Michael Ellerman
Acked-by: Michael Ellerman (powerpc)
cheers
Re: [PATCH] scsi: ibmvscsi: replace deprecated strncpy with strscpy
Justin Stitt writes: > strncpy() is deprecated for use on NUL-terminated destination strings > [1] and as such we should prefer more robust and less ambiguous string > interfaces. > > We expect partition_name to be NUL-terminated based on its usage with > format strings: > | dev_info(hostdata->dev, "host srp version: %s, " > |"host partition %s (%d), OS %d, max io %u\n", > |hostdata->madapter_info.srp_version, > |hostdata->madapter_info.partition_name, > |be32_to_cpu(hostdata->madapter_info.partition_number), > |be32_to_cpu(hostdata->madapter_info.os_type), > |be32_to_cpu(hostdata->madapter_info.port_max_txu[0])); > ... > | len = snprintf(buf, PAGE_SIZE, "%s\n", > |hostdata->madapter_info.partition_name); > > Moreover, NUL-padding is not required as madapter_info is explicitly > memset to 0: > | memset(&hostdata->madapter_info, 0x00, > | sizeof(hostdata->madapter_info)); > > Considering the above, a suitable replacement is `strscpy` [2] due to > the fact that it guarantees NUL-termination on the destination buffer > without unnecessarily NUL-padding. > > Link: > https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings > [1] > Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html > [2] > Link: https://github.com/KSPP/linux/issues/90 > Cc: linux-hardening@vger.kernel.org > Signed-off-by: Justin Stitt > --- > Note: build-tested only. I gave it a quick boot, no issues. Tested-by: Michael Ellerman (powerpc) cheers
Re: [PATCH 60/82] powerpc: Refactor intentional wrap-around test
Kees Cook writes: > In an effort to separate intentional arithmetic wrap-around from > unexpected wrap-around, we need to refactor places that depend on this > kind of math. One of the most common code patterns of this is: > > VAR + value < VAR > > Notably, this is considered "undefined behavior" for signed and pointer > types, which the kernel works around by using the -fno-strict-overflow > option in the build[1] (which used to just be -fwrapv). Regardless, we > want to get the kernel source to the position where we can meaningfully > instrument arithmetic wrap-around conditions and catch them when they > are unexpected, regardless of whether they are signed[2], unsigned[3], > or pointer[4] types. > > Refactor open-coded wrap-around addition test to use add_would_overflow(). > This paves the way to enabling the wrap-around sanitizers in the future. > > Link: https://git.kernel.org/linus/68df3755e383e6fecf2354a67b08f92f18536594 > [1] > Link: https://github.com/KSPP/linux/issues/26 [2] > Link: https://github.com/KSPP/linux/issues/27 [3] > Link: https://github.com/KSPP/linux/issues/344 [4] > Cc: Michael Ellerman > Cc: Nicholas Piggin > Cc: Christophe Leroy > Cc: "Aneesh Kumar K.V" > Cc: "Naveen N. Rao" > Cc: Mahesh Salgaonkar > Cc: Vasant Hegde > Cc: dingsenjie > Cc: linuxppc-...@lists.ozlabs.org > Cc: Aneesh Kumar K.V > Cc: Naveen N. Rao > Signed-off-by: Kees Cook > --- > arch/powerpc/platforms/powernv/opal-prd.c | 2 +- > arch/powerpc/xmon/xmon.c | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) Acked-by: Michael Ellerman (powerpc) cheers
Re: [PATCH] powerpc/pseries: Whitelist dtl slub object for copying to userspace
Kees Cook writes:
> On Fri, Jun 14, 2024 at 11:08:44PM +0530, Anjali K wrote:
>> Reading the dispatch trace log from /sys/kernel/debug/powerpc/dtl/cpu-*
>> results in a BUG() when the config CONFIG_HARDENED_USERCOPY is enabled as
>> shown below.
>>
>> kernel BUG at mm/usercopy.c:102!
>> Oops: Exception in kernel mode, sig: 5 [#1]
>> LE PAGE_SIZE=64K MMU=Radix SMP NR_CPUS=2048 NUMA pSeries
>> Modules linked in: xfs libcrc32c dm_service_time sd_mod t10_pi sg ibmvfc
>> scsi_transport_fc ibmveth pseries_wdt dm_multipath dm_mirror
>> dm_region_hash dm_log dm_mod fuse
>> CPU: 27 PID: 1815 Comm: python3 Not tainted 6.10.0-rc3 #85
>> Hardware name: IBM,9040-MRX POWER10 (raw) 0x800200 0xf06
>> of:IBM,FW1060.00 (NM1060_042) hv:phyp pSeries
>> NIP: c05d23d4 LR: c05d23d0 CTR: 006ee6f8
>> REGS: c00120c078c0 TRAP: 0700 Not tainted (6.10.0-rc3)
>> MSR: 80029033 CR: 2828220f XER:
>> 000e
>> CFAR: c01fdc80 IRQMASK: 0
>> [ ... GPRs omitted ... ]
>> NIP [c05d23d4] usercopy_abort+0x78/0xb0
>> LR [c05d23d0] usercopy_abort+0x74/0xb0
>> Call Trace:
>> usercopy_abort+0x74/0xb0 (unreliable)
>> __check_heap_object+0xf8/0x120
>> check_heap_object+0x218/0x240
>> __check_object_size+0x84/0x1a4
>> dtl_file_read+0x17c/0x2c4
>> full_proxy_read+0x8c/0x110
>> vfs_read+0xdc/0x3a0
>> ksys_read+0x84/0x144
>> system_call_exception+0x124/0x330
>> system_call_vectored_common+0x15c/0x2ec
>> --- interrupt: 3000 at 0x7fff81f3ab34
>>
>> Commit 6d07d1cd300f ("usercopy: Restrict non-usercopy caches to size 0")
>> requires that only whitelisted areas in slab/slub objects can be copied to
>> userspace when usercopy hardening is enabled using CONFIG_HARDENED_USERCOPY.
>> Dtl contains hypervisor dispatch events which are expected to be read by
>> privileged users. Hence mark this safe for user access.
>> Specify useroffset=0 and usersize=DISPATCH_LOG_BYTES to whitelist the
>> entire object.
>>
>> Co-developed-by: Vishal Chourasia
>> Signed-off-by: Vishal Chourasia
>> Signed-off-by: Anjali K
>> ---
>> arch/powerpc/platforms/pseries/setup.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/powerpc/platforms/pseries/setup.c
>> b/arch/powerpc/platforms/pseries/setup.c
>> index 284a6fa04b0c..cba40d9d1284 100644
>> --- a/arch/powerpc/platforms/pseries/setup.c
>> +++ b/arch/powerpc/platforms/pseries/setup.c
>> @@ -343,8 +343,8 @@ static int alloc_dispatch_log_kmem_cache(void)
>> {
>> void (*ctor)(void *) = get_dtl_cache_ctor();
>>
>> -dtl_cache = kmem_cache_create("dtl", DISPATCH_LOG_BYTES,
>> -DISPATCH_LOG_BYTES, 0, ctor);
>> +dtl_cache = kmem_cache_create_usercopy("dtl", DISPATCH_LOG_BYTES,
>> +DISPATCH_LOG_BYTES, 0, 0,
>> DISPATCH_LOG_BYTES, ctor);
>> if (!dtl_cache) {
>> pr_warn("Failed to create dispatch trace log buffer cache\n");
>> pr_warn("Stolen time statistics will be unreliable\n");
>
> Are you sure you want to universally expose this memory region? It
> sounds like it's only exposed via a debug interface. Maybe it'd be
> better to use a bounce buffer in the debug interface instead?
I'm not sure what the threat is?
The log entries are written by the hypervisor, but never read.
That kmem_cache is only used to allocate the array of dtl_entry structs,
the ring buffer itself (struct dtl) is allocated statically. So
overwriting the dtl_entries can't corrupt the structure of the ring
buffer, just the content.
An attacker could read the entries and see some kernel pointers, but
those are everywhere.
So it seems pretty harmless.
I guess there isn't a kmem_cache_create_user_readonly() ?
> diff --git a/arch/powerpc/platforms/pseries/dtl.c
> b/arch/powerpc/platforms/pseries/dtl.c
> index 3f1cdccebc9c..3adcff5cc4b2 100644
> --- a/arch/powerpc/platforms/pseries/dtl.c
> +++ b/arch/powerpc/platforms/pseries/dtl.c
> @@ -257,6 +257,22 @@ static int dtl_file_release(struct inode *inode, struct
> file *filp)
> return 0;
> }
>
> +static inline int bounce_copy(char __user *buf, void *src, size_t size)
> +{
> + u8 *bounce;
> + int rc;
> +
> + bounce = kmalloc(size, GFP_KERNEL);
> + if (!bounce)
> + return -ENOMEM;
> +
> + memcpy(bounce, src, size);
> + rc = copy_to_user(buf, bounce, size);
> +
> + kfree(bounce);
> + return rc;
> +}
Is there no generic version of that?
> @@ -300,7 +316,7 @@ static ssize_t dtl_file_read(struct file *filp, char
> __user *buf, size_t len,
> if (i + n_req > dtl->buf_entries) {
> read_size = dtl->buf_entries - i;
>
> - rc = copy_to_user(buf, &dtl->buf[i],
> + rc = bounce_copy(buf, &dtl->buf[i],
> read_size * sizeof(struct dtl_entry));
>
Re: [PATCH] powerpc/pseries: Whitelist dtl slub object for copying to userspace
Anjali K writes: > Hi Michael > > On 18/06/24 12:41, Michael Ellerman wrote: >> I guess there isn't a kmem_cache_create_user_readonly() ? > Thank you for your review. > > My understanding of the question is whether there's a way to whitelist a > region such that it can be copied to userspace, but not written to using > copy_from_user(). Yes that's what I meant, and I pretty much worked that out from looking at the implementation, but was hoping Kees would tell me it was there somewhere, or implement it :) Apologies for being cryptic. > No, we don't have a function to whitelist only for copy_to_user() and not > copy_from_user(). Yep. I'll take this patch as-is, I think we've established that it's pretty low risk to whitelist the whole cache. cheers
Re: [PATCH] powerpc/pseries: Whitelist dtl slub object for copying to userspace
On Fri, 14 Jun 2024 23:08:44 +0530, Anjali K wrote: > Reading the dispatch trace log from /sys/kernel/debug/powerpc/dtl/cpu-* > results in a BUG() when the config CONFIG_HARDENED_USERCOPY is enabled as > shown below. > > kernel BUG at mm/usercopy.c:102! > Oops: Exception in kernel mode, sig: 5 [#1] > LE PAGE_SIZE=64K MMU=Radix SMP NR_CPUS=2048 NUMA pSeries > Modules linked in: xfs libcrc32c dm_service_time sd_mod t10_pi sg ibmvfc > scsi_transport_fc ibmveth pseries_wdt dm_multipath dm_mirror > dm_region_hash dm_log dm_mod fuse > CPU: 27 PID: 1815 Comm: python3 Not tainted 6.10.0-rc3 #85 > Hardware name: IBM,9040-MRX POWER10 (raw) 0x800200 0xf06 > of:IBM,FW1060.00 (NM1060_042) hv:phyp pSeries > NIP: c05d23d4 LR: c05d23d0 CTR: 006ee6f8 > REGS: c00120c078c0 TRAP: 0700 Not tainted (6.10.0-rc3) > MSR: 80029033 CR: 2828220f XER: 000e > CFAR: c01fdc80 IRQMASK: 0 > [ ... GPRs omitted ... ] > NIP [c05d23d4] usercopy_abort+0x78/0xb0 > LR [c05d23d0] usercopy_abort+0x74/0xb0 > Call Trace: > usercopy_abort+0x74/0xb0 (unreliable) > __check_heap_object+0xf8/0x120 > check_heap_object+0x218/0x240 > __check_object_size+0x84/0x1a4 > dtl_file_read+0x17c/0x2c4 > full_proxy_read+0x8c/0x110 > vfs_read+0xdc/0x3a0 > ksys_read+0x84/0x144 > system_call_exception+0x124/0x330 > system_call_vectored_common+0x15c/0x2ec > --- interrupt: 3000 at 0x7fff81f3ab34 > > [...] Applied to powerpc/fixes. [1/1] powerpc/pseries: Whitelist dtl slub object for copying to userspace https://git.kernel.org/powerpc/c/1a14150e1656f7a332a943154fc486504db4d586 cheers
Re: [PATCH] powerpc: Add __must_check to set_memory_...()
Christophe Leroy writes:
> Hi Michael,
>
> Le 07/09/2024 à 17:40, Christophe Leroy a écrit :
>> After the following powerpc commits, all calls to set_memory_...()
>> functions check returned value.
>> - Commit 8f17bd2f4196 ("powerpc: Handle error in mark_rodata_ro() and
>> mark_initmem_nx()")
>> - Commit f7f18e30b468 ("powerpc/kprobes: Handle error returned by
>> set_memory_rox()")
>> - Commit 009cf11d4aab ("powerpc: Don't ignore errors from
>> set_memory_{n}p() in __kernel_map_pages()")
>> - Commit 9cbacb834b4a ("powerpc: Don't ignore errors from
>> set_memory_{n}p() in __kernel_map_pages()")
>> - Commit 78cb0945f714 ("powerpc: Handle error in mark_rodata_ro() and
>> mark_initmem_nx()")
>>
>> All calls in core parts of the kernel also always check returned value,
>> can be looked at with following query:
>>
>>$ git grep -w -e set_memory_ro -e set_memory_rw -e set_memory_x -e
>> set_memory_nx -e set_memory_rox `find . -maxdepth 1 -type d | grep -v arch |
>> grep /`
>>
>> It is now possible to flag those functions with __must_check to make
>> sure no new unchecked call it added.
>>
>> Link: https://github.com/KSPP/linux/issues/7
>> Signed-off-by: Christophe Leroy
>
> Do you plan to take this patch anytime soon ?
>
> The generic part of the same was already applied in previous cycle, see
> https://github.com/torvalds/linux/commit/82ce8e2f31a1eb05b1527c3d807bea40031df913
I was waiting for the generic part to land, sorry I missed it.
Will put this in next now.
cheers
Re: [PATCH][next] powerpc/ps3: replace open-coded sysfs_emit function
On Sat, 19 Oct 2024 15:13:49 +1300, Paulo Miguel Almeida wrote: > sysfs_emit() helper function should be used when formatting the value > to be returned to user space. > > This patch replaces open-coded sysfs_emit() in sysfs .show() callbacks > > Applied to powerpc/next. [1/1] powerpc/ps3: replace open-coded sysfs_emit function https://git.kernel.org/powerpc/c/2866949ec889cf383c481119c617b9cead733070 cheers
Re: [PATCH] powerpc: Add __must_check to set_memory_...()
On Sat, 07 Sep 2024 17:40:41 +0200, Christophe Leroy wrote:
> After the following powerpc commits, all calls to set_memory_...()
> functions check returned value.
> - Commit 8f17bd2f4196 ("powerpc: Handle error in mark_rodata_ro() and
> mark_initmem_nx()")
> - Commit f7f18e30b468 ("powerpc/kprobes: Handle error returned by
> set_memory_rox()")
> - Commit 009cf11d4aab ("powerpc: Don't ignore errors from
> set_memory_{n}p() in __kernel_map_pages()")
> - Commit 9cbacb834b4a ("powerpc: Don't ignore errors from
> set_memory_{n}p() in __kernel_map_pages()")
> - Commit 78cb0945f714 ("powerpc: Handle error in mark_rodata_ro() and
> mark_initmem_nx()")
>
> [...]
Applied to powerpc/next.
[1/1] powerpc: Add __must_check to set_memory_...()
https://git.kernel.org/powerpc/c/2abbd6d5fbe0eae3752b44c963248e19292e5104
cheers
