Re: [PATCH] drm/radeon: make -fstrict-flex-arrays=3 happy
On Mon, Apr 15, 2024 at 09:38:16AM -0400, Alex Deucher wrote:
> The driver parses a union where the layout up through the first
> array is the same, however, the array has different sizes
> depending on the elements in the union. Be explicit to
> fix the UBSAN checker.
>
> Closes: https://gitlab.freedesktop.org/drm/amd/-/issues/3323
> Fixes: df8fc4e934c1 ("kbuild: Enable -fstrict-flex-arrays=3")
> Signed-off-by: Alex Deucher
> Cc: Kees Cook
Yup, this looks correct to me. These were trailing arrays that were not
bounds checked prior to -fstrict-flex-arrays=3:
#define ATOM_DEVICE_DFP3_INDEX0x0009
...
#define ATOM_DEVICE_DFP5_INDEX0x000B
...
#define ATOM_DEVICE_RESERVEDF_INDEX 0x000F
...
#define ATOM_MAX_SUPPORTED_DEVICE_INFO
(ATOM_DEVICE_DFP3_INDEX+1)
...
#define ATOM_MAX_SUPPORTED_DEVICE
(ATOM_DEVICE_RESERVEDF_INDEX+1)
typedef struct _ATOM_SUPPORTED_DEVICES_INFO
...
ATOM_CONNECTOR_INFO_I2C asConnInfo[ATOM_MAX_SUPPORTED_DEVICE_INFO];
typedef struct _ATOM_SUPPORTED_DEVICES_INFO_2
...
ATOM_CONNECTOR_INFO_I2C asConnInfo[ATOM_MAX_SUPPORTED_DEVICE];
And these arrays had different sizes: 10 vs 16
Reviewed-by: Kees Cook
-Kees
> ---
> drivers/gpu/drm/radeon/radeon_atombios.c | 8 ++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/radeon/radeon_atombios.c
> b/drivers/gpu/drm/radeon/radeon_atombios.c
> index bb1f0a3371ab5..10793a433bf58 100644
> --- a/drivers/gpu/drm/radeon/radeon_atombios.c
> +++ b/drivers/gpu/drm/radeon/radeon_atombios.c
> @@ -923,8 +923,12 @@ bool
> radeon_get_atom_connector_info_from_supported_devices_table(struct
> max_device = ATOM_MAX_SUPPORTED_DEVICE_INFO;
>
> for (i = 0; i < max_device; i++) {
> - ATOM_CONNECTOR_INFO_I2C ci =
> - supported_devices->info.asConnInfo[i];
> + ATOM_CONNECTOR_INFO_I2C ci;
> +
> + if (frev > 1)
> + ci = supported_devices->info_2d1.asConnInfo[i];
> + else
> + ci = supported_devices->info.asConnInfo[i];
>
> bios_connectors[i].valid = false;
>
> --
> 2.44.0
>
--
Kees Cook
Re: [PATCH] drm/radeon: silence UBSAN warning (v3)
On Fri, Apr 12, 2024 at 10:28:19AM -0400, Alex Deucher wrote: > Convert a variable sized array from [1] to []. > > v2: fix up a few more. > v3: integrate comments from Kees. > > Tested-by: Jeff Johnson > Acked-by: Christian König (v1) > Signed-off-by: Alex Deucher LGTM! :) Reviewed-by: Kees Cook -- Kees Cook
Re: [PATCH] drm/radeon: silence UBSAN warning (v2)
On Mon, Apr 08, 2024 at 01:37:48PM -0400, Alex Deucher wrote:
> Convert a variable sized array from [1] to [].
>
> v2: fix up a few more.
>
> Acked-by: Christian König (v1)
> Signed-off-by: Alex Deucher
> ---
> drivers/gpu/drm/radeon/pptable.h | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/radeon/pptable.h
> b/drivers/gpu/drm/radeon/pptable.h
> index 94947229888ba..3493b1f979fed 100644
> --- a/drivers/gpu/drm/radeon/pptable.h
> +++ b/drivers/gpu/drm/radeon/pptable.h
> @@ -432,7 +432,7 @@ typedef struct _ATOM_PPLIB_STATE_V2
>/**
>* Driver will read the first ucNumDPMLevels in this array
>*/
> - UCHAR clockInfoIndex[1];
> + UCHAR clockInfoIndex[];
> } ATOM_PPLIB_STATE_V2;
The comment slightly above this hunk says:
//number of valid dpm levels in this state; Driver uses it to calculate
the whole
//size of the state: sizeof(ATOM_PPLIB_STATE_V2) + (ucNumDPMLevels - 1) *
sizeof(UCHAR)
This is wrong now, as ATOM_PPLIB_STATE_V2 isn't over-sized now. It
should be:
//size of the state: sizeof(ATOM_PPLIB_STATE_V2) + (ucNumDPMLevels) *
sizeof(UCHAR)
or better yet, struct_size(ATOM_PPLIB_STATE_V2, clockInfoIndex, ucNumDPMLevels)
I couldn't easily find any "sizeof" uses against these structs, but are
you sure there aren't size changes associated with this adjustment?
Also, if the comment is accurate, then clockInfoIndex can also gain a
__counted_by annotation:
UCHAR clockInfoIndex[] __counted_by(ucNumDPMLevels);
The use of __counted_by() seems like it could apply to the other changes
as well?
>
> typedef struct _StateArray{
This has a fake flex-array as well, but it's a flex array of flex
arrays. :(
typedef struct _StateArray{
//how many states we have
UCHAR ucNumEntries;
ATOM_PPLIB_STATE_V2 states[1];
}StateArray;
I suspect get_state_entry_v2() may trip the runtime checking too, though
it's using a bunch of casted pointer math instead of direct array
accesses, so maybe it's avoid the instrumentation for now?
> @@ -450,7 +450,7 @@ typedef struct _ClockInfoArray{
> //sizeof(ATOM_PPLIB_CLOCK_INFO)
> UCHAR ucEntrySize;
>
> -UCHAR clockInfo[1];
> +UCHAR clockInfo[];
> }ClockInfoArray;
>
> typedef struct _NonClockInfoArray{
> @@ -460,7 +460,7 @@ typedef struct _NonClockInfoArray{
> //sizeof(ATOM_PPLIB_NONCLOCK_INFO)
> UCHAR ucEntrySize;
>
> -ATOM_PPLIB_NONCLOCK_INFO nonClockInfo[1];
> +ATOM_PPLIB_NONCLOCK_INFO nonClockInfo[];
> }NonClockInfoArray;
>
> typedef struct _ATOM_PPLIB_Clock_Voltage_Dependency_Record
-Kees
--
Kees Cook
Re: 6.5.5: UBSAN: radeon_atombios.c: index 1 is out of range for type 'UCHAR [1]'
On April 8, 2024 5:45:29 PM PDT, Jeff Johnson wrote: >On 10/1/23 17:12, Justin Piszcz wrote: >>>> >>>> [Sun Oct 1 15:59:04 2023] UBSAN: array-index-out-of-bounds in >>>> drivers/gpu/drm/radeon/radeon_atombios.c:2620:43 >>>> [Sun Oct 1 15:59:04 2023] index 1 is out of range for type 'UCHAR [1]' >>>> [Sun Oct 1 15:59:04 2023] CPU: 5 PID: 1 Comm: swapper/0 Tainted: G >>>> T 6.5.5 #13 55df8de52754ef95effc50a55e9206abdea304ac >>>> [Sun Oct 1 15:59:04 2023] Hardware name: Supermicro X9SRL-F/X9SRL-F, >>>> BIOS 3.3 11/13/2018 >>>> [Sun Oct 1 15:59:04 2023] Call Trace: >>>> [Sun Oct 1 15:59:04 2023] >>>> [Sun Oct 1 15:59:04 2023] dump_stack_lvl+0x36/0x50 >>>> [Sun Oct 1 15:59:04 2023] __ubsan_handle_out_of_bounds+0xc7/0x110 >>>> [Sun Oct 1 15:59:04 2023] radeon_atombios_get_power_modes+0x87a/0x8f0 >>>> [Sun Oct 1 15:59:04 2023] radeon_pm_init+0x13a/0x7e0 >>>> [Sun Oct 1 15:59:04 2023] evergreen_init+0x13d/0x3d0 >>>> [Sun Oct 1 15:59:04 2023] radeon_device_init+0x60a/0xbf0 >>>> [Sun Oct 1 15:59:04 2023] radeon_driver_load_kms+0xb1/0x250 >>>> [Sun Oct 1 15:59:04 2023] drm_dev_register+0xfc/0x250 >>>> [Sun Oct 1 15:59:04 2023] radeon_pci_probe+0xd0/0x150 >>>> [Sun Oct 1 15:59:04 2023] pci_device_probe+0x97/0x130 >>>> [Sun Oct 1 15:59:04 2023] really_probe+0xbe/0x2f0 >>>> [Sun Oct 1 15:59:04 2023] ? __pfx___driver_attach+0x10/0x10 >>>> [Sun Oct 1 15:59:04 2023] __driver_probe_device+0x6e/0x120 >>>> [Sun Oct 1 15:59:04 2023] driver_probe_device+0x1a/0x90 >>>> [Sun Oct 1 15:59:04 2023] __driver_attach+0xd4/0x170 >>>> [Sun Oct 1 15:59:04 2023] bus_for_each_dev+0x87/0xe0 >>>> [Sun Oct 1 15:59:04 2023] bus_add_driver+0xf3/0x1f0 >>>> [Sun Oct 1 15:59:04 2023] driver_register+0x58/0x120 >>>> [Sun Oct 1 15:59:04 2023] ? __pfx_radeon_module_init+0x10/0x10 >>>> [Sun Oct 1 15:59:04 2023] do_one_initcall+0x93/0x4a0 >>>> [Sun Oct 1 15:59:04 2023] kernel_init_freeable+0x301/0x580 >>>> [Sun Oct 1 15:59:04 2023] ? __pfx_kernel_init+0x10/0x10 >>>> [Sun Oct 1 15:59:04 2023] kernel_init+0x15/0x1b0 >>>> [Sun Oct 1 15:59:04 2023] ret_from_fork+0x2f/0x50 >>>> [Sun Oct 1 15:59:04 2023] ? __pfx_kernel_init+0x10/0x10 >>>> [Sun Oct 1 15:59:04 2023] ret_from_fork_asm+0x1b/0x30 >>>> [Sun Oct 1 15:59:04 2023] >>>> [Sun Oct 1 15:59:04 2023] >>>> >>>> [Sun Oct 1 15:59:04 2023] [drm] radeon: dpm initialized >>>> [Sun Oct 1 15:59:04 2023] [drm] GART: num cpu pages 262144, num gpu >>>> pages 262144 >>>> [Sun Oct 1 15:59:04 2023] [drm] enabling PCIE gen 2 link speeds, >>>> disable with radeon.pcie_gen2=0 >>>> [Sun Oct 1 15:59:04 2023] [drm] PCIE GART of 1024M enabled (table at >>>> 0x0014C000). >>>> [Sun Oct 1 15:59:04 2023] radeon :03:00.0: WB enabled >>>> [Sun Oct 1 15:59:04 2023] radeon :03:00.0: fence driver on ring 0 >>>> use gpu addr 0x4c00 >>>> [Sun Oct 1 15:59:04 2023] radeon :03:00.0: fence driver on ring 3 >>>> use gpu addr 0x4c0c >>>> [Sun Oct 1 15:59:04 2023] radeon :03:00.0: fence driver on ring 5 >>>> use gpu addr 0x0005c418 >>>> [Sun Oct 1 15:59:04 2023] radeon :03:00.0: radeon: MSI limited to >>>> 32-bit >>>> [Sun Oct 1 15:59:04 2023] radeon :03:00.0: radeon: using MSI. >>>> [Sun Oct 1 15:59:04 2023] [drm] radeon: irq initialized. >>>> >>> >>> Please also open an issue on freedesktop tracker [1]. >>> >>> Thanks. >>> >>> [1]: https://gitlab.freedesktop.org/drm/amd/-/issues >> >> Issue opened: https://gitlab.freedesktop.org/drm/amd/-/issues/2894 >> >> Regards, >> Justin > >+Kees since I've worked with him on several of these flexible array issues. > >I just happened to look at kernel logs today for my ath1*k driver maintenance >and see the subject issue is present on my device, running 6.9.0-rc1. The >freedesktop issue tracker says the issue is closed, but any fix has not landed >in the upstream kernel. Is there a -next patch somewhere? > >[ 12.105270] UBSAN: array-index-out-of-bounds in >drivers/gpu/drm/radeon/radeon_atombios.c:2718:34 >[ 12.105272] index 48 is out of range for type 'UCHAR [1]' >[ > >If there isn't really an upstream fix, I can probably supply one. I would expect this to have fixed it: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/gpu/drm/radeon/pptable.h?id=c63079c61177ba1b17fa05c6875699a36924fe39 If not, there must be something else happening? -Kees -- Kees Cook
Re: [PATCH v2] drm/amd/display: Add NULL test for 'timing generator' in 'dcn21_set_pipe()'
On Thu, Feb 01, 2024 at 03:28:45PM +0530, Srinivasan Shanmugam wrote:
> In "u32 otg_inst = pipe_ctx->stream_res.tg->inst;"
> pipe_ctx->stream_res.tg could be NULL, it is relying on the caller to
> ensure the tg is not NULL.
>
> Fixes: 474ac4a875ca ("drm/amd/display: Implement some asic specific abm call
> backs.")
> Cc: Yongqiang Sun
> Cc: Anthony Koo
> Cc: Rodrigo Siqueira
> Cc: Aurabindo Pillai
> Signed-off-by: Srinivasan Shanmugam
> ---
> v2:
> - s/u32/uint32_t for consistency (Anthony)
>
> .../amd/display/dc/hwss/dcn21/dcn21_hwseq.c | 24 +++
> 1 file changed, 14 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/display/dc/hwss/dcn21/dcn21_hwseq.c
> b/drivers/gpu/drm/amd/display/dc/hwss/dcn21/dcn21_hwseq.c
> index 8e88dcaf88f5..8323077bba15 100644
> --- a/drivers/gpu/drm/amd/display/dc/hwss/dcn21/dcn21_hwseq.c
> +++ b/drivers/gpu/drm/amd/display/dc/hwss/dcn21/dcn21_hwseq.c
> @@ -206,28 +206,32 @@ void dcn21_set_abm_immediate_disable(struct pipe_ctx
> *pipe_ctx)
> void dcn21_set_pipe(struct pipe_ctx *pipe_ctx)
> {
> struct abm *abm = pipe_ctx->stream_res.abm;
> - uint32_t otg_inst = pipe_ctx->stream_res.tg->inst;
> + struct timing_generator *tg = pipe_ctx->stream_res.tg;
> struct panel_cntl *panel_cntl = pipe_ctx->stream->link->panel_cntl;
> struct dmcu *dmcu = pipe_ctx->stream->ctx->dc->res_pool->dmcu;
> + uint32_t otg_inst;
> +
> + if (!abm && !tg && !panel_cntl)
> + return;
> +
> + otg_inst = tg->inst;
Is the "if" supposed to be using "||"s instead of "&&"s? I noticed
Coverity complained "tg may be NULL" for the "tg->inst" dereference...
-Kees
--
Kees Cook
Re: [PATCH 0/3] Update LLVM Phabricator and Bugzilla links
On Tue, Jan 09, 2024 at 03:16:28PM -0700, Nathan Chancellor wrote: > This series updates all instances of LLVM Phabricator and Bugzilla links > to point to GitHub commits directly and LLVM's Bugzilla to GitHub issue > shortlinks respectively. > > I split up the Phabricator patch into BPF selftests and the rest of the > kernel in case the BPF folks want to take it separately from the rest of > the series, there are obviously no dependency issues in that case. The > Bugzilla change was mechanical enough and should have no conflicts. > > I am aiming this at Andrew and CC'ing other lists, in case maintainers > want to chime in, but I think this is pretty uncontroversial (famous > last words...). > > --- > Nathan Chancellor (3): > selftests/bpf: Update LLVM Phabricator links > arch and include: Update LLVM Phabricator links > treewide: Update LLVM Bugzilla links > > arch/arm64/Kconfig | 4 +-- > arch/powerpc/Makefile | 4 +-- > arch/powerpc/kvm/book3s_hv_nested.c| 2 +- > arch/riscv/Kconfig | 2 +- > arch/riscv/include/asm/ftrace.h| 2 +- > arch/s390/include/asm/ftrace.h | 2 +- > arch/x86/power/Makefile| 2 +- > crypto/blake2b_generic.c | 2 +- > drivers/firmware/efi/libstub/Makefile | 2 +- > drivers/gpu/drm/amd/amdgpu/sdma_v4_4_2.c | 2 +- > drivers/media/test-drivers/vicodec/codec-fwht.c| 2 +- > drivers/regulator/Kconfig | 2 +- > include/asm-generic/vmlinux.lds.h | 2 +- > include/linux/compiler-clang.h | 2 +- > lib/Kconfig.kasan | 2 +- > lib/raid6/Makefile | 2 +- > lib/stackinit_kunit.c | 2 +- > mm/slab_common.c | 2 +- > net/bridge/br_multicast.c | 2 +- > security/Kconfig | 2 +- > tools/testing/selftests/bpf/README.rst | 32 > +++--- > tools/testing/selftests/bpf/prog_tests/xdpwall.c | 2 +- > .../selftests/bpf/progs/test_core_reloc_type_id.c | 2 +- > 23 files changed, 40 insertions(+), 40 deletions(-) > --- > base-commit: 0dd3ee31125508cd67f7e7172247f05b7fd1753a > change-id: 20240109-update-llvm-links-d03f9d649e1e > > Best regards, > -- > Nathan Chancellor > Excellent! Thanks for doing this. I spot checked a handful I was familiar with and everything looks good to me. Reviewed-by: Kees Cook -- Kees Cook
Re: [PATCH 0/9] drm: Annotate structs with __counted_by
On Thu, Oct 05, 2023 at 11:42:38AM +0200, Christian König wrote: > Am 02.10.23 um 20:22 schrieb Kees Cook: > > On Mon, Oct 02, 2023 at 08:11:41PM +0200, Christian König wrote: > > > Am 02.10.23 um 20:08 schrieb Kees Cook: > > > > On Mon, Oct 02, 2023 at 08:01:57PM +0200, Christian König wrote: > > > > > Am 02.10.23 um 18:53 schrieb Kees Cook: > > > > > > On Mon, Oct 02, 2023 at 11:06:19AM -0400, Alex Deucher wrote: > > > > > > > On Mon, Oct 2, 2023 at 5:20 AM Christian König > > > > > > > wrote: > > > > > > > > Am 29.09.23 um 21:33 schrieb Kees Cook: > > > > > > > > > On Fri, 22 Sep 2023 10:32:05 -0700, Kees Cook wrote: > > > > > > > > > > This is a batch of patches touching drm for preparing for > > > > > > > > > > the coming > > > > > > > > > > implementation by GCC and Clang of the __counted_by > > > > > > > > > > attribute. Flexible > > > > > > > > > > array members annotated with __counted_by can have their > > > > > > > > > > accesses > > > > > > > > > > bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS > > > > > > > > > > (for array > > > > > > > > > > indexing) and CONFIG_FORTIFY_SOURCE (for > > > > > > > > > > strcpy/memcpy-family functions). > > > > > > > > > > > > > > > > > > > > As found with Coccinelle[1], add __counted_by to structs > > > > > > > > > > that would > > > > > > > > > > benefit from the annotation. > > > > > > > > > > > > > > > > > > > > [...] > > > > > > > > > Since this got Acks, I figure I should carry it in my tree. > > > > > > > > > Let me know > > > > > > > > > if this should go via drm instead. > > > > > > > > > > > > > > > > > > Applied to for-next/hardening, thanks! > > > > > > > > > > > > > > > > > > [1/9] drm/amd/pm: Annotate struct > > > > > > > > > smu10_voltage_dependency_table with __counted_by > > > > > > > > > https://git.kernel.org/kees/c/a6046ac659d6 > > > > > > > > STOP! In a follow up discussion Alex and I figured out that > > > > > > > > this won't work. > > > > > > I'm so confused; from the discussion I saw that Alex said both > > > > > > instances > > > > > > were false positives? > > > > > > > > > > > > > > The value in the structure is byte swapped based on some > > > > > > > > firmware > > > > > > > > endianness which not necessary matches the CPU endianness. > > > > > > > SMU10 is APU only so the endianess of the SMU firmware and the CPU > > > > > > > will always match. > > > > > > Which I think is what is being said here? > > > > > > > > > > > > > > Please revert that one from going upstream if it's already on > > > > > > > > it's way. > > > > > > > > > > > > > > > > And because of those reasons I strongly think that patches like > > > > > > > > this > > > > > > > > should go through the DRM tree :) > > > > > > Sure, that's fine -- please let me know. It was others Acked/etc. > > > > > > Who > > > > > > should carry these patches? > > > > > Probably best if the relevant maintainer pick them up individually. > > > > > > > > > > Some of those structures are filled in by firmware/hardware and only > > > > > the > > > > > maintainers can judge if that value actually matches what the compiler > > > > > needs. > > > > > > > > > > We have cases where individual bits are used as flags or when the > > > > > size is > > > > > byte swapped etc... > > > > > > > > > > Even Alex and I didn't immediately say how and where that field is > > > > > actually > > > > > used and had to dig that up. That's where the confusion came from. > > > > Okay, I've dropped them all from my tree. Several had Acks/Reviews, so > > > > hopefully those can get picked up for the DRM tree? > > > I will pick those up to go through drm-misc-next. > > > > > > Going to ping maintainers once more when I'm not sure if stuff is correct > > > or > > > not. > > Sounds great; thanks! > > I wasn't 100% sure for the VC4 patch, but pushed the whole set to > drm-misc-next anyway. > > This also means that the patches are now auto merged into the drm-tip > integration branch and should any build or unit test go boom we should > notice immediately and can revert it pretty easily. Thanks very much; I'll keep an eye out for any reports. -- Kees Cook
[PATCH] drm/amdgpu: Annotate struct amdgpu_bo_list with __counted_by
Prepare for the coming implementation by GCC and Clang of the __counted_by
attribute. Flexible array members annotated with __counted_by can have
their accesses bounds-checked at run-time via CONFIG_UBSAN_BOUNDS (for
array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family
functions).
As found with Coccinelle[1], add __counted_by for struct amdgpu_bo_list.
Additionally, since the element count member must be set before accessing
the annotated flexible array member, move its initialization earlier.
Cc: Alex Deucher
Cc: "Christian König"
Cc: "Pan, Xinhui"
Cc: David Airlie
Cc: Daniel Vetter
Cc: "Gustavo A. R. Silva"
Cc: Luben Tuikov
Cc: Christophe JAILLET
Cc: Felix Kuehling
Cc: amd-gfx@lists.freedesktop.org
Cc: dri-de...@lists.freedesktop.org
Cc: linux-harden...@vger.kernel.org
Link:
https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci
[1]
Signed-off-by: Kees Cook
---
drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c | 2 +-
drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.h | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c
index 6f5b641b631e..781e5c5ce04d 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c
@@ -84,6 +84,7 @@ int amdgpu_bo_list_create(struct amdgpu_device *adev, struct
drm_file *filp,
kref_init(>refcount);
+ list->num_entries = num_entries;
array = list->entries;
for (i = 0; i < num_entries; ++i) {
@@ -129,7 +130,6 @@ int amdgpu_bo_list_create(struct amdgpu_device *adev,
struct drm_file *filp,
}
list->first_userptr = first_userptr;
- list->num_entries = num_entries;
sort(array, last_entry, sizeof(struct amdgpu_bo_list_entry),
amdgpu_bo_list_entry_cmp, NULL);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.h
b/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.h
index 6a703be45d04..555cd6d877c3 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.h
@@ -56,7 +56,7 @@ struct amdgpu_bo_list {
*/
struct mutex bo_list_mutex;
- struct amdgpu_bo_list_entry entries[];
+ struct amdgpu_bo_list_entry entries[] __counted_by(num_entries);
};
int amdgpu_bo_list_get(struct amdgpu_fpriv *fpriv, int id,
--
2.34.1
Re: [PATCH 0/9] drm: Annotate structs with __counted_by
On Mon, Oct 02, 2023 at 08:11:41PM +0200, Christian König wrote: > Am 02.10.23 um 20:08 schrieb Kees Cook: > > On Mon, Oct 02, 2023 at 08:01:57PM +0200, Christian König wrote: > > > Am 02.10.23 um 18:53 schrieb Kees Cook: > > > > On Mon, Oct 02, 2023 at 11:06:19AM -0400, Alex Deucher wrote: > > > > > On Mon, Oct 2, 2023 at 5:20 AM Christian König > > > > > wrote: > > > > > > Am 29.09.23 um 21:33 schrieb Kees Cook: > > > > > > > On Fri, 22 Sep 2023 10:32:05 -0700, Kees Cook wrote: > > > > > > > > This is a batch of patches touching drm for preparing for the > > > > > > > > coming > > > > > > > > implementation by GCC and Clang of the __counted_by attribute. > > > > > > > > Flexible > > > > > > > > array members annotated with __counted_by can have their > > > > > > > > accesses > > > > > > > > bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS > > > > > > > > (for array > > > > > > > > indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family > > > > > > > > functions). > > > > > > > > > > > > > > > > As found with Coccinelle[1], add __counted_by to structs that > > > > > > > > would > > > > > > > > benefit from the annotation. > > > > > > > > > > > > > > > > [...] > > > > > > > Since this got Acks, I figure I should carry it in my tree. Let > > > > > > > me know > > > > > > > if this should go via drm instead. > > > > > > > > > > > > > > Applied to for-next/hardening, thanks! > > > > > > > > > > > > > > [1/9] drm/amd/pm: Annotate struct smu10_voltage_dependency_table > > > > > > > with __counted_by > > > > > > > https://git.kernel.org/kees/c/a6046ac659d6 > > > > > > STOP! In a follow up discussion Alex and I figured out that this > > > > > > won't work. > > > > I'm so confused; from the discussion I saw that Alex said both instances > > > > were false positives? > > > > > > > > > > The value in the structure is byte swapped based on some firmware > > > > > > endianness which not necessary matches the CPU endianness. > > > > > SMU10 is APU only so the endianess of the SMU firmware and the CPU > > > > > will always match. > > > > Which I think is what is being said here? > > > > > > > > > > Please revert that one from going upstream if it's already on it's > > > > > > way. > > > > > > > > > > > > And because of those reasons I strongly think that patches like this > > > > > > should go through the DRM tree :) > > > > Sure, that's fine -- please let me know. It was others Acked/etc. Who > > > > should carry these patches? > > > Probably best if the relevant maintainer pick them up individually. > > > > > > Some of those structures are filled in by firmware/hardware and only the > > > maintainers can judge if that value actually matches what the compiler > > > needs. > > > > > > We have cases where individual bits are used as flags or when the size is > > > byte swapped etc... > > > > > > Even Alex and I didn't immediately say how and where that field is > > > actually > > > used and had to dig that up. That's where the confusion came from. > > Okay, I've dropped them all from my tree. Several had Acks/Reviews, so > > hopefully those can get picked up for the DRM tree? > > I will pick those up to go through drm-misc-next. > > Going to ping maintainers once more when I'm not sure if stuff is correct or > not. Sounds great; thanks! -Kees -- Kees Cook
Re: [PATCH 0/9] drm: Annotate structs with __counted_by
On Mon, Oct 02, 2023 at 08:01:57PM +0200, Christian König wrote: > Am 02.10.23 um 18:53 schrieb Kees Cook: > > On Mon, Oct 02, 2023 at 11:06:19AM -0400, Alex Deucher wrote: > > > On Mon, Oct 2, 2023 at 5:20 AM Christian König > > > wrote: > > > > Am 29.09.23 um 21:33 schrieb Kees Cook: > > > > > On Fri, 22 Sep 2023 10:32:05 -0700, Kees Cook wrote: > > > > > > This is a batch of patches touching drm for preparing for the coming > > > > > > implementation by GCC and Clang of the __counted_by attribute. > > > > > > Flexible > > > > > > array members annotated with __counted_by can have their accesses > > > > > > bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS (for > > > > > > array > > > > > > indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family > > > > > > functions). > > > > > > > > > > > > As found with Coccinelle[1], add __counted_by to structs that would > > > > > > benefit from the annotation. > > > > > > > > > > > > [...] > > > > > Since this got Acks, I figure I should carry it in my tree. Let me > > > > > know > > > > > if this should go via drm instead. > > > > > > > > > > Applied to for-next/hardening, thanks! > > > > > > > > > > [1/9] drm/amd/pm: Annotate struct smu10_voltage_dependency_table with > > > > > __counted_by > > > > > https://git.kernel.org/kees/c/a6046ac659d6 > > > > STOP! In a follow up discussion Alex and I figured out that this won't > > > > work. > > I'm so confused; from the discussion I saw that Alex said both instances > > were false positives? > > > > > > The value in the structure is byte swapped based on some firmware > > > > endianness which not necessary matches the CPU endianness. > > > SMU10 is APU only so the endianess of the SMU firmware and the CPU > > > will always match. > > Which I think is what is being said here? > > > > > > Please revert that one from going upstream if it's already on it's way. > > > > > > > > And because of those reasons I strongly think that patches like this > > > > should go through the DRM tree :) > > Sure, that's fine -- please let me know. It was others Acked/etc. Who > > should carry these patches? > > Probably best if the relevant maintainer pick them up individually. > > Some of those structures are filled in by firmware/hardware and only the > maintainers can judge if that value actually matches what the compiler > needs. > > We have cases where individual bits are used as flags or when the size is > byte swapped etc... > > Even Alex and I didn't immediately say how and where that field is actually > used and had to dig that up. That's where the confusion came from. Okay, I've dropped them all from my tree. Several had Acks/Reviews, so hopefully those can get picked up for the DRM tree? Thanks! -Kees -- Kees Cook
Re: [PATCH 0/9] drm: Annotate structs with __counted_by
On Mon, Oct 02, 2023 at 11:06:19AM -0400, Alex Deucher wrote: > On Mon, Oct 2, 2023 at 5:20 AM Christian König > wrote: > > > > Am 29.09.23 um 21:33 schrieb Kees Cook: > > > On Fri, 22 Sep 2023 10:32:05 -0700, Kees Cook wrote: > > >> This is a batch of patches touching drm for preparing for the coming > > >> implementation by GCC and Clang of the __counted_by attribute. Flexible > > >> array members annotated with __counted_by can have their accesses > > >> bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS (for array > > >> indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family functions). > > >> > > >> As found with Coccinelle[1], add __counted_by to structs that would > > >> benefit from the annotation. > > >> > > >> [...] > > > Since this got Acks, I figure I should carry it in my tree. Let me know > > > if this should go via drm instead. > > > > > > Applied to for-next/hardening, thanks! > > > > > > [1/9] drm/amd/pm: Annotate struct smu10_voltage_dependency_table with > > > __counted_by > > >https://git.kernel.org/kees/c/a6046ac659d6 > > > > STOP! In a follow up discussion Alex and I figured out that this won't work. I'm so confused; from the discussion I saw that Alex said both instances were false positives? > > > > The value in the structure is byte swapped based on some firmware > > endianness which not necessary matches the CPU endianness. > > SMU10 is APU only so the endianess of the SMU firmware and the CPU > will always match. Which I think is what is being said here? > > Please revert that one from going upstream if it's already on it's way. > > > > And because of those reasons I strongly think that patches like this > > should go through the DRM tree :) Sure, that's fine -- please let me know. It was others Acked/etc. Who should carry these patches? Thanks! -Kees > > > > Regards, > > Christian. > > > > > [2/9] drm/amdgpu/discovery: Annotate struct ip_hw_instance with > > > __counted_by > > >https://git.kernel.org/kees/c/4df33089b46f > > > [3/9] drm/i915/selftests: Annotate struct perf_series with __counted_by > > >https://git.kernel.org/kees/c/ffd3f823bdf6 > > > [4/9] drm/msm/dpu: Annotate struct dpu_hw_intr with __counted_by > > >https://git.kernel.org/kees/c/2de35a989b76 > > > [5/9] drm/nouveau/pm: Annotate struct nvkm_perfdom with __counted_by > > >https://git.kernel.org/kees/c/188aeb08bfaa > > > [6/9] drm/vc4: Annotate struct vc4_perfmon with __counted_by > > >https://git.kernel.org/kees/c/59a54dc896c3 > > > [7/9] drm/virtio: Annotate struct virtio_gpu_object_array with > > > __counted_by > > >https://git.kernel.org/kees/c/5cd476de33af > > > [8/9] drm/vmwgfx: Annotate struct vmw_surface_dirty with __counted_by > > >https://git.kernel.org/kees/c/b426f2e5356a > > > [9/9] drm/v3d: Annotate struct v3d_perfmon with __counted_by > > >https://git.kernel.org/kees/c/dc662fa1b0e4 > > > > > > Take care, > > > > > -- Kees Cook
Re: [PATCH 0/9] drm: Annotate structs with __counted_by
On Fri, 22 Sep 2023 10:32:05 -0700, Kees Cook wrote: > This is a batch of patches touching drm for preparing for the coming > implementation by GCC and Clang of the __counted_by attribute. Flexible > array members annotated with __counted_by can have their accesses > bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS (for array > indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family functions). > > As found with Coccinelle[1], add __counted_by to structs that would > benefit from the annotation. > > [...] Since this got Acks, I figure I should carry it in my tree. Let me know if this should go via drm instead. Applied to for-next/hardening, thanks! [1/9] drm/amd/pm: Annotate struct smu10_voltage_dependency_table with __counted_by https://git.kernel.org/kees/c/a6046ac659d6 [2/9] drm/amdgpu/discovery: Annotate struct ip_hw_instance with __counted_by https://git.kernel.org/kees/c/4df33089b46f [3/9] drm/i915/selftests: Annotate struct perf_series with __counted_by https://git.kernel.org/kees/c/ffd3f823bdf6 [4/9] drm/msm/dpu: Annotate struct dpu_hw_intr with __counted_by https://git.kernel.org/kees/c/2de35a989b76 [5/9] drm/nouveau/pm: Annotate struct nvkm_perfdom with __counted_by https://git.kernel.org/kees/c/188aeb08bfaa [6/9] drm/vc4: Annotate struct vc4_perfmon with __counted_by https://git.kernel.org/kees/c/59a54dc896c3 [7/9] drm/virtio: Annotate struct virtio_gpu_object_array with __counted_by https://git.kernel.org/kees/c/5cd476de33af [8/9] drm/vmwgfx: Annotate struct vmw_surface_dirty with __counted_by https://git.kernel.org/kees/c/b426f2e5356a [9/9] drm/v3d: Annotate struct v3d_perfmon with __counted_by https://git.kernel.org/kees/c/dc662fa1b0e4 Take care, -- Kees Cook
Re: [PATCH 1/9] drm/amd/pm: Annotate struct smu10_voltage_dependency_table with __counted_by
On Mon, Sep 25, 2023 at 08:30:30AM +0200, Christian König wrote:
> Am 22.09.23 um 19:41 schrieb Alex Deucher:
> > On Fri, Sep 22, 2023 at 1:32 PM Kees Cook wrote:
> > > Prepare for the coming implementation by GCC and Clang of the __counted_by
> > > attribute. Flexible array members annotated with __counted_by can have
> > > their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS
> > > (for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family
> > > functions).
> > >
> > > As found with Coccinelle[1], add __counted_by for struct
> > > smu10_voltage_dependency_table.
> > >
> > > [1]
> > > https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci
> > >
> > > Cc: Evan Quan
> > > Cc: Alex Deucher
> > > Cc: "Christian König"
> > > Cc: "Pan, Xinhui"
> > > Cc: David Airlie
> > > Cc: Daniel Vetter
> > > Cc: Xiaojian Du
> > > Cc: Huang Rui
> > > Cc: Kevin Wang
> > > Cc: amd-gfx@lists.freedesktop.org
> > > Cc: dri-de...@lists.freedesktop.org
> > > Signed-off-by: Kees Cook
> > Acked-by: Alex Deucher
>
> Mhm, I'm not sure if this is a good idea. That is a structure filled in by
> the firmware, isn't it?
>
> That would imply that we might need to byte swap count before it is
> checkable.
The script found this instance because of this:
static int smu10_get_clock_voltage_dependency_table(struct pp_hwmgr *hwmgr,
struct smu10_voltage_dependency_table **pptable,
uint32_t num_entry, const DpmClock_t
*pclk_dependency_table)
{
uint32_t i;
struct smu10_voltage_dependency_table *ptable;
ptable = kzalloc(struct_size(ptable, entries, num_entry), GFP_KERNEL);
if (NULL == ptable)
return -ENOMEM;
ptable->count = num_entry;
So the implication is that it's native byte order... but you tell me! I
certainly don't want this annotation if it's going to break stuff. :)
--
Kees Cook
Re: [PATCH 3/9] drm/i915/selftests: Annotate struct perf_series with __counted_by
On Mon, Sep 25, 2023 at 12:08:36PM +0200, Andrzej Hajda wrote: > > > On 22.09.2023 19:32, Kees Cook wrote: > > Prepare for the coming implementation by GCC and Clang of the __counted_by > > attribute. Flexible array members annotated with __counted_by can have > > their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS > > (for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family > > functions). > > > > As found with Coccinelle[1], add __counted_by for struct perf_series. > > > > [1] > > https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci > > > > Cc: Jani Nikula > > Cc: Joonas Lahtinen > > Cc: Rodrigo Vivi > > Cc: Tvrtko Ursulin > > Cc: David Airlie > > Cc: Daniel Vetter > > Cc: Chris Wilson > > Cc: John Harrison > > Cc: Andi Shyti > > Cc: Matthew Brost > > Cc: intel-...@lists.freedesktop.org > > Cc: dri-de...@lists.freedesktop.org > > Signed-off-by: Kees Cook > > I am surprised this is the only finding in i915, I would expected more. I'm sure there are more, but it's likely my Coccinelle pattern didn't catch it. There are many many flexible arrays in drm. :) $ grep -nRH '\[\];$' drivers/gpu/drm include/uapi/drm | grep -v :extern | wc -l 122 If anyone has some patterns I can add to the Coccinelle script, I can take another pass at it. > Anyway: > > Reviewed-by: Andrzej Hajda Thank you! -Kees -- Kees Cook
[PATCH 9/9] drm/v3d: Annotate struct v3d_perfmon with __counted_by
Prepare for the coming implementation by GCC and Clang of the __counted_by
attribute. Flexible array members annotated with __counted_by can have
their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS
(for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family
functions).
As found with Coccinelle[1], add __counted_by for struct v3d_perfmon.
[1]
https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci
Cc: Emma Anholt
Cc: Melissa Wen
Cc: David Airlie
Cc: Daniel Vetter
Cc: dri-de...@lists.freedesktop.org
Signed-off-by: Kees Cook
---
drivers/gpu/drm/v3d/v3d_drv.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/v3d/v3d_drv.h b/drivers/gpu/drm/v3d/v3d_drv.h
index 7f664a4b2a75..106454f28956 100644
--- a/drivers/gpu/drm/v3d/v3d_drv.h
+++ b/drivers/gpu/drm/v3d/v3d_drv.h
@@ -59,7 +59,7 @@ struct v3d_perfmon {
* values can't be reset, but you can fake a reset by
* destroying the perfmon and creating a new one.
*/
- u64 values[];
+ u64 values[] __counted_by(ncounters);
};
struct v3d_dev {
--
2.34.1
[PATCH 6/9] drm/vc4: Annotate struct vc4_perfmon with __counted_by
Prepare for the coming implementation by GCC and Clang of the __counted_by
attribute. Flexible array members annotated with __counted_by can have
their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS
(for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family
functions).
As found with Coccinelle[1], add __counted_by for struct vc4_perfmon.
[1]
https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci
Cc: Emma Anholt
Cc: Maxime Ripard
Cc: David Airlie
Cc: Daniel Vetter
Cc: dri-de...@lists.freedesktop.org
Signed-off-by: Kees Cook
---
drivers/gpu/drm/vc4/vc4_drv.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
index bf66499765fb..ab61e96e7e14 100644
--- a/drivers/gpu/drm/vc4/vc4_drv.h
+++ b/drivers/gpu/drm/vc4/vc4_drv.h
@@ -76,7 +76,7 @@ struct vc4_perfmon {
* Note that counter values can't be reset, but you can fake a reset by
* destroying the perfmon and creating a new one.
*/
- u64 counters[];
+ u64 counters[] __counted_by(ncounters);
};
struct vc4_dev {
--
2.34.1
[PATCH 4/9] drm/msm/dpu: Annotate struct dpu_hw_intr with __counted_by
Prepare for the coming implementation by GCC and Clang of the __counted_by
attribute. Flexible array members annotated with __counted_by can have
their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS
(for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family
functions).
As found with Coccinelle[1], add __counted_by for struct dpu_hw_intr.
[1]
https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci
Cc: Rob Clark
Cc: Abhinav Kumar
Cc: Dmitry Baryshkov
Cc: Sean Paul
Cc: Marijn Suijten
Cc: David Airlie
Cc: Daniel Vetter
Cc: Bjorn Andersson
Cc: linux-arm-...@vger.kernel.org
Cc: dri-de...@lists.freedesktop.org
Cc: freedr...@lists.freedesktop.org
Signed-off-by: Kees Cook
---
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.h
b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.h
index dab761e54863..50cf9523d367 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.h
@@ -61,7 +61,7 @@ struct dpu_hw_intr {
void (*cb)(void *arg, int irq_idx);
void *arg;
atomic_t count;
- } irq_tbl[];
+ } irq_tbl[] __counted_by(total_irqs);
};
/**
--
2.34.1
[PATCH 2/9] drm/amdgpu/discovery: Annotate struct ip_hw_instance with __counted_by
Prepare for the coming implementation by GCC and Clang of the __counted_by
attribute. Flexible array members annotated with __counted_by can have
their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS
(for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family
functions).
As found with Coccinelle[1], add __counted_by for struct ip_hw_instance.
[1]
https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci
Cc: Alex Deucher
Cc: "Christian König"
Cc: "Pan, Xinhui"
Cc: David Airlie
Cc: Daniel Vetter
Cc: Hawking Zhang
Cc: amd-gfx@lists.freedesktop.org
Cc: dri-de...@lists.freedesktop.org
Signed-off-by: Kees Cook
---
drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
index d1bc7b212520..be4c97a3d7bf 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
@@ -662,7 +662,7 @@ struct ip_hw_instance {
u8 harvest;
int num_base_addresses;
- u32 base_addr[];
+ u32 base_addr[] __counted_by(num_base_addresses);
};
struct ip_hw_id {
--
2.34.1
[PATCH 5/9] drm/nouveau/pm: Annotate struct nvkm_perfdom with __counted_by
Prepare for the coming implementation by GCC and Clang of the __counted_by
attribute. Flexible array members annotated with __counted_by can have
their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS
(for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family
functions).
As found with Coccinelle[1], add __counted_by for struct nvkm_perfdom.
[1]
https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci
Cc: Ben Skeggs
Cc: Karol Herbst
Cc: Lyude Paul
Cc: David Airlie
Cc: Daniel Vetter
Cc: dri-de...@lists.freedesktop.org
Cc: nouv...@lists.freedesktop.org
Signed-off-by: Kees Cook
---
drivers/gpu/drm/nouveau/nvkm/engine/pm/priv.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/nouveau/nvkm/engine/pm/priv.h
b/drivers/gpu/drm/nouveau/nvkm/engine/pm/priv.h
index 6ae25d3e7f45..c011227f7052 100644
--- a/drivers/gpu/drm/nouveau/nvkm/engine/pm/priv.h
+++ b/drivers/gpu/drm/nouveau/nvkm/engine/pm/priv.h
@@ -82,7 +82,7 @@ struct nvkm_perfdom {
u8 mode;
u32 clk;
u16 signal_nr;
- struct nvkm_perfsig signal[];
+ struct nvkm_perfsig signal[] __counted_by(signal_nr);
};
struct nvkm_funcdom {
--
2.34.1
[PATCH 7/9] drm/virtio: Annotate struct virtio_gpu_object_array with __counted_by
Prepare for the coming implementation by GCC and Clang of the __counted_by
attribute. Flexible array members annotated with __counted_by can have
their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS
(for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family
functions).
As found with Coccinelle[1], add __counted_by for struct
virtio_gpu_object_array.
[1]
https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci
Cc: David Airlie
Cc: Gerd Hoffmann
Cc: Gurchetan Singh
Cc: Chia-I Wu
Cc: Daniel Vetter
Cc: dri-de...@lists.freedesktop.org
Cc: virtualizat...@lists.linux-foundation.org
Signed-off-by: Kees Cook
---
drivers/gpu/drm/virtio/virtgpu_drv.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h
b/drivers/gpu/drm/virtio/virtgpu_drv.h
index 8513b671f871..96365a772f77 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drv.h
+++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
@@ -119,7 +119,7 @@ struct virtio_gpu_object_array {
struct ww_acquire_ctx ticket;
struct list_head next;
u32 nents, total;
- struct drm_gem_object *objs[];
+ struct drm_gem_object *objs[] __counted_by(total);
};
struct virtio_gpu_vbuffer;
--
2.34.1
[PATCH 8/9] drm/vmwgfx: Annotate struct vmw_surface_dirty with __counted_by
Prepare for the coming implementation by GCC and Clang of the __counted_by
attribute. Flexible array members annotated with __counted_by can have
their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS
(for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family
functions).
As found with Coccinelle[1], add __counted_by for struct vmw_surface_dirty.
[1]
https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci
Cc: Zack Rusin
Cc: VMware Graphics Reviewers
Cc: David Airlie
Cc: Daniel Vetter
Cc: dri-de...@lists.freedesktop.org
Signed-off-by: Kees Cook
---
drivers/gpu/drm/vmwgfx/vmwgfx_surface.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
index 5db403ee8261..2d1d857f99ae 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
@@ -77,7 +77,7 @@ struct vmw_surface_offset {
struct vmw_surface_dirty {
struct vmw_surface_cache cache;
u32 num_subres;
- SVGA3dBox boxes[];
+ SVGA3dBox boxes[] __counted_by(num_subres);
};
static void vmw_user_surface_free(struct vmw_resource *res);
--
2.34.1
[PATCH 0/9] drm: Annotate structs with __counted_by
Hi, This is a batch of patches touching drm for preparing for the coming implementation by GCC and Clang of the __counted_by attribute. Flexible array members annotated with __counted_by can have their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS (for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family functions). As found with Coccinelle[1], add __counted_by to structs that would benefit from the annotation. Since the element count member must be set before accessing the annotated flexible array member, some patches also move the member's initialization earlier. (These are noted in the individual patches.) -Kees [1] https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci Kees Cook (9): drm/amd/pm: Annotate struct smu10_voltage_dependency_table with __counted_by drm/amdgpu/discovery: Annotate struct ip_hw_instance with __counted_by drm/i915/selftests: Annotate struct perf_series with __counted_by drm/msm/dpu: Annotate struct dpu_hw_intr with __counted_by drm/nouveau/pm: Annotate struct nvkm_perfdom with __counted_by drm/vc4: Annotate struct vc4_perfmon with __counted_by drm/virtio: Annotate struct virtio_gpu_object_array with __counted_by drm/vmwgfx: Annotate struct vmw_surface_dirty with __counted_by drm/v3d: Annotate struct v3d_perfmon with __counted_by drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c| 2 +- drivers/gpu/drm/amd/pm/powerplay/hwmgr/smu10_hwmgr.h | 2 +- drivers/gpu/drm/i915/selftests/i915_request.c| 2 +- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.h| 2 +- drivers/gpu/drm/nouveau/nvkm/engine/pm/priv.h| 2 +- drivers/gpu/drm/v3d/v3d_drv.h| 2 +- drivers/gpu/drm/vc4/vc4_drv.h| 2 +- drivers/gpu/drm/virtio/virtgpu_drv.h | 2 +- drivers/gpu/drm/vmwgfx/vmwgfx_surface.c | 2 +- 9 files changed, 9 insertions(+), 9 deletions(-) -- 2.34.1
[PATCH 1/9] drm/amd/pm: Annotate struct smu10_voltage_dependency_table with __counted_by
Prepare for the coming implementation by GCC and Clang of the __counted_by
attribute. Flexible array members annotated with __counted_by can have
their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS
(for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family
functions).
As found with Coccinelle[1], add __counted_by for struct
smu10_voltage_dependency_table.
[1]
https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci
Cc: Evan Quan
Cc: Alex Deucher
Cc: "Christian König"
Cc: "Pan, Xinhui"
Cc: David Airlie
Cc: Daniel Vetter
Cc: Xiaojian Du
Cc: Huang Rui
Cc: Kevin Wang
Cc: amd-gfx@lists.freedesktop.org
Cc: dri-de...@lists.freedesktop.org
Signed-off-by: Kees Cook
---
drivers/gpu/drm/amd/pm/powerplay/hwmgr/smu10_hwmgr.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/smu10_hwmgr.h
b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/smu10_hwmgr.h
index 808e0ecbe1f0..42adc2a3dcbc 100644
--- a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/smu10_hwmgr.h
+++ b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/smu10_hwmgr.h
@@ -192,7 +192,7 @@ struct smu10_clock_voltage_dependency_record {
struct smu10_voltage_dependency_table {
uint32_t count;
- struct smu10_clock_voltage_dependency_record entries[];
+ struct smu10_clock_voltage_dependency_record entries[]
__counted_by(count);
};
struct smu10_clock_voltage_information {
--
2.34.1
[PATCH 3/9] drm/i915/selftests: Annotate struct perf_series with __counted_by
Prepare for the coming implementation by GCC and Clang of the __counted_by
attribute. Flexible array members annotated with __counted_by can have
their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS
(for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family
functions).
As found with Coccinelle[1], add __counted_by for struct perf_series.
[1]
https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci
Cc: Jani Nikula
Cc: Joonas Lahtinen
Cc: Rodrigo Vivi
Cc: Tvrtko Ursulin
Cc: David Airlie
Cc: Daniel Vetter
Cc: Chris Wilson
Cc: John Harrison
Cc: Andi Shyti
Cc: Matthew Brost
Cc: intel-...@lists.freedesktop.org
Cc: dri-de...@lists.freedesktop.org
Signed-off-by: Kees Cook
---
drivers/gpu/drm/i915/selftests/i915_request.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/selftests/i915_request.c
b/drivers/gpu/drm/i915/selftests/i915_request.c
index a9b79888c193..acae30a04a94 100644
--- a/drivers/gpu/drm/i915/selftests/i915_request.c
+++ b/drivers/gpu/drm/i915/selftests/i915_request.c
@@ -1924,7 +1924,7 @@ struct perf_stats {
struct perf_series {
struct drm_i915_private *i915;
unsigned int nengines;
- struct intel_context *ce[];
+ struct intel_context *ce[] __counted_by(nengines);
};
static int cmp_u32(const void *A, const void *B)
--
2.34.1
Re: [PATCH][next] drm/amdgpu/discovery: Replace fake flex-arrays with flexible-array members
On Sun, May 28, 2023 at 02:26:37PM -0600, Gustavo A. R. Silva wrote:
> Zero-length and one-element arrays are deprecated, and we are moving
> towards adopting C99 flexible-array members, instead.
>
> Use the DECLARE_FLEX_ARRAY() helper macro to transform zero-length
> arrays in a union into flexible-array members. And replace a one-element
> array with a C99 flexible-array member.
>
> Address the following warnings found with GCC-13 and
> -fstrict-flex-arrays=3 enabled:
> drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c:1009:89: warning: array
> subscript kk is outside array bounds of ‘uint32_t[0]’ {aka ‘unsigned int[]’}
> [-Warray-bounds=]
> drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c:1007:94: warning: array
> subscript kk is outside array bounds of ‘uint64_t[0]’ {aka ‘long long
> unsigned int[]’} [-Warray-bounds=]
> drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c:1310:94: warning: array
> subscript k is outside array bounds of ‘uint64_t[0]’ {aka ‘long long unsigned
> int[]’} [-Warray-bounds=]
> drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c:1309:57: warning: array
> subscript k is outside array bounds of ‘uint32_t[0]’ {aka ‘unsigned int[]’}
> [-Warray-bounds=]
>
> This helps with the ongoing efforts to tighten the FORTIFY_SOURCE
> routines on memcpy() and help us make progress towards globally
> enabling -fstrict-flex-arrays=3 [1].
>
> This results in no differences in binary output.
>
> Link: https://github.com/KSPP/linux/issues/21
> Link: https://github.com/KSPP/linux/issues/193
> Link: https://github.com/KSPP/linux/issues/300
> Link: https://gcc.gnu.org/pipermail/gcc-patches/2022-October/602902.html [1]
> Signed-off-by: Gustavo A. R. Silva
Reviewed-by: Kees Cook
--
Kees Cook
Re: [PATCH v2] drm/amd/display: enable more strict compile checks
Hi! On Wed, May 24, 2023 at 04:27:31PM -0400, Hamza Mahfooz wrote: > + Kees > > On 5/24/23 15:50, Alex Deucher wrote: > > On Wed, May 24, 2023 at 3:46 PM Felix Kuehling > > wrote: > > > > > > Sure, I think we tried enabling warnings as errors before and had to > > > revert it because of weird compiler quirks or the variety of compiler > > > versions that need to be supported. > > > > > > Alex, are you planning to upstream this, or is this only to enforce more > > > internal discipline about not ignoring warnings? > > > > I'd like to upstream it. Upstream already has CONFIG_WERROR as a > > config option, but it's been problematic to enable in CI because of > > various breakages outside of the driver and in different compilers. > > That said, I don't know how much trouble enabling it will cause with > > various compilers in the wild. -Wmisleading-indentation is already part of -Wall, so this is globally enabled already. -Wunused is enabled under W=1, and it's pretty noisy still. If you can get builds clean in drm, that'll be a good step towards getting it enabled globally. (A middle ground with less to clean up might be -Wunused-but-set-variable) I agree about -Werror: just stick with CONFIG_WERROR instead. -Kees > > > > Alex > > > > > > > > Regards, > > > Felix > > > > > > > > > On 2023-05-24 15:41, Russell, Kent wrote: > > > > > > > > [AMD Official Use Only - General] > > > > > > > > > > > > (Adding Felix in CC) > > > > > > > > I’m a fan of adding it to KFD as well. Felix, can you foresee any > > > > issues here? > > > > > > > > Kent > > > > > > > > *From:* amd-gfx *On Behalf Of > > > > *Ho, Kenny > > > > *Sent:* Wednesday, May 24, 2023 3:23 PM > > > > *To:* Alex Deucher ; Mahfooz, Hamza > > > > > > > > *Cc:* Li, Sun peng (Leo) ; Wentland, Harry > > > > ; Pan, Xinhui ; Siqueira, > > > > Rodrigo ; linux-ker...@vger.kernel.org; > > > > dri-de...@lists.freedesktop.org; amd-gfx@lists.freedesktop.org; Daniel > > > > Vetter ; Deucher, Alexander > > > > ; David Airlie ; Koenig, > > > > Christian > > > > *Subject:* Re: [PATCH v2] drm/amd/display: enable more strict compile > > > > checks > > > > > > > > [AMD Official Use Only - General] > > > > > > > > [AMD Official Use Only - General] > > > > > > > > (+ Felix) > > > > > > > > Should we do the same for other modules under amd (amdkfd)? I was > > > > going to enable full kernel werror in the kconfig used by my CI but > > > > this is probably better. > > > > > > > > Kenny > > > > > > > > > > > > > > > > *From:*Alex Deucher > > > > *Sent:* Wednesday, May 24, 2023 3:22 PM > > > > *To:* Mahfooz, Hamza > > > > *Cc:* amd-gfx@lists.freedesktop.org ; > > > > Li, Sun peng (Leo) ; Ho, Kenny ; > > > > Pan, Xinhui ; Siqueira, Rodrigo > > > > ; linux-ker...@vger.kernel.org > > > > ; dri-de...@lists.freedesktop.org > > > > ; Daniel Vetter ; > > > > Deucher, Alexander ; David Airlie > > > > ; Wentland, Harry ; Koenig, > > > > Christian > > > > *Subject:* Re: [PATCH v2] drm/amd/display: enable more strict compile > > > > checks > > > > > > > > On Wed, May 24, 2023 at 3:20 PM Hamza Mahfooz > > > > wrote: > > > > > > > > > > Currently, there are quite a number of issues that are quite easy for > > > > > the CI to catch, that slip through the cracks. Among them, there are > > > > > unused variable and indentation issues. Also, we should consider all > > > > > warnings to be compile errors, since the community will eventually end > > > > > up complaining about them. So, enable -Werror, -Wunused and > > > > > -Wmisleading-indentation for all kernel builds. > > > > > > > > > > Cc: Alex Deucher > > > > > Cc: Harry Wentland > > > > > Cc: Kenny Ho > > > > > Signed-off-by: Hamza Mahfooz > > > > > --- > > > > > v2: fix grammatical error > > > > > --- > > > > > drivers/gpu/drm/amd/display/Makefile | 2 ++ > > > > > 1 file changed, 2 insertions(+) > > > > > > > > > > diff --git a/drivers/gpu/drm/amd/display/Makefile > > > > b/drivers/gpu/drm/amd/display/Makefile > > > > > index 0d610cb376bb..3c44162ebe21 100644 > > > > > --- a/drivers/gpu/drm/amd/display/Makefile > > > > > +++ b/drivers/gpu/drm/amd/display/Makefile > > > > > @@ -26,6 +26,8 @@ > > > > > > > > > > AMDDALPATH = $(RELATIVE_AMD_DISPLAY_PATH) > > > > > > > > > > +subdir-ccflags-y += -Werror -Wunused -Wmisleading-indentation > > > > > + > > > > > > > > Care to enable this for the rest of amdgpu as well? Or send out an > > > > additional patch to do that? Either way: > > > > Reviewed-by: Alex Deucher > > > > > > > > Alex > > > > > > > > > subdir-ccflags-y += -I$(FULL_AMD_DISPLAY_PATH)/dc/inc/ > > > > > subdir-ccflags-y += -I$(FULL_AMD_DISPLAY_PATH)/dc/inc/hw > > > > > subdir-ccflags-y += -I$(FULL_AMD_DISPLAY_PATH)/dc/clk_mgr > > > > > -- > > > > > 2.40.1 > > > > > > > > > > -- > Hamza > -- Kees Cook
Re: [PATCH] drm/radeon: Replace all non-returning strlcpy with strscpy
On Mon, 22 May 2023 15:50:32 +, Azeem Shaikh wrote: > strlcpy() reads the entire source buffer first. > This read may exceed the destination size limit. > This is both inefficient and can lead to linear read > overflows if a source string is not NUL-terminated [1]. > In an effort to remove strlcpy() completely [2], replace > strlcpy() here with strscpy(). > No return values were used, so direct replacement is safe. > > [...] Applied to for-next/hardening, thanks! [1/1] drm/radeon: Replace all non-returning strlcpy with strscpy https://git.kernel.org/kees/c/76ea3f6ef93f -- Kees Cook
Re: [PATCH] drm/amd/pm: Replace all non-returning strlcpy with strscpy
On Mon, 22 May 2023 15:52:45 +, Azeem Shaikh wrote: > strlcpy() reads the entire source buffer first. > This read may exceed the destination size limit. > This is both inefficient and can lead to linear read > overflows if a source string is not NUL-terminated [1]. > In an effort to remove strlcpy() completely [2], replace > strlcpy() here with strscpy(). > No return values were used, so direct replacement is safe. > > [...] Applied to for-next/hardening, thanks! [1/1] drm/amd/pm: Replace all non-returning strlcpy with strscpy https://git.kernel.org/kees/c/0f53b61b1ca0 -- Kees Cook
Re: [PATCH] drm/radeon: Replace all non-returning strlcpy with strscpy
On Mon, May 22, 2023 at 03:50:32PM +, Azeem Shaikh wrote: > strlcpy() reads the entire source buffer first. > This read may exceed the destination size limit. > This is both inefficient and can lead to linear read > overflows if a source string is not NUL-terminated [1]. > In an effort to remove strlcpy() completely [2], replace > strlcpy() here with strscpy(). > No return values were used, so direct replacement is safe. > > [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy > [2] https://github.com/KSPP/linux/issues/89 > > Signed-off-by: Azeem Shaikh Reviewed-by: Kees Cook -- Kees Cook
Re: [PATCH] drm/amd/pm: Replace all non-returning strlcpy with strscpy
On Mon, May 22, 2023 at 03:52:45PM +, Azeem Shaikh wrote: > strlcpy() reads the entire source buffer first. > This read may exceed the destination size limit. > This is both inefficient and can lead to linear read > overflows if a source string is not NUL-terminated [1]. > In an effort to remove strlcpy() completely [2], replace > strlcpy() here with strscpy(). > No return values were used, so direct replacement is safe. > > [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy > [2] https://github.com/KSPP/linux/issues/89 > > Signed-off-by: Azeem Shaikh Reviewed-by: Kees Cook -- Kees Cook
Re: [PATCH] drm/amd: fix some dead code in `gfx_v9_0_init_cp_compute_microcode`
On Thu, Jan 12, 2023 at 04:37:01PM -0600, Mario Limonciello wrote:
> Some dead code was introdcued as part of utilizing the `amdgpu_ucode_*`
> helpers. Adjust the control flow to make sure that firmware is released
> in the appropriate error flows.
>
> Reported-by: coverity-bot
> Addresses-Coverity-ID: 1530548 ("Control flow issues")
> Fixes: ec787deb2ddf ("drm/amd: Use `amdgpu_ucode_*` helpers for GFX9")
> Signed-off-by: Mario Limonciello
Reviewed-by: Kees Cook
--
Kees Cook
Re: Coverity: dm_dmub_sw_init(): Incorrect expression
On Thu, Jan 12, 2023 at 10:39:20PM +, Limonciello, Mario wrote: > This particular one was fixed already in > https://patchwork.freedesktop.org/patch/518050/ which got applied today. Ah-ha; thanks! -- Kees Cook
Re: [PATCH] drm/amdkfd: Fix the memory overrun
On Mon, Nov 07, 2022 at 03:08:06PM +0800, Ma Jun wrote: > Fix the memory overrun issue caused by wrong array size. > > Signed-off-by: Ma Jun Reviewed-by: Kees Cook -- Kees Cook
Re: [PATCH 2/2] drm/amdgpu: Fix type of second parameter in odn_edit_dpm_table() callback
On Wed, Nov 02, 2022 at 08:25:40AM -0700, Nathan Chancellor wrote: > With clang's kernel control flow integrity (kCFI, CONFIG_CFI_CLANG), > indirect call targets are validated against the expected function > pointer prototype to make sure the call target is valid to help mitigate > ROP attacks. If they are not identical, there is a failure at run time, > which manifests as either a kernel panic or thread getting killed. A > proposed warning in clang aims to catch these at compile time, which > reveals: > > drivers/gpu/drm/amd/amdgpu/../pm/swsmu/amdgpu_smu.c:3008:29: error: > incompatible function pointer types initializing 'int (*)(void *, uint32_t, > long *, uint32_t)' (aka 'int (*)(void *, unsigned int, long *, unsigned > int)') with an expression of type 'int (void *, enum PP_OD_DPM_TABLE_COMMAND, > long *, uint32_t)' (aka 'int (void *, enum PP_OD_DPM_TABLE_COMMAND, long *, > unsigned int)') [-Werror,-Wincompatible-function-pointer-types-strict] > .odn_edit_dpm_table = smu_od_edit_dpm_table, > ^ > 1 error generated. > > There are only two implementations of ->odn_edit_dpm_table() in 'struct > amd_pm_funcs': smu_od_edit_dpm_table() and pp_odn_edit_dpm_table(). One > has a second parameter type of 'enum PP_OD_DPM_TABLE_COMMAND' and the > other uses 'u32'. Ultimately, smu_od_edit_dpm_table() calls > ->od_edit_dpm_table() from 'struct pptable_funcs' and > pp_odn_edit_dpm_table() calls ->odn_edit_dpm_table() from 'struct > pp_hwmgr_func', which both have a second parameter type of 'enum > PP_OD_DPM_TABLE_COMMAND'. > > Update the type parameter in both the prototype in 'struct amd_pm_funcs' > and pp_odn_edit_dpm_table() to 'enum PP_OD_DPM_TABLE_COMMAND', which > cleans up the warning. > > Link: https://github.com/ClangBuiltLinux/linux/issues/1750 > Reported-by: Sami Tolvanen > Signed-off-by: Nathan Chancellor Reviewed-by: Kees Cook -- Kees Cook
Re: [PATCH 1/2] drm/amdgpu: Fix type of second parameter in trans_msg() callback
On Wed, Nov 02, 2022 at 08:25:39AM -0700, Nathan Chancellor wrote: > With clang's kernel control flow integrity (kCFI, CONFIG_CFI_CLANG), > indirect call targets are validated against the expected function > pointer prototype to make sure the call target is valid to help mitigate > ROP attacks. If they are not identical, there is a failure at run time, > which manifests as either a kernel panic or thread getting killed. A > proposed warning in clang aims to catch these at compile time, which > reveals: > > drivers/gpu/drm/amd/amdgpu/mxgpu_ai.c:412:15: error: incompatible function > pointer types initializing 'void (*)(struct amdgpu_device *, u32, u32, u32, > u32)' (aka 'void (*)(struct amdgpu_device *, unsigned int, unsigned int, > unsigned int, unsigned int)') with an expression of type 'void (struct > amdgpu_device *, enum idh_request, u32, u32, u32)' (aka 'void (struct > amdgpu_device *, enum idh_request, unsigned int, unsigned int, unsigned > int)') [-Werror,-Wincompatible-function-pointer-types-strict] > .trans_msg = xgpu_ai_mailbox_trans_msg, > ^ > 1 error generated. > > drivers/gpu/drm/amd/amdgpu/mxgpu_nv.c:435:15: error: incompatible function > pointer types initializing 'void (*)(struct amdgpu_device *, u32, u32, u32, > u32)' (aka 'void (*)(struct amdgpu_device *, unsigned int, unsigned int, > unsigned int, unsigned int)') with an expression of type 'void (struct > amdgpu_device *, enum idh_request, u32, u32, u32)' (aka 'void (struct > amdgpu_device *, enum idh_request, unsigned int, unsigned int, unsigned > int)') [-Werror,-Wincompatible-function-pointer-types-strict] > .trans_msg = xgpu_nv_mailbox_trans_msg, > ^ > 1 error generated. > > The type of the second parameter in the prototype should be 'enum > idh_request' instead of 'u32'. Update it to clear up the warnings. > > Link: https://github.com/ClangBuiltLinux/linux/issues/1750 > Reported-by: Sami Tolvanen > Signed-off-by: Nathan Chancellor Reviewed-by: Kees Cook -- Kees Cook
Re: [PATCH v2] [next] drm/radeon: Replace one-element array with flexible-array member
On Tue, Nov 01, 2022 at 06:09:16PM -0400, Alex Deucher wrote: > On Tue, Nov 1, 2022 at 5:54 PM Kees Cook wrote: > > Does the ROM always only have a single byte there? This seems unlikely > > given the member "ucFakeEDIDLength" (and the code below). > > I'm not sure. I'm mostly concerned about this: > > record += fake_edid_record->ucFakeEDIDLength ? > fake_edid_record->ucFakeEDIDLength + 2 : > sizeof(ATOM_FAKE_EDID_PATCH_RECORD); But this is exactly what the code currently does, as noted in the commit log: "It's worth mentioning that doing a build before/after this patch results in no binary output differences. > Presumably the record should only exist if ucFakeEDIDLength is non 0, > but I don't know if there are some OEMs out there that just included > an empty record for some reason. Maybe the code is wrong today and > there are some OEMs that include it and the array is already size 0. > In that case, Paulo's original patches are probably more correct. Right, but if true, that seems to be a distinctly separate bug fix? -- Kees Cook
Re: [PATCH] drm/radeon: Replace kmap() with kmap_local_page()
On Wed, Nov 02, 2022 at 12:11:53AM +0100, Fabio M. De Francesco wrote: > On lunedì 17 ottobre 2022 18:52:10 CET Alex Deucher wrote: > > Applied. Thanks! > > Many thanks to you! > > However, about a week ago, I received a report saying that this patch is "Not > Applicable". > > That email was also referring to another patch, for which I'll reply in its > own thread. > > That report has a link to https://patchwork.linuxtv.org/project/linux-media/ > patch/20221013210714.16320-1-fmdefrance...@gmail.com/ > > Can you please let me understand why, despite it was applied, this patch > later > shifted "State" to "Not Applicable"? The kernel has multiple patchwork instances, so you got an "N/A" from linux-media, but it was applied to the drm tree. (Yes, confusing. :P) -- Kees Cook
Re: [PATCH v2] [next] drm/radeon: Replace one-element array with flexible-array member
On Tue, Nov 01, 2022 at 10:42:14AM -0400, Alex Deucher wrote:
> On Fri, Oct 28, 2022 at 11:32 PM Paulo Miguel Almeida
> wrote:
> >
> > One-element arrays are deprecated, and we are replacing them with
> > flexible array members instead. So, replace one-element array with
> > flexible-array member in struct _ATOM_FAKE_EDID_PATCH_RECORD and
> > refactor the rest of the code accordingly.
> >
> > It's worth mentioning that doing a build before/after this patch results
> > in no binary output differences.
> >
> > This helps with the ongoing efforts to tighten the FORTIFY_SOURCE
> > routines on memcpy() and help us make progress towards globally
> > enabling -fstrict-flex-arrays=3 [1].
>
> This seems like a worthy goal, and I'm not opposed to the patch per
> se, but it seems a bit at odds with what this header represents.
> atombios.h represents the memory layout of the data stored in the ROM
> on the GPU. This changes the memory layout of that ROM. We can work
It doesn't though. Or maybe I'm misunderstanding what you mean.
> around that in the driver code, but if someone were to take this
> header to try and write some standalone tool or use it for something
> else in the kernel, it would not reflect reality.
Does the ROM always only have a single byte there? This seems unlikely
given the member "ucFakeEDIDLength" (and the code below).
The problem on the kernel side is that the code just before the patch
context in drivers/gpu/drm/amd/amdgpu/atombios_encoders.c will become
a problem soon:
if (fake_edid_record->ucFakeEDIDLength) {
struct edid *edid;
int edid_size =
max((int)EDID_LENGTH,
(int)fake_edid_record->ucFakeEDIDLength);
edid = kmalloc(edid_size, GFP_KERNEL);
if (edid) {
memcpy((u8 *)edid, (u8
*)_edid_record->ucFakeEDIDString[0],
fake_edid_record->ucFakeEDIDLength);
if (drm_edid_is_valid(edid)) {
...
the memcpy() from "fake_edid_record->ucFakeEDIDString" will eventually
start to WARN, since the size of that field will be seen as a single byte
under -fstrict-flex-arrays. At this moment, no, there's neither source
bounds checking nor -fstrict-flex-arrays, but we're trying to clean up
everything we can find now so that we don't have to do this all again
later. :)
-Kees
P.S. Also this could be shorter with fewer casts:
struct edid *edid;
u8 edid_size =
max_t(u8, EDID_LENGTH,
fake_edid_record->ucFakeEDIDLength);
edid = kmemdup(fake_edid_record->ucFakeEDIDString, edid_size,
GFP_KERNEL);
if (edid) {
if (drm_edid_is_valid(edid)) {
adev->mode_info.bios_hardcoded_edid = edid;
...
--
Kees Cook
Re: [PATCH v2] [next] drm/radeon: Replace one-element array with flexible-array member
On Sat, Oct 29, 2022 at 04:32:05PM +1300, Paulo Miguel Almeida wrote: > One-element arrays are deprecated, and we are replacing them with > flexible array members instead. So, replace one-element array with > flexible-array member in struct _ATOM_FAKE_EDID_PATCH_RECORD and > refactor the rest of the code accordingly. > > It's worth mentioning that doing a build before/after this patch results > in no binary output differences. Thanks for checking it! > > This helps with the ongoing efforts to tighten the FORTIFY_SOURCE > routines on memcpy() and help us make progress towards globally > enabling -fstrict-flex-arrays=3 [1]. > > Link: https://github.com/KSPP/linux/issues/79 > Link: https://github.com/KSPP/linux/issues/239 > Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101836 [1] > > Signed-off-by: Paulo Miguel Almeida Reviewed-by: Kees Cook -- Kees Cook
Re: [PATCH] [next] drm/amdgpu: Replace one-element array with flexible-array member
On Fri, Oct 28, 2022 at 09:18:39AM +0200, Christian König wrote: > Am 28.10.22 um 07:10 schrieb Paulo Miguel Almeida: > > One-element arrays are deprecated, and we are replacing them with > > flexible array members instead. So, replace one-element array with > > flexible-array member in struct _ATOM_FAKE_EDID_PATCH_RECORD and > > refactor the rest of the code accordingly. > > > > This helps with the ongoing efforts to tighten the FORTIFY_SOURCE > > routines on memcpy() and help us make progress towards globally > > enabling -fstrict-flex-arrays=3 [1]. > > > > Link: https://github.com/KSPP/linux/issues/79 > > Link: https://github.com/KSPP/linux/issues/238 > > Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101836 [1] > > I'm not sure if that's a good idea. We had multiple attempts to refactor > this now and it always caused a regression. > > Additional to that the header in question came from our BIOS team and isn't > following Linux styles in general. > > Alex what do you think? Fake flexible arrays (i.e. 1-element arrays) are deprecated in Linux[1] (and, frankly, deprecated in C since 1999 and even well before then given the 0-sized extension that was added in GCC), so we can't continue to bring them into kernel sources. Their use breaks both compile-time and run-time bounds checking efforts, etc. All that said, converting away from them can be tricky, and I think such conversions need to explicitly show how they were checked for binary differences[2]. Paulo, can you please check for deltas and report your findings in the commit log? Note that add struct_size() use in the same patch may result in binary differences, so for more complex cases, you may want to split the 1-element conversion from the struct_size() conversions. -Kees [1] https://docs.kernel.org/process/deprecated.html#zero-length-and-one-element-arrays [2] https://outflux.net/blog/archives/2022/06/24/finding-binary-differences/ -- Kees Cook
Re: [PATCH] drm/radeon: Replace kmap() with kmap_local_page()
On Thu, Oct 13, 2022 at 11:07:14PM +0200, Fabio M. De Francesco wrote: > The use of kmap() is being deprecated in favor of kmap_local_page(). > > There are two main problems with kmap(): (1) It comes with an overhead as > the mapping space is restricted and protected by a global lock for > synchronization and (2) it also requires global TLB invalidation when the > kmap’s pool wraps and it might block when the mapping space is fully > utilized until a slot becomes available. > > With kmap_local_page() the mappings are per thread, CPU local, can take > page faults, and can be called from any context (including interrupts). > It is faster than kmap() in kernels with HIGHMEM enabled. Furthermore, > the tasks can be preempted and, when they are scheduled to run again, the > kernel virtual addresses are restored and still valid. > > Therefore, replace kmap() with kmap_local_page() in radeon_ttm_gtt_read(). > > Cc: "Venkataramanan, Anirudh" > Suggested-by: Ira Weiny > Signed-off-by: Fabio M. De Francesco Reviewed-by: Kees Cook -- Kees Cook
Re: mainline build failure due to 5d8c3e836fc2 ("drm/amd/display: fix array-bounds error in dc_stream_remove_writeback()")
On Thu, Oct 06, 2022 at 12:39:40PM -0700, Linus Torvalds wrote:
> What confuses me is that error message ("array subscript [0, 0] is
> outside array bounds of 'struct dc_writeback_info[1]') which seems to
> be aware that the value is actually 0.
I've seen bugs in the tracker where the reporting is broken but the
range checker is working "correctly", which seems to be the case here.
> If somebody cannot come up with a fix, I suspect the solution is "gcc
> array bounds analysis is terminally buggy" and we just need to disable
> it for gcc-11 too.
It does continue to find bugs, so I'd rather keep it on. GCC has fixed
all the issues we've run into so far (though not all have been back
ported to GCC 12 yet, so yes, let's keep -Warray-bounds disabled there).
Specifically, I've been tracking:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105679 Fixed 13+
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99578 Fixed 12+, 11.3
And it looks like Sudip's proposed fix for this particular code is
additionally fixing unsigned vs signed as well. I think -Warray-bounds
did its job (though, with quite a confusing index range in the report).
-Kees
--
Kees Cook
Re: Build regressions/improvements in v6.0-rc4
On Mon, Sep 05, 2022 at 09:46:01AM +0200, Geert Uytterhoeven wrote: > On Mon, 5 Sep 2022, Geert Uytterhoeven wrote: > > JFYI, when comparing v6.0-rc4[1] to v6.0-rc3[3], the summaries are: > > - build errors: +3/-16 > > + /kisskb/src/arch/sh/kernel/machvec.c: error: array subscript 'struct > sh_machine_vector[0]' is partly outside array bounds of 'long int[1]' > [-Werror=array-bounds]: => 105:33 > > sh4-gcc11/sh-allyesconfig (-Werror) Sent a patch for this: https://lore.kernel.org/linux-hardening/20220907234345.96798-1-keesc...@chromium.org/ > + /kisskb/src/include/linux/fortify-string.h: error: call to > '__write_overflow_field' declared with attribute warning: detected write > beyond size of field (1st parameter); maybe use struct_group()? > [-Werror=attribute-warning]: => 258:25 > > s390x-gcc11/s390-allyesconfig (inlined from 'copy_process' at > /kisskb/src/kernel/fork.c:2200:2) This error appears to have vanished? > > [3] > > http://kisskb.ellerman.id.au/kisskb/branch/linus/head/b90cb1053190353cc30f0fef0ef1f378ccc063c5/ > > (all 135 configs) Status Date/time Target OK Sep 7, 13:54linus/s390-allyesconfig/s390x-gcc11 -- Kees Cook
Re: Build regressions/improvements in v6.0-rc4
On Mon, Sep 05, 2022 at 09:46:01AM +0200, Geert Uytterhoeven wrote:
> On Mon, 5 Sep 2022, Geert Uytterhoeven wrote:
> > JFYI, when comparing v6.0-rc4[1] to v6.0-rc3[3], the summaries are:
> > - build errors: +3/-16
>
> + /kisskb/src/arch/sh/kernel/machvec.c: error: array subscript 'struct
> sh_machine_vector[0]' is partly outside array bounds of 'long int[1]'
> [-Werror=array-bounds]: => 105:33
>
> sh4-gcc11/sh-allyesconfig (-Werror)
Interesting -- I wonder why this suddenly appeared. I think the fix is
the common "linker addresses need to be char arrays" fix:
diff --git a/arch/sh/include/asm/sections.h b/arch/sh/include/asm/sections.h
index 8edb824049b9..0cb0ca149ac3 100644
--- a/arch/sh/include/asm/sections.h
+++ b/arch/sh/include/asm/sections.h
@@ -4,7 +4,7 @@
#include
-extern long __machvec_start, __machvec_end;
+extern char __machvec_start[], __machvec_end[];
extern char __uncached_start, __uncached_end;
extern char __start_eh_frame[], __stop_eh_frame[];
diff --git a/arch/sh/kernel/machvec.c b/arch/sh/kernel/machvec.c
index d606679a211e..57efaf5b82ae 100644
--- a/arch/sh/kernel/machvec.c
+++ b/arch/sh/kernel/machvec.c
@@ -20,8 +20,8 @@
#define MV_NAME_SIZE 32
#define for_each_mv(mv) \
- for ((mv) = (struct sh_machine_vector *)&__machvec_start; \
-(mv) && (unsigned long)(mv) < (unsigned long)&__machvec_end; \
+ for ((mv) = (struct sh_machine_vector *)__machvec_start; \
+(mv) && (unsigned long)(mv) < (unsigned long)__machvec_end; \
(mv)++)
static struct sh_machine_vector * __init get_mv_byname(const char *name)
@@ -87,8 +87,8 @@ void __init sh_mv_setup(void)
if (!machvec_selected) {
unsigned long machvec_size;
- machvec_size = ((unsigned long)&__machvec_end -
- (unsigned long)&__machvec_start);
+ machvec_size = ((unsigned long)__machvec_end -
+ (unsigned long)__machvec_start);
/*
* Sanity check for machvec section alignment. Ensure
@@ -102,7 +102,7 @@ void __init sh_mv_setup(void)
* vector (usually the only one) from .machvec.init.
*/
if (machvec_size >= sizeof(struct sh_machine_vector))
- sh_mv = *(struct sh_machine_vector *)&__machvec_start;
+ sh_mv = *(struct sh_machine_vector *)__machvec_start;
}
pr_notice("Booting machvec: %s\n", get_system_type());
>
> +
> /kisskb/src/drivers/gpu/drm/amd/amdgpu/../display/dc/dml/dcn32/display_mode_vba_32.c:
> error: the frame size of 2144 bytes is larger than 2048 bytes
> [-Werror=frame-larger-than=]: => 3768:1
>
> x86_64-gcc8/x86-allmodconfig (in function
> dml32_ModeSupportAndSystemConfigurationFull())
This I don't know about it, but looks like a recent commit: dda4fb85e433f
Given it's a 2000 line function, I assume it could be improved! :)
> + /kisskb/src/include/linux/fortify-string.h: error: call to
> '__write_overflow_field' declared with attribute warning: detected write
> beyond size of field (1st parameter); maybe use struct_group()?
> [-Werror=attribute-warning]: => 258:25
>
> s390x-gcc11/s390-allyesconfig (inlined from 'copy_process' at
> /kisskb/src/kernel/fork.c:2200:2)
This is:
memset(>irqtrace, 0, sizeof(p->irqtrace));
p->irqtrace is:
struct irqtrace_events irqtrace;
But that's a whole object destination... why would only s390 warn?
-Kees
--
Kees Cook
Re: [PATCH][next] drm/amd/display: Fix Wstringop-overflow warnings in dc_link_dp.c
On Thu, Mar 03, 2022 at 11:25:03AM -0600, Gustavo A. R. Silva wrote:
> Fix the following Wstringop-overflow warnings when building with GCC-11:
>
> drivers/gpu/drm/amd/amdgpu/../display/dc/core/dc_link_dpia.c:493:17: warning:
> ‘dp_decide_lane_settings’ accessing 4 bytes in a region of size 1
> [-Wstringop-overflow=]
Can you "show your work" a little more here? I don't actually see the
what is getting fixed:
enum dc_lane_count {
...
LANE_COUNT_FOUR = 4,
...
LANE_COUNT_DP_MAX = LANE_COUNT_FOUR
};
struct link_training_settings {
...
union dpcd_training_lane dpcd_lane_settings[LANE_COUNT_DP_MAX];
};
void dp_hw_to_dpcd_lane_settings(
...
union dpcd_training_lane dpcd_lane_settings[LANE_COUNT_DP_MAX])
{
...
}
static enum link_training_result dpia_training_cr_transparent(
...
struct link_training_settings *lt_settings)
{
...
dp_decide_lane_settings(lt_settings, dpcd_lane_adjust,
lt_settings->hw_lane_settings,
lt_settings->dpcd_lane_settings);
...
}
Everything looks to be the correct size?
--
Kees Cook
Re: [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
On Wed, Mar 02, 2022 at 12:18:45PM -0800, Linus Torvalds wrote: > On Wed, Mar 2, 2022 at 12:07 PM Kees Cook wrote: > > > > I've long wanted to change kfree() to explicitly set pointers to NULL on > > free. https://github.com/KSPP/linux/issues/87 > > We've had this discussion with the gcc people in the past, and gcc > actually has some support for it, but it's sadly tied to the actual > function name (ie gcc has some special-casing for "free()") > > See > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94527 > > for some of that discussion. > > Oh, and I see some patch actually got merged since I looked there last > so that you can mark "deallocator" functions, but I think it's only > for the context matching, not for actually killing accesses to the > pointer afterwards. Ah! I missed that getting added in GCC 11. But yes, there it is: https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#index-malloc-function-attribute Hah, now we may need to split __malloc from __alloc_size. ;) I'd still like the NULL assignment behavior, though, since some things can easily avoid static analysis. -- Kees Cook
Re: [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
On Wed, Mar 02, 2022 at 10:29:31AM +0100, Rasmus Villemoes wrote:
> This won't help the current issue (because it doesn't exist and might
> never), but just in case some compiler people are listening, I'd like to
> have some sort of way to tell the compiler "treat this variable as
> uninitialized from here on". So one could do
>
> #define kfree(p) do { __kfree(p); __magic_uninit(p); } while (0)
>
> with __magic_uninit being a magic no-op that doesn't affect the
> semantics of the code, but could be used by the compiler's "[is/may be]
> used uninitialized" machinery to flag e.g. double frees on some odd
> error path etc. It would probably only work for local automatic
> variables, but it should be possible to just ignore the hint if p is
> some expression like foo->bar or has side effects. If we had that, the
> end-of-loop test could include that to "uninitialize" the iterator.
I've long wanted to change kfree() to explicitly set pointers to NULL on
free. https://github.com/KSPP/linux/issues/87
The thing stopping a trivial transformation of kfree() is:
kfree(get_some_pointer());
I would argue, though, that the above is poor form: the thing holding
the pointer should be the thing freeing it, so these cases should be
refactored and kfree() could do the NULLing by default.
Quoting myself in the above issue:
Without doing massive tree-wide changes, I think we need compiler
support. If we had something like __builtin_is_lvalue(), we could
distinguish function returns from lvalues. For example, right now a
common case are things like:
kfree(get_some_ptr());
But if we could at least gain coverage of the lvalue cases, and detect
them statically at compile-time, we could do:
#define __kfree_and_null(x) do { __kfree(*x); *x = NULL; } while (0)
#define kfree(x) __builtin_choose_expr(__builtin_is_lvalue(x),
__kfree_and_null(&(x)), __kfree(x))
Alternatively, we could do a tree-wide change of the former case (findable
with Coccinelle) and change them into something like kfree_no_null()
and redefine kfree() itself:
#define kfree_no_null(x) do { void *__ptr = (x); __kfree(__ptr); } while (0)
#define kfree(x) do { __kfree(x); x = NULL; } while (0)
--
Kees Cook
Re: [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
On Mon, Feb 28, 2022 at 04:45:11PM -0800, Linus Torvalds wrote: > Really. The "-Wshadow doesn't work on the kernel" is not some new > issue, because you have to do completely insane things to the source > code to enable it. The first big glitch with -Wshadow was with shadowed global variables. GCC 4.8 fixed that, but it still yells about shadowed functions. What _almost_ works is -Wshadow=local. At first glace, all the warnings look solvable, but then one will eventually discover __wait_event() and associated macros that mix when and how deeply it intentionally shadows variables. :) Another way to try to catch misused shadow variables is -Wunused-but-set-varible, but it, too, has tons of false positives. I tried to capture some of the rationale and research here: https://github.com/KSPP/linux/issues/152 -- Kees Cook
Re: [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
On Tue, Mar 01, 2022 at 12:28:15PM +0100, Jakob Koschel wrote: > Based on the coccinelle script there are ~480 cases that need fixing > in total. I'll now finish all of them and then split them by > submodules as Greg suggested and repost a patch set per submodule. > Sounds good? To help with this splitting, see: https://github.com/kees/kernel-tools/blob/trunk/split-on-maintainer It's not perfect, but it'll get you really close. For example, if you had a single big tree-wide patch applied to your tree: $ rm 0*.patch $ git format-patch -1 HEAD $ mv 0*.patch treewide.patch $ split-on-maintainer treewide.patch $ ls 0*.patch If you have a build log before the patch that spits out warnings, the --build-log argument can extract those warnings on a per-file basis, too (though this can be fragile). -- Kees Cook
[PATCH v3] drm/amd/pm: And destination bounds checking to struct copy
In preparation for FORTIFY_SOURCE performing compile-time and run-time
field bounds checking for memcpy(), memmove(), and memset(), avoid
intentionally writing across neighboring fields.
The "Board Parameters" members of the structs:
struct atom_smc_dpm_info_v4_5
struct atom_smc_dpm_info_v4_6
struct atom_smc_dpm_info_v4_7
struct atom_smc_dpm_info_v4_10
are written to the corresponding members of the corresponding PPTable_t
variables, but they lack destination size bounds checking, which means
the compiler cannot verify at compile time that this is an intended and
safe memcpy().
Since the header files are effectively immutable[1] and a struct_group()
cannot be used, nor a common struct referenced by both sides of the
memcpy() arguments, add a new helper, amdgpu_memcpy_trailing(), to
perform the bounds checking at compile time. Replace the open-coded
memcpy()s with amdgpu_memcpy_trailing() which includes enough context
for the bounds checking.
"objdump -d" shows no object code changes.
[1] https://lore.kernel.org/lkml/e56aad3c-a06f-da07-f491-a894a570d...@amd.com
Cc: "Christian König"
Cc: "Pan, Xinhui"
Cc: David Airlie
Cc: Daniel Vetter
Cc: Hawking Zhang
Cc: Feifei Xu
Cc: Likun Gao
Cc: Jiawei Gu
Cc: Evan Quan
Cc: amd-gfx@lists.freedesktop.org
Cc: dri-de...@lists.freedesktop.org
Reviewed-by: Lijo Lazar
Acked-by: Alex Deucher
Signed-off-by: Kees Cook
---
v3: rename amdgpu_memcpy_trailing() to smu_memcpy_trailing()
v2: https://lore.kernel.org/lkml/20210825161957.3904130-1-keesc...@chromium.org
v1: https://lore.kernel.org/lkml/20210819201441.3545027-1-keesc...@chromium.org
---
drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h | 24 +++
.../gpu/drm/amd/pm/swsmu/smu11/arcturus_ppt.c | 6 ++---
.../gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c | 8 +++
.../drm/amd/pm/swsmu/smu13/aldebaran_ppt.c| 5 ++--
4 files changed, 32 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
b/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
index 715b4225f5ee..8156729c370b 100644
--- a/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
+++ b/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
@@ -1335,6 +1335,30 @@ enum smu_cmn2asic_mapping_type {
#define WORKLOAD_MAP(profile, workload) \
[profile] = {1, (workload)}
+/**
+ * smu_memcpy_trailing - Copy the end of one structure into the middle of
another
+ *
+ * @dst: Pointer to destination struct
+ * @first_dst_member: The member name in @dst where the overwrite begins
+ * @last_dst_member: The member name in @dst where the overwrite ends after
+ * @src: Pointer to the source struct
+ * @first_src_member: The member name in @src where the copy begins
+ *
+ */
+#define smu_memcpy_trailing(dst, first_dst_member, last_dst_member, \
+ src, first_src_member) \
+({\
+ size_t __src_offset = offsetof(typeof(*(src)), first_src_member); \
+ size_t __src_size = sizeof(*(src)) - __src_offset; \
+ size_t __dst_offset = offsetof(typeof(*(dst)), first_dst_member); \
+ size_t __dst_size = offsetofend(typeof(*(dst)), last_dst_member) - \
+ __dst_offset; \
+ BUILD_BUG_ON(__src_size != __dst_size);\
+ __builtin_memcpy((u8 *)(dst) + __dst_offset, \
+(u8 *)(src) + __src_offset, \
+__dst_size); \
+})
+
#if !defined(SWSMU_CODE_LAYER_L2) && !defined(SWSMU_CODE_LAYER_L3) &&
!defined(SWSMU_CODE_LAYER_L4)
int smu_get_power_limit(void *handle,
uint32_t *limit,
diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu11/arcturus_ppt.c
b/drivers/gpu/drm/amd/pm/swsmu/smu11/arcturus_ppt.c
index 273df66cac14..e343cc218990 100644
--- a/drivers/gpu/drm/amd/pm/swsmu/smu11/arcturus_ppt.c
+++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/arcturus_ppt.c
@@ -483,10 +483,8 @@ static int arcturus_append_powerplay_table(struct
smu_context *smu)
if ((smc_dpm_table->table_header.format_revision == 4) &&
(smc_dpm_table->table_header.content_revision == 6))
- memcpy(_pptable->MaxVoltageStepGfx,
- _dpm_table->maxvoltagestepgfx,
- sizeof(*smc_dpm_table) - offsetof(struct
atom_smc_dpm_info_v4_6, maxvoltagestepgfx));
-
+ smu_memcpy_trailing(smc_pptable, MaxVoltageStepGfx,
BoardReserved,
+ smc_dpm_table, maxvoltagestepgfx);
return 0;
}
diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c
b/drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c
index f96681700c41..a5fc5d7cb6c7 100644
--- a/drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_pp
Re: [PATCH v2] drm/amd/pm: And destination bounds checking to struct copy
On Thu, Aug 26, 2021 at 03:51:29PM -0400, Alex Deucher wrote:
> On Wed, Aug 25, 2021 at 12:20 PM Kees Cook wrote:
> >
> > In preparation for FORTIFY_SOURCE performing compile-time and run-time
> > field bounds checking for memcpy(), memmove(), and memset(), avoid
> > intentionally writing across neighboring fields.
> >
> > The "Board Parameters" members of the structs:
> > struct atom_smc_dpm_info_v4_5
> > struct atom_smc_dpm_info_v4_6
> > struct atom_smc_dpm_info_v4_7
> > struct atom_smc_dpm_info_v4_10
> > are written to the corresponding members of the corresponding PPTable_t
> > variables, but they lack destination size bounds checking, which means
> > the compiler cannot verify at compile time that this is an intended and
> > safe memcpy().
> >
> > Since the header files are effectively immutable[1] and a struct_group()
> > cannot be used, nor a common struct referenced by both sides of the
> > memcpy() arguments, add a new helper, amdgpu_memcpy_trailing(), to
> > perform the bounds checking at compile time. Replace the open-coded
> > memcpy()s with amdgpu_memcpy_trailing() which includes enough context
> > for the bounds checking.
> >
> > "objdump -d" shows no object code changes.
> >
> > [1]
> > https://lore.kernel.org/lkml/e56aad3c-a06f-da07-f491-a894a570d...@amd.com
> >
> > Cc: "Christian König"
> > Cc: "Pan, Xinhui"
> > Cc: David Airlie
> > Cc: Daniel Vetter
> > Cc: Hawking Zhang
> > Cc: Feifei Xu
> > Cc: Likun Gao
> > Cc: Jiawei Gu
> > Cc: Evan Quan
> > Cc: amd-gfx@lists.freedesktop.org
> > Cc: dri-de...@lists.freedesktop.org
> > Reviewed-by: Lijo Lazar
> > Acked-by: Alex Deucher
> > Signed-off-by: Kees Cook
> > ---
> > v2:
> > - rename and move helper to drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
> > - add reviews/acks
> > v1:
> > https://lore.kernel.org/lkml/20210819201441.3545027-1-keesc...@chromium.org/
> > ---
> > drivers/gpu/drm/amd/amdgpu/amdgpu.h | 1 +
> > drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h | 24 +++
> > .../gpu/drm/amd/pm/swsmu/smu11/arcturus_ppt.c | 6 ++---
> > .../gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c | 8 +++
> > .../drm/amd/pm/swsmu/smu13/aldebaran_ppt.c| 5 ++--
> > 5 files changed, 33 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> > index dc3c6b3a00e5..c911387045e2 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> > @@ -1452,4 +1452,5 @@ static inline int amdgpu_in_reset(struct
> > amdgpu_device *adev)
> > {
> > return atomic_read(>in_gpu_reset);
> > }
> > +
> > #endif
> > diff --git a/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
> > b/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
> > index 715b4225f5ee..29031eb11d39 100644
> > --- a/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
> > +++ b/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
> > @@ -1335,6 +1335,30 @@ enum smu_cmn2asic_mapping_type {
> > #define WORKLOAD_MAP(profile, workload) \
> > [profile] = {1, (workload)}
> >
> > +/**
> > + * amdgpu_memcpy_trailing - Copy the end of one structure into the middle
> > of another
> > + *
> > + * @dst: Pointer to destination struct
> > + * @first_dst_member: The member name in @dst where the overwrite begins
> > + * @last_dst_member: The member name in @dst where the overwrite ends after
> > + * @src: Pointer to the source struct
> > + * @first_src_member: The member name in @src where the copy begins
> > + *
> > + */
> > +#define amdgpu_memcpy_trailing(dst, first_dst_member, last_dst_member,\
>
> I would change this to smu_memcpy_trailing() for consistency. Other
Sure; I will send a v3.
> than that, it the patch looks good to me. Did you want me to pick
> this up or do you want to keep this with the other patches in your
> series?
Since this has no external dependencies, it's probably best to go via
your tree.
Thanks!
-Kees
>
> Thanks!
>
> Alex
>
> > + src, first_src_member) \
> > +({\
> > + size_t __src_offset = offsetof(typeof(*(src)), first_src_member); \
> > + size_t __src_size = sizeof(*(src)) - __src_offset; \
> > + size_t __dst_offset = o
[PATCH v2] drm/amd/pm: And destination bounds checking to struct copy
In preparation for FORTIFY_SOURCE performing compile-time and run-time
field bounds checking for memcpy(), memmove(), and memset(), avoid
intentionally writing across neighboring fields.
The "Board Parameters" members of the structs:
struct atom_smc_dpm_info_v4_5
struct atom_smc_dpm_info_v4_6
struct atom_smc_dpm_info_v4_7
struct atom_smc_dpm_info_v4_10
are written to the corresponding members of the corresponding PPTable_t
variables, but they lack destination size bounds checking, which means
the compiler cannot verify at compile time that this is an intended and
safe memcpy().
Since the header files are effectively immutable[1] and a struct_group()
cannot be used, nor a common struct referenced by both sides of the
memcpy() arguments, add a new helper, amdgpu_memcpy_trailing(), to
perform the bounds checking at compile time. Replace the open-coded
memcpy()s with amdgpu_memcpy_trailing() which includes enough context
for the bounds checking.
"objdump -d" shows no object code changes.
[1] https://lore.kernel.org/lkml/e56aad3c-a06f-da07-f491-a894a570d...@amd.com
Cc: "Christian König"
Cc: "Pan, Xinhui"
Cc: David Airlie
Cc: Daniel Vetter
Cc: Hawking Zhang
Cc: Feifei Xu
Cc: Likun Gao
Cc: Jiawei Gu
Cc: Evan Quan
Cc: amd-gfx@lists.freedesktop.org
Cc: dri-de...@lists.freedesktop.org
Reviewed-by: Lijo Lazar
Acked-by: Alex Deucher
Signed-off-by: Kees Cook
---
v2:
- rename and move helper to drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
- add reviews/acks
v1: https://lore.kernel.org/lkml/20210819201441.3545027-1-keesc...@chromium.org/
---
drivers/gpu/drm/amd/amdgpu/amdgpu.h | 1 +
drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h | 24 +++
.../gpu/drm/amd/pm/swsmu/smu11/arcturus_ppt.c | 6 ++---
.../gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c | 8 +++
.../drm/amd/pm/swsmu/smu13/aldebaran_ppt.c| 5 ++--
5 files changed, 33 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index dc3c6b3a00e5..c911387045e2 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -1452,4 +1452,5 @@ static inline int amdgpu_in_reset(struct amdgpu_device
*adev)
{
return atomic_read(>in_gpu_reset);
}
+
#endif
diff --git a/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
b/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
index 715b4225f5ee..29031eb11d39 100644
--- a/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
+++ b/drivers/gpu/drm/amd/pm/inc/amdgpu_smu.h
@@ -1335,6 +1335,30 @@ enum smu_cmn2asic_mapping_type {
#define WORKLOAD_MAP(profile, workload) \
[profile] = {1, (workload)}
+/**
+ * amdgpu_memcpy_trailing - Copy the end of one structure into the middle of
another
+ *
+ * @dst: Pointer to destination struct
+ * @first_dst_member: The member name in @dst where the overwrite begins
+ * @last_dst_member: The member name in @dst where the overwrite ends after
+ * @src: Pointer to the source struct
+ * @first_src_member: The member name in @src where the copy begins
+ *
+ */
+#define amdgpu_memcpy_trailing(dst, first_dst_member, last_dst_member,\
+ src, first_src_member) \
+({\
+ size_t __src_offset = offsetof(typeof(*(src)), first_src_member); \
+ size_t __src_size = sizeof(*(src)) - __src_offset; \
+ size_t __dst_offset = offsetof(typeof(*(dst)), first_dst_member); \
+ size_t __dst_size = offsetofend(typeof(*(dst)), last_dst_member) - \
+ __dst_offset; \
+ BUILD_BUG_ON(__src_size != __dst_size);\
+ __builtin_memcpy((u8 *)(dst) + __dst_offset, \
+(u8 *)(src) + __src_offset, \
+__dst_size); \
+})
+
#if !defined(SWSMU_CODE_LAYER_L2) && !defined(SWSMU_CODE_LAYER_L3) &&
!defined(SWSMU_CODE_LAYER_L4)
int smu_get_power_limit(void *handle,
uint32_t *limit,
diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu11/arcturus_ppt.c
b/drivers/gpu/drm/amd/pm/swsmu/smu11/arcturus_ppt.c
index 273df66cac14..bda8fc12c91f 100644
--- a/drivers/gpu/drm/amd/pm/swsmu/smu11/arcturus_ppt.c
+++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/arcturus_ppt.c
@@ -483,10 +483,8 @@ static int arcturus_append_powerplay_table(struct
smu_context *smu)
if ((smc_dpm_table->table_header.format_revision == 4) &&
(smc_dpm_table->table_header.content_revision == 6))
- memcpy(_pptable->MaxVoltageStepGfx,
- _dpm_table->maxvoltagestepgfx,
- sizeof(*smc_dpm_table) - offsetof(struct
atom_smc_dpm_info_v4_6, maxvoltagestepgfx));
-
+
Re: [PATCH] drm/amd/pm: And destination bounds checking to struct copy
On August 22, 2021 11:28:54 PM PDT, "Christian König"
wrote:
>
>
>Am 19.08.21 um 22:14 schrieb Kees Cook:
>> [...]
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>> index 96e895d6be35..4605934a4fb7 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>> @@ -1446,4 +1446,29 @@ static inline int amdgpu_in_reset(struct
>> amdgpu_device *adev)
>> {
>> return atomic_read(>in_gpu_reset);
>> }
>> +
>> +/**
>> + * memcpy_trailing - Copy the end of one structure into the middle of
>> another
>> + *
>> + * @dst: Pointer to destination struct
>> + * @first_dst_member: The member name in @dst where the overwrite begins
>> + * @last_dst_member: The member name in @dst where the overwrite ends after
>> + * @src: Pointer to the source struct
>> + * @first_src_member: The member name in @src where the copy begins
>> + *
>> + */
>> +#define memcpy_trailing(dst, first_dst_member, last_dst_member,
>>\
>> +src, first_src_member) \
>
>Please don't add a function like this into amdgpu.h, especially when it
>is only used by the SMU code.
Sure, I'm happy to move it. It wasn't clear to me which headers were considered
"immutable". Which header should I put this in?
>And please give it an amdgpu_ prefix so that we are not confusing it
>with a core function.
Sure, I will include that.
>Apart from that looks good to me.
Thanks!
-Kees
Re: [PATCH v2 18/63] drm/amd/pm: Use struct_group() for memcpy() region
On Thu, Aug 19, 2021 at 10:33:43AM +0530, Lazar, Lijo wrote:
> On 8/19/2021 5:29 AM, Kees Cook wrote:
> > On Wed, Aug 18, 2021 at 05:12:28PM +0530, Lazar, Lijo wrote:
> > >
> > > On 8/18/2021 11:34 AM, Kees Cook wrote:
> > > > In preparation for FORTIFY_SOURCE performing compile-time and run-time
> > > > field bounds checking for memcpy(), memmove(), and memset(), avoid
> > > > intentionally writing across neighboring fields.
> > > >
> > > > Use struct_group() in structs:
> > > > struct atom_smc_dpm_info_v4_5
> > > > struct atom_smc_dpm_info_v4_6
> > > > struct atom_smc_dpm_info_v4_7
> > > > struct atom_smc_dpm_info_v4_10
> > > > PPTable_t
> > > > so the grouped members can be referenced together. This will allow
> > > > memcpy() and sizeof() to more easily reason about sizes, improve
> > > > readability, and avoid future warnings about writing beyond the end of
> > > > the first member.
> > > >
> > > > "pahole" shows no size nor member offset changes to any structs.
> > > > "objdump -d" shows no object code changes.
> > > >
> > > > Cc: "Christian König"
> > > > Cc: "Pan, Xinhui"
> > > > Cc: David Airlie
> > > > Cc: Daniel Vetter
> > > > Cc: Hawking Zhang
> > > > Cc: Feifei Xu
> > > > Cc: Lijo Lazar
> > > > Cc: Likun Gao
> > > > Cc: Jiawei Gu
> > > > Cc: Evan Quan
> > > > Cc: amd-gfx@lists.freedesktop.org
> > > > Cc: dri-de...@lists.freedesktop.org
> > > > Signed-off-by: Kees Cook
> > > > Acked-by: Alex Deucher
> > > > Link:
> > > > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Flkml%2FCADnq5_Npb8uYvd%2BR4UHgf-w8-cQj3JoODjviJR_Y9w9wqJ71mQ%40mail.gmail.comdata=04%7C01%7Clijo.lazar%40amd.com%7C3861f20094074bf7328808d962a433f2%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637649279701053991%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=386LcfJJGfQfHsXBuK17LMqxJ2nFtGoj%2FUjoN2ZtJd0%3Dreserved=0
> > > > ---
> > > >drivers/gpu/drm/amd/include/atomfirmware.h | 9 -
> > > >.../gpu/drm/amd/pm/inc/smu11_driver_if_arcturus.h| 3 ++-
> > > >drivers/gpu/drm/amd/pm/inc/smu11_driver_if_navi10.h | 3 ++-
> > > >.../gpu/drm/amd/pm/inc/smu13_driver_if_aldebaran.h | 3 ++-
> > >
> > > Hi Kees,
> >
> > Hi! Thanks for looking into this.
> >
> > > The headers which define these structs are firmware/VBIOS interfaces and
> > > are
> > > picked directly from those components. There are difficulties in grouping
> > > them to structs at the original source as that involves other component
> > > changes.
> >
> > So, can you help me understand this a bit more? It sounds like these are
> > generated headers, yes? I'd like to understand your constraints and
> > weight them against various benefits that could be achieved here.
> >
> > The groupings I made do appear to be roughly documented already,
> > for example:
> >
> > struct atom_common_table_header table_header;
> > // SECTION: BOARD PARAMETERS
> > + struct_group(dpm_info,
> >
> > Something emitted the "BOARD PARAMETERS" section heading as a comment,
> > so it likely also would know where it ends, yes? The good news here is
> > that for the dpm_info groups, they all end at the end of the existing
> > structs, see:
> > struct atom_smc_dpm_info_v4_5
> > struct atom_smc_dpm_info_v4_6
> > struct atom_smc_dpm_info_v4_7
> > struct atom_smc_dpm_info_v4_10
> >
> > The matching regions in the PPTable_t structs are similarly marked with a
> > "BOARD PARAMETERS" section heading comment:
> >
> > --- a/drivers/gpu/drm/amd/pm/inc/smu11_driver_if_arcturus.h
> > +++ b/drivers/gpu/drm/amd/pm/inc/smu11_driver_if_arcturus.h
> > @@ -643,6 +643,7 @@ typedef struct {
> > // SECTION: BOARD PARAMETERS
> > // SVI2 Board Parameters
> > + struct_group(v4_6,
> > uint16_t MaxVoltageStepGfx; // In mV(Q2) Max voltage step that SMU
> > will request. Multiple steps are taken if voltage change exceeds this value.
> > uint16_t MaxVoltageStepSoc; // In mV(Q2) Max voltage step that SMU
> > will request. Multiple steps are taken if vol
[PATCH] drm/amd/pm: And destination bounds checking to struct copy
In preparation for FORTIFY_SOURCE performing compile-time and run-time
field bounds checking for memcpy(), memmove(), and memset(), avoid
intentionally writing across neighboring fields.
The "Board Parameters" members of the structs:
struct atom_smc_dpm_info_v4_5
struct atom_smc_dpm_info_v4_6
struct atom_smc_dpm_info_v4_7
struct atom_smc_dpm_info_v4_10
are written to the corresponding members of the corresponding PPTable_t
variables, but they lack destination size bounds checking, which means
the compiler cannot verify at compile time that this is an intended and
safe memcpy().
Since the header files are effectively immutable[1] and a struct_group()
cannot be used, nor a common struct referenced by both sides of the
memcpy() arguments, add a new helper, memcpy_trailing(), to perform the
bounds checking at compile time. Replace the open-coded memcpy()s with
memcpy_trailing() which includes enough context for the bounds checking.
"objdump -d" shows no object code changes.
[1] https://lore.kernel.org/lkml/e56aad3c-a06f-da07-f491-a894a570d...@amd.com
Cc: Lijo Lazar
Cc: "Christian König"
Cc: "Pan, Xinhui"
Cc: David Airlie
Cc: Daniel Vetter
Cc: Hawking Zhang
Cc: Feifei Xu
Cc: Likun Gao
Cc: Jiawei Gu
Cc: Evan Quan
Cc: amd-gfx@lists.freedesktop.org
Cc: dri-de...@lists.freedesktop.org
Signed-off-by: Kees Cook
Link:
https://lore.kernel.org/lkml/cadnq5_npb8uyvd+r4uhgf-w8-cqj3joodjvijr_y9w9wqj7...@mail.gmail.com
---
Alex, I dropped your prior Acked-by, since the implementation is very
different. If you're still happy with it, I can add it back. :)
---
drivers/gpu/drm/amd/amdgpu/amdgpu.h | 25 +++
.../gpu/drm/amd/pm/swsmu/smu11/arcturus_ppt.c | 6 ++---
.../gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c | 8 +++---
.../drm/amd/pm/swsmu/smu13/aldebaran_ppt.c| 5 ++--
4 files changed, 33 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index 96e895d6be35..4605934a4fb7 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -1446,4 +1446,29 @@ static inline int amdgpu_in_reset(struct amdgpu_device
*adev)
{
return atomic_read(>in_gpu_reset);
}
+
+/**
+ * memcpy_trailing - Copy the end of one structure into the middle of another
+ *
+ * @dst: Pointer to destination struct
+ * @first_dst_member: The member name in @dst where the overwrite begins
+ * @last_dst_member: The member name in @dst where the overwrite ends after
+ * @src: Pointer to the source struct
+ * @first_src_member: The member name in @src where the copy begins
+ *
+ */
+#define memcpy_trailing(dst, first_dst_member, last_dst_member,
\
+ src, first_src_member) \
+({\
+ size_t __src_offset = offsetof(typeof(*(src)), first_src_member); \
+ size_t __src_size = sizeof(*(src)) - __src_offset; \
+ size_t __dst_offset = offsetof(typeof(*(dst)), first_dst_member); \
+ size_t __dst_size = offsetofend(typeof(*(dst)), last_dst_member) - \
+ __dst_offset; \
+ BUILD_BUG_ON(__src_size != __dst_size);\
+ __builtin_memcpy((u8 *)(dst) + __dst_offset, \
+(u8 *)(src) + __src_offset, \
+__dst_size); \
+})
+
#endif
diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu11/arcturus_ppt.c
b/drivers/gpu/drm/amd/pm/swsmu/smu11/arcturus_ppt.c
index 8ab58781ae13..1918e6232319 100644
--- a/drivers/gpu/drm/amd/pm/swsmu/smu11/arcturus_ppt.c
+++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/arcturus_ppt.c
@@ -465,10 +465,8 @@ static int arcturus_append_powerplay_table(struct
smu_context *smu)
if ((smc_dpm_table->table_header.format_revision == 4) &&
(smc_dpm_table->table_header.content_revision == 6))
- memcpy(_pptable->MaxVoltageStepGfx,
- _dpm_table->maxvoltagestepgfx,
- sizeof(*smc_dpm_table) - offsetof(struct
atom_smc_dpm_info_v4_6, maxvoltagestepgfx));
-
+ memcpy_trailing(smc_pptable, MaxVoltageStepGfx, BoardReserved,
+ smc_dpm_table, maxvoltagestepgfx);
return 0;
}
diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c
b/drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c
index 2e5d3669652b..b738042e064d 100644
--- a/drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c
+++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c
@@ -431,16 +431,16 @@ static int navi10_append_powerplay_table(struct
smu_context *smu)
switch (smc_dpm_table->table_header.content_revision) {
case 5: /* n
Re: [PATCH v2 18/63] drm/amd/pm: Use struct_group() for memcpy() region
On Wed, Aug 18, 2021 at 05:12:28PM +0530, Lazar, Lijo wrote:
>
> On 8/18/2021 11:34 AM, Kees Cook wrote:
> > In preparation for FORTIFY_SOURCE performing compile-time and run-time
> > field bounds checking for memcpy(), memmove(), and memset(), avoid
> > intentionally writing across neighboring fields.
> >
> > Use struct_group() in structs:
> > struct atom_smc_dpm_info_v4_5
> > struct atom_smc_dpm_info_v4_6
> > struct atom_smc_dpm_info_v4_7
> > struct atom_smc_dpm_info_v4_10
> > PPTable_t
> > so the grouped members can be referenced together. This will allow
> > memcpy() and sizeof() to more easily reason about sizes, improve
> > readability, and avoid future warnings about writing beyond the end of
> > the first member.
> >
> > "pahole" shows no size nor member offset changes to any structs.
> > "objdump -d" shows no object code changes.
> >
> > Cc: "Christian König"
> > Cc: "Pan, Xinhui"
> > Cc: David Airlie
> > Cc: Daniel Vetter
> > Cc: Hawking Zhang
> > Cc: Feifei Xu
> > Cc: Lijo Lazar
> > Cc: Likun Gao
> > Cc: Jiawei Gu
> > Cc: Evan Quan
> > Cc: amd-gfx@lists.freedesktop.org
> > Cc: dri-de...@lists.freedesktop.org
> > Signed-off-by: Kees Cook
> > Acked-by: Alex Deucher
> > Link:
> > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Flkml%2FCADnq5_Npb8uYvd%2BR4UHgf-w8-cQj3JoODjviJR_Y9w9wqJ71mQ%40mail.gmail.comdata=04%7C01%7Clijo.lazar%40amd.com%7C92b8d2f072f0444b9f8508d9620f6971%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637648640625729624%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=rKh5LUXCRUsorYM3kSpG2tkB%2Fczwl9I9EBnWBCtbg6Q%3Dreserved=0
> > ---
> > drivers/gpu/drm/amd/include/atomfirmware.h | 9 -
> > .../gpu/drm/amd/pm/inc/smu11_driver_if_arcturus.h| 3 ++-
> > drivers/gpu/drm/amd/pm/inc/smu11_driver_if_navi10.h | 3 ++-
> > .../gpu/drm/amd/pm/inc/smu13_driver_if_aldebaran.h | 3 ++-
>
> Hi Kees,
Hi! Thanks for looking into this.
> The headers which define these structs are firmware/VBIOS interfaces and are
> picked directly from those components. There are difficulties in grouping
> them to structs at the original source as that involves other component
> changes.
So, can you help me understand this a bit more? It sounds like these are
generated headers, yes? I'd like to understand your constraints and
weight them against various benefits that could be achieved here.
The groupings I made do appear to be roughly documented already,
for example:
struct atom_common_table_header table_header;
// SECTION: BOARD PARAMETERS
+ struct_group(dpm_info,
Something emitted the "BOARD PARAMETERS" section heading as a comment,
so it likely also would know where it ends, yes? The good news here is
that for the dpm_info groups, they all end at the end of the existing
structs, see:
struct atom_smc_dpm_info_v4_5
struct atom_smc_dpm_info_v4_6
struct atom_smc_dpm_info_v4_7
struct atom_smc_dpm_info_v4_10
The matching regions in the PPTable_t structs are similarly marked with a
"BOARD PARAMETERS" section heading comment:
--- a/drivers/gpu/drm/amd/pm/inc/smu11_driver_if_arcturus.h
+++ b/drivers/gpu/drm/amd/pm/inc/smu11_driver_if_arcturus.h
@@ -643,6 +643,7 @@ typedef struct {
// SECTION: BOARD PARAMETERS
// SVI2 Board Parameters
+ struct_group(v4_6,
uint16_t MaxVoltageStepGfx; // In mV(Q2) Max voltage step that SMU will
request. Multiple steps are taken if voltage change exceeds this value.
uint16_t MaxVoltageStepSoc; // In mV(Q2) Max voltage step that SMU will
request. Multiple steps are taken if voltage change exceeds this value.
@@ -728,10 +729,10 @@ typedef struct {
uint32_t BoardVoltageCoeffB;// decode by /1000
uint32_t BoardReserved[7];
+ );
// Padding for MMHUB - do not modify this
uint32_t MmHubPadding[8]; // SMU internal use
-
} PPTable_t;
Where they end seems known as well (the padding switches from a "Board"
to "MmHub" prefix at exactly the matching size).
So, given that these regions are already known by the export tool, how
about just updating the export tool to emit a struct there? I imagine
the problem with this would be the identifier churn needed, but that's
entirely mechanical.
However, I'm curious about another aspect of these regions: they are,
by definition, the same. Why isn't there a single struct describing
them already, given the existing redundancy? For example, look at the
member names: maxvoltagestepgfx vs MaxVoltageStepGfx. Why aren't these
the same? And then why aren't they descr
[PATCH v2 18/63] drm/amd/pm: Use struct_group() for memcpy() region
In preparation for FORTIFY_SOURCE performing compile-time and run-time
field bounds checking for memcpy(), memmove(), and memset(), avoid
intentionally writing across neighboring fields.
Use struct_group() in structs:
struct atom_smc_dpm_info_v4_5
struct atom_smc_dpm_info_v4_6
struct atom_smc_dpm_info_v4_7
struct atom_smc_dpm_info_v4_10
PPTable_t
so the grouped members can be referenced together. This will allow
memcpy() and sizeof() to more easily reason about sizes, improve
readability, and avoid future warnings about writing beyond the end of
the first member.
"pahole" shows no size nor member offset changes to any structs.
"objdump -d" shows no object code changes.
Cc: "Christian König"
Cc: "Pan, Xinhui"
Cc: David Airlie
Cc: Daniel Vetter
Cc: Hawking Zhang
Cc: Feifei Xu
Cc: Lijo Lazar
Cc: Likun Gao
Cc: Jiawei Gu
Cc: Evan Quan
Cc: amd-gfx@lists.freedesktop.org
Cc: dri-de...@lists.freedesktop.org
Signed-off-by: Kees Cook
Acked-by: Alex Deucher
Link:
https://lore.kernel.org/lkml/cadnq5_npb8uyvd+r4uhgf-w8-cqj3joodjvijr_y9w9wqj7...@mail.gmail.com
---
drivers/gpu/drm/amd/include/atomfirmware.h | 9 -
.../gpu/drm/amd/pm/inc/smu11_driver_if_arcturus.h| 3 ++-
drivers/gpu/drm/amd/pm/inc/smu11_driver_if_navi10.h | 3 ++-
.../gpu/drm/amd/pm/inc/smu13_driver_if_aldebaran.h | 3 ++-
drivers/gpu/drm/amd/pm/swsmu/smu11/arcturus_ppt.c| 6 +++---
drivers/gpu/drm/amd/pm/swsmu/smu11/navi10_ppt.c | 12
drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c | 6 +++---
7 files changed, 28 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/amd/include/atomfirmware.h
b/drivers/gpu/drm/amd/include/atomfirmware.h
index 44955458fe38..7bf3edf15410 100644
--- a/drivers/gpu/drm/amd/include/atomfirmware.h
+++ b/drivers/gpu/drm/amd/include/atomfirmware.h
@@ -2081,6 +2081,7 @@ struct atom_smc_dpm_info_v4_5
{
struct atom_common_table_header table_header;
// SECTION: BOARD PARAMETERS
+ struct_group(dpm_info,
// I2C Control
struct smudpm_i2c_controller_config_v2 I2cControllers[8];
@@ -2159,7 +2160,7 @@ struct atom_smc_dpm_info_v4_5
uint32_t MvddRatio; // This is used for MVDD Vid workaround. It has 16
fractional bits (Q16.16)
uint32_t BoardReserved[9];
-
+ );
};
struct atom_smc_dpm_info_v4_6
@@ -2168,6 +2169,7 @@ struct atom_smc_dpm_info_v4_6
// section: board parameters
uint32_t i2c_padding[3]; // old i2c control are moved to new area
+ struct_group(dpm_info,
uint16_t maxvoltagestepgfx; // in mv(q2) max voltage step that smu will
request. multiple steps are taken if voltage change exceeds this value.
uint16_t maxvoltagestepsoc; // in mv(q2) max voltage step that smu will
request. multiple steps are taken if voltage change exceeds this value.
@@ -2246,12 +2248,14 @@ struct atom_smc_dpm_info_v4_6
// reserved
uint32_t boardreserved[10];
+ );
};
struct atom_smc_dpm_info_v4_7
{
struct atom_common_table_header table_header;
// SECTION: BOARD PARAMETERS
+ struct_group(dpm_info,
// I2C Control
struct smudpm_i2c_controller_config_v2 I2cControllers[8];
@@ -2348,6 +2352,7 @@ struct atom_smc_dpm_info_v4_7
uint8_t Padding8_Psi2;
uint32_t BoardReserved[5];
+ );
};
struct smudpm_i2c_controller_config_v3
@@ -2478,6 +2483,7 @@ struct atom_smc_dpm_info_v4_10
struct atom_common_table_header table_header;
// SECTION: BOARD PARAMETERS
+ struct_group(dpm_info,
// Telemetry Settings
uint16_t GfxMaxCurrent; // in Amps
uint8_t GfxOffset; // in Amps
@@ -2524,6 +2530,7 @@ struct atom_smc_dpm_info_v4_10
uint16_t spare5;
uint32_t reserved[16];
+ );
};
/*
diff --git a/drivers/gpu/drm/amd/pm/inc/smu11_driver_if_arcturus.h
b/drivers/gpu/drm/amd/pm/inc/smu11_driver_if_arcturus.h
index 43d43d6addc0..8093a98800c3 100644
--- a/drivers/gpu/drm/amd/pm/inc/smu11_driver_if_arcturus.h
+++ b/drivers/gpu/drm/amd/pm/inc/smu11_driver_if_arcturus.h
@@ -643,6 +643,7 @@ typedef struct {
// SECTION: BOARD PARAMETERS
// SVI2 Board Parameters
+ struct_group(v4_6,
uint16_t MaxVoltageStepGfx; // In mV(Q2) Max voltage step that SMU will
request. Multiple steps are taken if voltage change exceeds this value.
uint16_t MaxVoltageStepSoc; // In mV(Q2) Max voltage step that SMU will
request. Multiple steps are taken if voltage change exceeds this value.
@@ -728,10 +729,10 @@ typedef struct {
uint32_t BoardVoltageCoeffB;// decode by /1000
uint32_t BoardReserved[7];
+ );
// Padding for MMHUB - do not modify this
uint32_t MmHubPadding[8]; // SMU internal use
-
} PPTable_t;
typedef struct {
diff --git a/drivers/gpu/drm/amd/pm/inc/smu11_driver_if_navi10.h
b/drivers/gpu/drm/amd/pm/inc/smu11_driver_if_navi10.h
index 04752ade1016..0b4e6e907e95 100644
--- a/drivers/gpu/drm/amd/pm/inc/smu11_driver_if_n
[PATCH] drm/amd/display: Avoid HDCP over-read and corruption
Instead of reading the desired 5 bytes of the actual target field,
the code was reading 8. This could result in a corrupted value if the
trailing 3 bytes were non-zero, so instead use an appropriately sized
and zero-initialized bounce buffer, and read only 5 bytes before casting
to u64.
Signed-off-by: Kees Cook
---
drivers/gpu/drm/amd/display/modules/hdcp/hdcp1_execution.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/display/modules/hdcp/hdcp1_execution.c
b/drivers/gpu/drm/amd/display/modules/hdcp/hdcp1_execution.c
index 2cbd931363bd..6d26d9c63ab2 100644
--- a/drivers/gpu/drm/amd/display/modules/hdcp/hdcp1_execution.c
+++ b/drivers/gpu/drm/amd/display/modules/hdcp/hdcp1_execution.c
@@ -29,8 +29,10 @@ static inline enum mod_hdcp_status validate_bksv(struct
mod_hdcp *hdcp)
{
uint64_t n = 0;
uint8_t count = 0;
+ u8 bksv[sizeof(n)] = { };
- memcpy(, hdcp->auth.msg.hdcp1.bksv, sizeof(uint64_t));
+ memcpy(bksv, hdcp->auth.msg.hdcp1.bksv,
sizeof(hdcp->auth.msg.hdcp1.bksv));
+ n = *(uint64_t *)bksv;
while (n) {
count++;
--
2.25.1
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH] drm/amdgpu: Add vbios info ioctl interface
Hi!
This patch needs some fixing.
On Thu, Apr 22, 2021 at 10:34:48AM +0800, Jiawei Gu wrote:
> + case AMDGPU_INFO_VBIOS_INFO: {
> + struct drm_amdgpu_info_vbios vbios_info = {};
> + struct atom_context *atom_context;
> +
> + atom_context = adev->mode_info.atom_context;
> + memcpy(vbios_info.name, atom_context->name,
> sizeof(atom_context->name));
> + vbios_info.dbdf = PCI_DEVID(adev->pdev->bus->number,
> adev->pdev->devfn);
> + memcpy(vbios_info.vbios_pn, atom_context->vbios_pn,
> sizeof(atom_context->vbios_pn));
> + vbios_info.version = atom_context->version;
> + memcpy(vbios_info.date, atom_context->date,
> sizeof(atom_context->date));
> + memcpy(vbios_info.serial, adev->serial,
> sizeof(adev->serial));
This writes beyond the end of vbios_info.serial.
> + vbios_info.dev_id = adev->pdev->device;
> + vbios_info.rev_id = adev->pdev->revision;
> + vbios_info.sub_dev_id = atom_context->sub_dev_id;
> + vbios_info.sub_ved_id = atom_context->sub_ved_id;
Though it gets "repaired" by these writes.
> +
> + return copy_to_user(out, _info,
> + min((size_t)size,
> sizeof(vbios_info))) ? -EFAULT : 0;
> + }
sizeof(adev->serial) != sizeof(vbios_info.serial)
adev is struct amdgpu_device:
struct amdgpu_device {
...
charserial[20];
> +struct drm_amdgpu_info_vbios {
> [...]
> + __u8 serial[16];
> + __u32 dev_id;
> + __u32 rev_id;
> + __u32 sub_dev_id;
> + __u32 sub_ved_id;
> +};
Is there a truncation issue (20 vs 16) and is this intended to be a
NUL-terminated string?
--
Kees Cook
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH] drm/amdgpu: Add vbios info ioctl interface
On Sat, May 08, 2021 at 06:01:23AM +, Gu, JiaWei (Will) wrote:
> [AMD Official Use Only - Internal Distribution Only]
>
> Thanks for this catching Kees.
>
> Yes it should be 20, not 16. I was not aware that serial size had been
> changed from 16 to 20 in struct amdgpu_device.
> Will submit a fix soon.
You might want to add a BUILD_BUG_ON() to keep those in sync, especially
since it's about to be UAPI.
-Kees
>
> Best regards,
> Jiawei
>
>
> -Original Message-
> From: Kees Cook
> Sent: Saturday, May 8, 2021 12:28 PM
> To: Gu, JiaWei (Will) ; Deucher, Alexander
>
> Cc: StDenis, Tom ; Deucher, Alexander
> ; Christian König
> ; Gu, JiaWei (Will) ;
> amd-gfx@lists.freedesktop.org; Nieto, David M ;
> linux-n...@vger.kernel.org
> Subject: Re: [PATCH] drm/amdgpu: Add vbios info ioctl interface
>
> Hi!
>
> This patch needs some fixing.
>
> On Thu, Apr 22, 2021 at 10:34:48AM +0800, Jiawei Gu wrote:
> > + case AMDGPU_INFO_VBIOS_INFO: {
> > + struct drm_amdgpu_info_vbios vbios_info = {};
> > + struct atom_context *atom_context;
> > +
> > + atom_context = adev->mode_info.atom_context;
> > + memcpy(vbios_info.name, atom_context->name,
> > sizeof(atom_context->name));
> > + vbios_info.dbdf = PCI_DEVID(adev->pdev->bus->number,
> > adev->pdev->devfn);
> > + memcpy(vbios_info.vbios_pn, atom_context->vbios_pn,
> > sizeof(atom_context->vbios_pn));
> > + vbios_info.version = atom_context->version;
> > + memcpy(vbios_info.date, atom_context->date,
> > sizeof(atom_context->date));
> > + memcpy(vbios_info.serial, adev->serial,
> > sizeof(adev->serial));
>
> This writes beyond the end of vbios_info.serial.
>
> > + vbios_info.dev_id = adev->pdev->device;
> > + vbios_info.rev_id = adev->pdev->revision;
> > + vbios_info.sub_dev_id = atom_context->sub_dev_id;
> > + vbios_info.sub_ved_id = atom_context->sub_ved_id;
>
> Though it gets "repaired" by these writes.
>
> > +
> > + return copy_to_user(out, _info,
> > + min((size_t)size,
> > sizeof(vbios_info))) ? -EFAULT : 0;
> > + }
>
> sizeof(adev->serial) != sizeof(vbios_info.serial)
>
> adev is struct amdgpu_device:
>
> struct amdgpu_device {
> ...
> charserial[20];
>
>
> > +struct drm_amdgpu_info_vbios {
> > [...]
> > + __u8 serial[16];
> > + __u32 dev_id;
> > + __u32 rev_id;
> > + __u32 sub_dev_id;
> > + __u32 sub_ved_id;
> > +};
>
> Is there a truncation issue (20 vs 16) and is this intended to be a
> NUL-terminated string?
>
> --
> Kees Cook
--
Kees Cook
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH 0/2] drm/radeon: Fix off-by-one power_state index heap overwrite
Hi, This is an attempt at fixing a bug[1] uncovered by the relocation of the slab freelist pointer offset, as well as some related clean-ups. I don't have hardware to do runtime testing, but it builds. ;) -Kees [1] https://bugzilla.kernel.org/show_bug.cgi?id=211537 Kees Cook (2): drm/radeon: Fix off-by-one power_state index heap overwrite drm/radeon: Avoid power table parsing memory leaks drivers/gpu/drm/radeon/radeon_atombios.c | 26 1 file changed, 18 insertions(+), 8 deletions(-) -- 2.25.1 ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH 1/2] drm/radeon: Fix off-by-one power_state index heap overwrite
An out of bounds write happens when setting the default power state.
KASAN sees this as:
[drm] radeon: 512M of GTT memory ready.
[drm] GART: num cpu pages 131072, num gpu pages 131072
==
BUG: KASAN: slab-out-of-bounds in
radeon_atombios_parse_power_table_1_3+0x1837/0x1998 [radeon]
Write of size 4 at addr 88810178d858 by task systemd-udevd/157
CPU: 0 PID: 157 Comm: systemd-udevd Not tainted 5.12.0-E620 #50
Hardware name: eMachineseMachines E620 /Nile , BIOS V1.03
09/30/2008
Call Trace:
dump_stack+0xa5/0xe6
print_address_description.constprop.0+0x18/0x239
kasan_report+0x170/0x1a8
radeon_atombios_parse_power_table_1_3+0x1837/0x1998 [radeon]
radeon_atombios_get_power_modes+0x144/0x1888 [radeon]
radeon_pm_init+0x1019/0x1904 [radeon]
rs690_init+0x76e/0x84a [radeon]
radeon_device_init+0x1c1a/0x21e5 [radeon]
radeon_driver_load_kms+0xf5/0x30b [radeon]
drm_dev_register+0x255/0x4a0 [drm]
radeon_pci_probe+0x246/0x2f6 [radeon]
pci_device_probe+0x1aa/0x294
really_probe+0x30e/0x850
driver_probe_device+0xe6/0x135
device_driver_attach+0xc1/0xf8
__driver_attach+0x13f/0x146
bus_for_each_dev+0xfa/0x146
bus_add_driver+0x2b3/0x447
driver_register+0x242/0x2c1
do_one_initcall+0x149/0x2fd
do_init_module+0x1ae/0x573
load_module+0x4dee/0x5cca
__do_sys_finit_module+0xf1/0x140
do_syscall_64+0x33/0x40
entry_SYSCALL_64_after_hwframe+0x44/0xae
Without KASAN, this will manifest later when the kernel attempts to
allocate memory that was stomped, since it collides with the inline slab
freelist pointer:
invalid opcode: [#1] SMP NOPTI
CPU: 0 PID: 781 Comm: openrc-run.sh Tainted: GW 5.10.12-gentoo-E620 #2
Hardware name: eMachineseMachines E620 /Nile , BIOS V1.03
09/30/2008
RIP: 0010:kfree+0x115/0x230
Code: 89 c5 e8 75 ea ff ff 48 8b 00 0f ba e0 09 72 63 e8 1f f4 ff ff 41 89 c4
48 8b 45 00 0f ba e0 10 72 0a 48 8b 45 08 a8 01 75 02 <0f> 0b 44 89 e1 48 c7 c2
00 f0 ff ff be 06 00 00 00 48 d3 e2 48 c7
RSP: 0018:b42f40267e10 EFLAGS: 00010246
RAX: d61280ee8d88 RBX: 0004 RCX: 801d
RDX: 4000 RSI: ba1360b0 RDI: d61280ee8d80
RBP: d61280ee8d80 R08: b91bebdf R09:
R10: 8fe2c1047ac8 R11: R12:
R13: R14: R15: 0100
FS: 7fe80eff6b68() GS:8fe339c0() knlGS:
CS: 0010 DS: ES: CR0: 80050033
CR2: 7fe80eec7bc0 CR3: 38012000 CR4: 06f0
Call Trace:
__free_fdtable+0x16/0x1f
put_files_struct+0x81/0x9b
do_exit+0x433/0x94d
do_group_exit+0xa6/0xa6
__x64_sys_exit_group+0xf/0xf
do_syscall_64+0x33/0x40
entry_SYSCALL_64_after_hwframe+0x44/0xa9
RIP: 0033:0x7fe80ef64bea
Code: Unable to access opcode bytes at RIP 0x7fe80ef64bc0.
RSP: 002b:7ffdb1c47528 EFLAGS: 0246 ORIG_RAX: 00e7
RAX: ffda RBX: 0003 RCX: 7fe80ef64bea
RDX: 7fe80ef64f60 RSI: RDI:
RBP: R08: 0001 R09:
R10: 7fe80ee2c620 R11: 0246 R12: 7fe80eff41e0
R13: R14: 0024 R15: 7fe80edf9cd0
Modules linked in: radeon(+) ath5k(+) snd_hda_codec_realtek ...
Use a valid power_state index when initializing the "flags" and "misc"
and "misc2" fields.
Reported-by: Erhard F.
Fixes: a48b9b4edb8b ("drm/radeon/kms/pm: add asic specific callbacks for
getting power state (v2)")
Fixes: 79daedc94281 ("drm/radeon/kms: minor pm cleanups")
Signed-off-by: Kees Cook
---
drivers/gpu/drm/radeon/radeon_atombios.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/radeon/radeon_atombios.c
b/drivers/gpu/drm/radeon/radeon_atombios.c
index 42301b4e56f5..f9f4efa1738c 100644
--- a/drivers/gpu/drm/radeon/radeon_atombios.c
+++ b/drivers/gpu/drm/radeon/radeon_atombios.c
@@ -2250,10 +2250,10 @@ static int radeon_atombios_parse_power_table_1_3(struct
radeon_device *rdev)
rdev->pm.default_power_state_index = state_index - 1;
rdev->pm.power_state[state_index - 1].default_clock_mode =
>pm.power_state[state_index - 1].clock_info[0];
- rdev->pm.power_state[state_index].flags &=
+ rdev->pm.power_state[state_index - 1].flags &=
~RADEON_PM_STATE_SINGLE_DISPLAY_ONLY;
- rdev->pm.power_state[state_index].misc = 0;
- rdev->pm.power_state[state_index].misc2 = 0;
+ rdev->pm.power_state[state_index - 1].misc = 0;
+ rdev->pm.power_state[state_index - 1].misc2 = 0;
}
return state_index;
}
--
2.25.1
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH 2/2] drm/radeon: Avoid power table parsing memory leaks
Avoid leaving a hanging pre-allocated clock_info if last mode is
invalid, and avoid heap corruption if no valid modes are found.
Fixes: 6991b8f2a319 ("drm/radeon/kms: fix segfault in pm rework")
Signed-off-by: Kees Cook
---
drivers/gpu/drm/radeon/radeon_atombios.c | 20 +++-
1 file changed, 15 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/radeon/radeon_atombios.c
b/drivers/gpu/drm/radeon/radeon_atombios.c
index f9f4efa1738c..28c4413f4dc8 100644
--- a/drivers/gpu/drm/radeon/radeon_atombios.c
+++ b/drivers/gpu/drm/radeon/radeon_atombios.c
@@ -2120,11 +2120,14 @@ static int radeon_atombios_parse_power_table_1_3(struct
radeon_device *rdev)
return state_index;
/* last mode is usually default, array is low to high */
for (i = 0; i < num_modes; i++) {
- rdev->pm.power_state[state_index].clock_info =
- kcalloc(1, sizeof(struct radeon_pm_clock_info),
- GFP_KERNEL);
+ /* avoid memory leaks from invalid modes or unknown frev. */
+ if (!rdev->pm.power_state[state_index].clock_info) {
+ rdev->pm.power_state[state_index].clock_info =
+ kzalloc(sizeof(struct radeon_pm_clock_info),
+ GFP_KERNEL);
+ }
if (!rdev->pm.power_state[state_index].clock_info)
- return state_index;
+ goto out;
rdev->pm.power_state[state_index].num_clock_modes = 1;
rdev->pm.power_state[state_index].clock_info[0].voltage.type =
VOLTAGE_NONE;
switch (frev) {
@@ -2243,8 +2246,15 @@ static int radeon_atombios_parse_power_table_1_3(struct
radeon_device *rdev)
break;
}
}
+out:
+ /* free any unused clock_info allocation. */
+ if (state_index && state_index < num_modes) {
+ kfree(rdev->pm.power_state[state_index].clock_info);
+ rdev->pm.power_state[state_index].clock_info = NULL;
+ }
+
/* last mode is usually default */
- if (rdev->pm.default_power_state_index == -1) {
+ if (state_index && rdev->pm.default_power_state_index == -1) {
rdev->pm.power_state[state_index - 1].type =
POWER_STATE_TYPE_DEFAULT;
rdev->pm.default_power_state_index = state_index - 1;
--
2.25.1
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [Intel-wired-lan] [PATCH 000/141] Fix fall-through warnings for Clang
On Tue, Nov 24, 2020 at 11:05:35PM -0800, James Bottomley wrote: > Now, what we have seems to be about 6 cases (at least what's been shown > in this thread) where a missing break would cause potentially user > visible issues. That means the value of this isn't zero, but it's not > a no-brainer massive win either. That's why I think asking what we've > invested vs the return isn't a useless exercise. The number is much higher[1]. If it were 6 in the entire history of the kernel, I would agree with you. :) Some were fixed _before_ Gustavo's effort too, which I also count towards the idea of "this is a dangerous weakness in C, and now we have stopped it forever." > But the broader point I'm making is just because the compiler people > come up with a shiny new warning doesn't necessarily mean the problem > it's detecting is one that causes us actual problems in the code base. > I'd really be happier if we had a theory about what classes of CVE or > bug we could eliminate before we embrace the next new warning. But we did! It was long ago justified and documented[2], and even links to the CWE[3] for it. This wasn't random joy over discovering a new warning we could turn on, this was turning on a warning that the compiler folks finally gave us to handle an entire class of flaws. If we need to update the code-base to address it not a useful debate -- that was settled already, even if you're only discovering it now. :P. This last patch set is about finishing that work for Clang, which is correctly even more strict than GCC. -Kees [1] https://outflux.net/slides/2019/lss/kspp.pdf calls out specific numbers (about 6.5% of the patches fixed missing breaks): v4.19: 3 of 129 v4.20: 2 of 59 v5.0: 3 of 56 v5.1: 10 of 100 v5.2: 6 of 71 v5.3: 7 of 69 And in the history of the kernel, it's been an ongoing source of flaws: $ l --no-merges | grep -i 'missing break' | wc -l 185 The frequency of such errors being "naturally" found was pretty steady until the static checkers started warning, and then it was on the rise, but the full effort flushed the rest out, and now it's dropped to almost zero: 1 v2.6.12 3 v2.6.16.28 1 v2.6.17 1 v2.6.19 2 v2.6.21 1 v2.6.22 3 v2.6.24 3 v2.6.29 1 v2.6.32 1 v2.6.33 1 v2.6.35 4 v2.6.36 3 v2.6.38 2 v2.6.39 7 v3.0 2 v3.1 2 v3.2 2 v3.3 3 v3.4 1 v3.5 8 v3.6 7 v3.7 3 v3.8 6 v3.9 3 v3.10 2 v3.11 5 v3.12 5 v3.13 2 v3.14 4 v3.15 2 v3.16 3 v3.17 2 v3.18 2 v3.19 1 v4.0 2 v4.1 5 v4.2 4 v4.5 5 v4.7 6 v4.8 1 v4.9 3 v4.10 2 v4.11 6 v4.12 3 v4.13 2 v4.14 5 v4.15 2 v4.16 7 v4.18 2 v4.19 6 v4.20 3 v5.0 12 v5.1 3 v5.2 4 v5.3 2 v5.4 1 v5.8 And the reason it's fully zero, is because we still have the cases we're cleaning up right now. Even this last one from v5.8 is specifically of the same type this series addresses: case 4: color_index = TrueCModeIndex; + break; default: return; } [2] https://www.kernel.org/doc/html/latest/process/deprecated.html#implicit-switch-case-fall-through All switch/case blocks must end in one of: break; fallthrough; continue; goto ; return [expression]; [3] https://cwe.mitre.org/data/definitions/484.html -- Kees Cook ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 000/141] Fix fall-through warnings for Clang
On Mon, Nov 23, 2020 at 05:32:51PM -0800, Nick Desaulniers wrote:
> On Sun, Nov 22, 2020 at 8:17 AM Kees Cook wrote:
> >
> > On Fri, Nov 20, 2020 at 11:51:42AM -0800, Jakub Kicinski wrote:
> > > If none of the 140 patches here fix a real bug, and there is no change
> > > to machine code then it sounds to me like a W=2 kind of a warning.
> >
> > FWIW, this series has found at least one bug so far:
> > https://lore.kernel.org/lkml/CAFCwf11izHF=g1mGry1fE5kvFFFrxzhPSM6qKAO8gxSp=kr...@mail.gmail.com/
>
> So looks like the bulk of these are:
> switch (x) {
> case 0:
> ++x;
> default:
> break;
> }
>
> I have a patch that fixes those up for clang:
> https://reviews.llvm.org/D91895
I still think this isn't right -- it's a case statement that runs off
the end without an explicit flow control determination. I think Clang is
right to warn for these, and GCC should also warn.
--
Kees Cook
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [Intel-wired-lan] [PATCH 000/141] Fix fall-through warnings for Clang
On Mon, Nov 23, 2020 at 08:31:30AM -0800, James Bottomley wrote: > Really, no ... something which produces no improvement has no value at > all ... we really shouldn't be wasting maintainer time with it because > it has a cost to merge. I'm not sure we understand where the balance > lies in value vs cost to merge but I am confident in the zero value > case. What? We can't measure how many future bugs aren't introduced because the kernel requires explicit case flow-control statements for all new code. We already enable -Wimplicit-fallthrough globally, so that's not the discussion. The issue is that Clang is (correctly) even more strict than GCC for this, so these are the remaining ones to fix for full Clang coverage too. People have spent more time debating this already than it would have taken to apply the patches. :) This is about robustness and language wrangling. It's a big code-base, and this is the price of our managing technical debt for permanent robustness improvements. (The numbers I ran from Gustavo's earlier patches were that about 10% of the places adjusted were identified as legitimate bugs being fixed. This final series may be lower, but there are still bugs being found from it -- we need to finish this and shut the door on it for good.) -- Kees Cook ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 000/141] Fix fall-through warnings for Clang
On Fri, Nov 20, 2020 at 11:51:42AM -0800, Jakub Kicinski wrote: > On Fri, 20 Nov 2020 11:30:40 -0800 Kees Cook wrote: > > On Fri, Nov 20, 2020 at 10:53:44AM -0800, Jakub Kicinski wrote: > > > On Fri, 20 Nov 2020 12:21:39 -0600 Gustavo A. R. Silva wrote: > > > > This series aims to fix almost all remaining fall-through warnings in > > > > order to enable -Wimplicit-fallthrough for Clang. > > > > > > > > In preparation to enable -Wimplicit-fallthrough for Clang, explicitly > > > > add multiple break/goto/return/fallthrough statements instead of just > > > > letting the code fall through to the next case. > > > > > > > > Notice that in order to enable -Wimplicit-fallthrough for Clang, this > > > > change[1] is meant to be reverted at some point. So, this patch helps > > > > to move in that direction. > > > > > > > > Something important to mention is that there is currently a discrepancy > > > > between GCC and Clang when dealing with switch fall-through to empty > > > > case > > > > statements or to cases that only contain a break/continue/return > > > > statement[2][3][4]. > > > > > > Are we sure we want to make this change? Was it discussed before? > > > > > > Are there any bugs Clangs puritanical definition of fallthrough helped > > > find? > > > > > > IMVHO compiler warnings are supposed to warn about issues that could > > > be bugs. Falling through to default: break; can hardly be a bug?! > > > > It's certainly a place where the intent is not always clear. I think > > this makes all the cases unambiguous, and doesn't impact the machine > > code, since the compiler will happily optimize away any behavioral > > redundancy. > > If none of the 140 patches here fix a real bug, and there is no change > to machine code then it sounds to me like a W=2 kind of a warning. FWIW, this series has found at least one bug so far: https://lore.kernel.org/lkml/CAFCwf11izHF=g1mGry1fE5kvFFFrxzhPSM6qKAO8gxSp=kr...@mail.gmail.com/ -- Kees Cook ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 000/141] Fix fall-through warnings for Clang
On Fri, Nov 20, 2020 at 11:51:42AM -0800, Jakub Kicinski wrote: > On Fri, 20 Nov 2020 11:30:40 -0800 Kees Cook wrote: > > On Fri, Nov 20, 2020 at 10:53:44AM -0800, Jakub Kicinski wrote: > > > On Fri, 20 Nov 2020 12:21:39 -0600 Gustavo A. R. Silva wrote: > > > > This series aims to fix almost all remaining fall-through warnings in > > > > order to enable -Wimplicit-fallthrough for Clang. > > > > > > > > In preparation to enable -Wimplicit-fallthrough for Clang, explicitly > > > > add multiple break/goto/return/fallthrough statements instead of just > > > > letting the code fall through to the next case. > > > > > > > > Notice that in order to enable -Wimplicit-fallthrough for Clang, this > > > > change[1] is meant to be reverted at some point. So, this patch helps > > > > to move in that direction. > > > > > > > > Something important to mention is that there is currently a discrepancy > > > > between GCC and Clang when dealing with switch fall-through to empty > > > > case > > > > statements or to cases that only contain a break/continue/return > > > > statement[2][3][4]. > > > > > > Are we sure we want to make this change? Was it discussed before? > > > > > > Are there any bugs Clangs puritanical definition of fallthrough helped > > > find? > > > > > > IMVHO compiler warnings are supposed to warn about issues that could > > > be bugs. Falling through to default: break; can hardly be a bug?! > > > > It's certainly a place where the intent is not always clear. I think > > this makes all the cases unambiguous, and doesn't impact the machine > > code, since the compiler will happily optimize away any behavioral > > redundancy. > > If none of the 140 patches here fix a real bug, and there is no change > to machine code then it sounds to me like a W=2 kind of a warning. I'd like to avoid splitting common -W options between default and W=2 just based on the compiler. Getting -Wimplicit-fallthrough enabled found plenty of bugs, so making sure it works correctly for both compilers feels justified to me. (This is just a subset of the same C language short-coming.) > I think clang is just being annoying here, but if I'm the only one who > feels this way chances are I'm wrong :) It's being pretty pedantic, but I don't think it's unreasonable to explicitly state how every case ends. GCC's silence for the case of "fall through to a break" doesn't really seem justified. -- Kees Cook ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 000/141] Fix fall-through warnings for Clang
On Fri, Nov 20, 2020 at 10:53:44AM -0800, Jakub Kicinski wrote: > On Fri, 20 Nov 2020 12:21:39 -0600 Gustavo A. R. Silva wrote: > > This series aims to fix almost all remaining fall-through warnings in > > order to enable -Wimplicit-fallthrough for Clang. > > > > In preparation to enable -Wimplicit-fallthrough for Clang, explicitly > > add multiple break/goto/return/fallthrough statements instead of just > > letting the code fall through to the next case. > > > > Notice that in order to enable -Wimplicit-fallthrough for Clang, this > > change[1] is meant to be reverted at some point. So, this patch helps > > to move in that direction. > > > > Something important to mention is that there is currently a discrepancy > > between GCC and Clang when dealing with switch fall-through to empty case > > statements or to cases that only contain a break/continue/return > > statement[2][3][4]. > > Are we sure we want to make this change? Was it discussed before? > > Are there any bugs Clangs puritanical definition of fallthrough helped > find? > > IMVHO compiler warnings are supposed to warn about issues that could > be bugs. Falling through to default: break; can hardly be a bug?! It's certainly a place where the intent is not always clear. I think this makes all the cases unambiguous, and doesn't impact the machine code, since the compiler will happily optimize away any behavioral redundancy. -- Kees Cook ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH] amdgpu_dm: fix nonblocking atomic commit use-after-free
On Fri, Jul 24, 2020 at 09:45:18AM +0200, Paul Menzel wrote:
> Am 24.07.20 um 00:32 schrieb Kees Cook:
> > On Thu, Jul 23, 2020 at 09:10:15PM +, Mazin Rezk wrote:
> As Linux 5.8-rc7 is going to be released this Sunday, I wonder, if commit
> 3202fa62f ("slub: relocate freelist pointer to middle of object") should be
> reverted for now to fix the regression for the users according to Linux’ no
> regression policy. Once the AMDGPU/DRM driver issue is fixed, it can be
> reapplied. I know it’s not optimal, but as some testing is going to be
> involved for the fix, I’d argue it’s the best option for the users.
Well, the SLUB defense was already released in v5.7, so I'm not sure it
really helps for amdgpu_dm users seeing it there too. There was a fix to
disable the async path for this driver that worked around the bug too,
yes? That seems like a safer and more focused change that doesn't revert
the SLUB defense for all users, and would actually provide a complete,
I think, workaround whereas reverting the SLUB change means the race
still exists. For example, it would be hit with slab poisoning, etc.
--
Kees Cook
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH] amdgpu_dm: fix nonblocking atomic commit use-after-free
On Thu, Jul 23, 2020 at 09:10:15PM +, Mazin Rezk wrote:
> When amdgpu_dm_atomic_commit_tail is running in the workqueue,
> drm_atomic_state_put will get called while amdgpu_dm_atomic_commit_tail is
> running, causing a race condition where state (and then dm_state) is
> sometimes freed while amdgpu_dm_atomic_commit_tail is running. This bug has
> occurred since 5.7-rc1 and is well documented among polaris11 users [1].
>
> Prior to 5.7, this was not a noticeable issue since the freelist pointer
> was stored at the beginning of dm_state (base), which was unused. After
> changing the freelist pointer to be stored in the middle of the struct, the
> freelist pointer overwrote the context, causing dc_state to become garbage
> data and made the call to dm_enable_per_frame_crtc_master_sync dereference
> a freelist pointer.
>
> This patch fixes the aforementioned issue by calling drm_atomic_state_get
> in amdgpu_dm_atomic_commit before drm_atomic_helper_commit is called and
> drm_atomic_state_put after amdgpu_dm_atomic_commit_tail is complete.
>
> According to my testing on 5.8.0-rc6, this should fix bug 207383 on
> Bugzilla [1].
>
> [1] https://bugzilla.kernel.org/show_bug.cgi?id=207383
Nice work tracking this down!
> Fixes: 3202fa62f ("slub: relocate freelist pointer to middle of object")
I do, however, object to this Fixes tag. :) The flaw appears to have
been with amdgpu_dm's reference tracking of "state" in the nonblocking
case. (How this reference counting is supposed to work correctly, though,
I'm not sure.) If I look at where the drm helper was split from being
the default callback, it looks like this was what introduced the bug:
da5c47f682ab ("drm/amd/display: Remove acrtc->stream")
? 3202fa62f certainly exposed it much more quickly, but there was a race
even without 3202fa62f where something could have realloced the memory
and written over it.
--
Kees Cook
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [Regression] hangs caused by commit 3202fa62fb (slub: relocate freelist pointer to middle of object)
On Tue, Jul 21, 2020 at 04:55:12PM +0200, Paul Menzel wrote: > No idea, if you are aware of it yet, but three people verified that commit > 3202fa62fb (slub: relocate freelist pointer to middle of object) causes a > regression on AMD hardware [1]. Hi, thanks for emailing; I don't get bugzilla notifications, so I hadn't seen this yet. > It’d be great, if you took a look, and advised if this commit (and > follow-ups) should be reverted, until the issue is analyzed. There have been a number of fixes to that commit (which I see are mentioned in a quick skim of the bug), but they've mostly been around additional slab debugging features. If it's causing a problem outside of that, my instinct would be there might be a use-after-free happening, but I'll go read the bug more closely now, and comment there (or here, if needed). Thanks! -- Kees Cook ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 0/2] drm/radeon: have the callers of set_memory_*() check the return value
On Wed, Jan 08, 2020 at 01:56:47PM +0100, Christian König wrote: > Am 07.01.20 um 20:25 schrieb Tianlin Li: > > Right now several architectures allow their set_memory_*() family of > > functions to fail, but callers may not be checking the return values. > > If set_memory_*() returns with an error, call-site assumptions may be > > infact wrong to assume that it would either succeed or not succeed at > > all. Ideally, the failure of set_memory_*() should be passed up the > > call stack, and callers should examine the failure and deal with it. > > > > Need to fix the callers and add the __must_check attribute. They also > > may not provide any level of atomicity, in the sense that the memory > > protections may be left incomplete on failure. This issue likely has a > > few steps on effects architectures: > > 1)Have all callers of set_memory_*() helpers check the return value. > > 2)Add __must_check to all set_memory_*() helpers so that new uses do > > not ignore the return value. > > 3)Add atomicity to the calls so that the memory protections aren't left > > in a partial state. > > > > This series is part of step 1. Make drm/radeon check the return value of > > set_memory_*(). > > I'm a little hesitate merge that. This hardware is >15 years old and nobody > of the developers have any system left to test this change on. If that's true it should be removed from the tree. We need to be able to correctly make these kinds of changes in the kernel. > Would it be to much of a problem to just add something like: r = > set_memory_*(); (void)r; /* Intentionally ignored */. This seems like a bad idea -- we shouldn't be papering over failures like this when there is logic available to deal with it. > Apart from that certainly a good idea to add __must_check to the functions. Agreed! -Kees -- Kees Cook ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: linux-next: Tree for Sep 4 (amd/display/)
On Wed, Sep 4, 2019 at 1:58 PM Randy Dunlap wrote: > > On 9/4/19 6:34 AM, Stephen Rothwell wrote: > > Hi all, > > > > News: this will be the last linux-next I will release until Sept 30. > > > > Changes since 20190903: > > > > on x86_64: > > In file included from > ../drivers/gpu/drm/amd/amdgpu/../display/dc/dml/dcn20/display_rq_dlg_calc_20v2.c:77:0: > ../drivers/gpu/drm/amd/amdgpu/../display/dc/dml/dcn20/../dml_inline_defs.h: > In function ‘dml_min’: > ../drivers/gpu/drm/amd/amdgpu/../display/dc/dml/dcn20/../dml_inline_defs.h:34:1: > error: SSE register return with SSE disabled I'm still tripping over this too. What compilers are people building with where this is NOT happening for an allmodconfig? I'm using: gcc (Ubuntu 7.4.0-1ubuntu1~18.04.1) 7.4.0 But it happens on newer compilers too. -- Kees Cook
Re: [PATCH v19 00/15] arm64: untag user pointers passed to the kernel
On Thu, Aug 08, 2019 at 03:33:00PM -0700, Andrew Morton wrote: > On Thu, 8 Aug 2019 14:12:19 -0700 Kees Cook wrote: > > > > The ones that are left are the mm ones: 4, 5, 6, 7 and 8. > > > > > > Andrew, could you take a look and give your Acked-by or pick them up > > > directly? > > > > Given the subsystem Acks, it seems like 3-10 and 12 could all just go > > via Andrew? I hope he agrees. :) > > I'll grab everything that has not yet appeared in linux-next. If more > of these patches appear in linux-next I'll drop those as well. > > The review discussion against " [PATCH v19 02/15] arm64: Introduce > prctl() options to control the tagged user addresses ABI" has petered > out inconclusively. prctl() vs arch_prctl(). I've always disliked arch_prctl() existing at all. Given that tagging is likely to be a multi-architectural feature, it seems like the controls should live in prctl() to me. -- Kees Cook
Re: [PATCH v19 00/15] arm64: untag user pointers passed to the kernel
On Wed, Aug 07, 2019 at 07:17:35PM +0200, Andrey Konovalov wrote: > On Tue, Aug 6, 2019 at 7:13 PM Will Deacon wrote: > > > > On Wed, Jul 24, 2019 at 03:20:59PM +0100, Will Deacon wrote: > > > On Wed, Jul 24, 2019 at 04:16:49PM +0200, Andrey Konovalov wrote: > > > > On Wed, Jul 24, 2019 at 4:02 PM Will Deacon wrote: > > > > > On Tue, Jul 23, 2019 at 08:03:29PM +0200, Andrey Konovalov wrote: > > > > > > Should this go through the mm or the arm tree? > > > > > > > > > > I would certainly prefer to take at least the arm64 bits via the > > > > > arm64 tree > > > > > (i.e. patches 1, 2 and 15). We also need a Documentation patch > > > > > describing > > > > > the new ABI. > > > > > > > > Sounds good! Should I post those patches together with the > > > > Documentation patches from Vincenzo as a separate patchset? > > > > > > Yes, please (although as you say below, we need a new version of those > > > patches from Vincenzo to address the feedback on v5). The other thing I > > > should say is that I'd be happy to queue the other patches in the series > > > too, but some of them are missing acks from the relevant maintainers (e.g. > > > the mm/ and fs/ changes). > > > > Ok, I've queued patches 1, 2, and 15 on a stable branch here: > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git/log/?h=for-next/tbi > > > > which should find its way into -next shortly via our for-next/core branch. > > If you want to make changes, please send additional patches on top. > > > > This is targetting 5.4, but I will drop it before the merge window if > > we don't have both of the following in place: > > > > * Updated ABI documentation with Acks from Catalin and Kevin > > Catalin has posted a new version today. > > > * The other patches in the series either Acked (so I can pick them up) > > or queued via some other tree(s) for 5.4. > > So we have the following patches in this series: > > 1. arm64: untag user pointers in access_ok and __uaccess_mask_ptr > 2. arm64: Introduce prctl() options to control the tagged user addresses ABI > 3. lib: untag user pointers in strn*_user > 4. mm: untag user pointers passed to memory syscalls > 5. mm: untag user pointers in mm/gup.c > 6. mm: untag user pointers in get_vaddr_frames > 7. fs/namespace: untag user pointers in copy_mount_options > 8. userfaultfd: untag user pointers > 9. drm/amdgpu: untag user pointers > 10. drm/radeon: untag user pointers in radeon_gem_userptr_ioctl > 11. IB/mlx4: untag user pointers in mlx4_get_umem_mr > 12. media/v4l2-core: untag user pointers in videobuf_dma_contig_user_get > 13. tee/shm: untag user pointers in tee_shm_register > 14. vfio/type1: untag user pointers in vaddr_get_pfn > 15. selftests, arm64: add a selftest for passing tagged pointers to kernel > > 1, 2 and 15 have been picked by Will. > > 11 has been picked up by Jason. > > 9, 10, 12, 13 and 14 have acks from their subsystem maintainers. > > 3 touches generic lib code, I'm not sure if there's a dedicated > maintainer for that. Andrew tends to pick up lib/ patches. > The ones that are left are the mm ones: 4, 5, 6, 7 and 8. > > Andrew, could you take a look and give your Acked-by or pick them up directly? Given the subsystem Acks, it seems like 3-10 and 12 could all just go via Andrew? I hope he agrees. :) -- Kees Cook
Re: [PATCH v18 07/15] fs/namespace: untag user pointers in copy_mount_options
+Eric Biederman too, who might be able to Ack this... On Mon, Jul 15, 2019 at 06:00:04PM +0200, Andrey Konovalov wrote: > On Mon, Jun 24, 2019 at 7:50 PM Catalin Marinas > wrote: > > > > On Mon, Jun 24, 2019 at 04:32:52PM +0200, Andrey Konovalov wrote: > > > This patch is a part of a series that extends kernel ABI to allow to pass > > > tagged user pointers (with the top byte set to something else other than > > > 0x00) as syscall arguments. > > > > > > In copy_mount_options a user address is being subtracted from TASK_SIZE. > > > If the address is lower than TASK_SIZE, the size is calculated to not > > > allow the exact_copy_from_user() call to cross TASK_SIZE boundary. > > > However if the address is tagged, then the size will be calculated > > > incorrectly. > > > > > > Untag the address before subtracting. > > > > > > Reviewed-by: Khalid Aziz > > > Reviewed-by: Vincenzo Frascino > > > Reviewed-by: Kees Cook > > > Reviewed-by: Catalin Marinas > > > Signed-off-by: Andrey Konovalov > > > --- > > > fs/namespace.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/fs/namespace.c b/fs/namespace.c > > > index 7660c2749c96..ec78f7223917 100644 > > > --- a/fs/namespace.c > > > +++ b/fs/namespace.c > > > @@ -2994,7 +2994,7 @@ void *copy_mount_options(const void __user * data) > > >* the remainder of the page. > > >*/ > > > /* copy_from_user cannot cross TASK_SIZE ! */ > > > - size = TASK_SIZE - (unsigned long)data; > > > + size = TASK_SIZE - (unsigned long)untagged_addr(data); > > > if (size > PAGE_SIZE) > > > size = PAGE_SIZE; > > > > I think this patch needs an ack from Al Viro (cc'ed). > > > > -- > > Catalin > > Hi Al, > > Could you take a look and give your acked-by? > > Thanks! -- Kees Cook
Re: [PATCH v18 02/15] arm64: Introduce prctl() options to control the tagged user addresses ABI
On Mon, Jun 24, 2019 at 04:32:47PM +0200, Andrey Konovalov wrote:
> From: Catalin Marinas
>
> It is not desirable to relax the ABI to allow tagged user addresses into
> the kernel indiscriminately. This patch introduces a prctl() interface
> for enabling or disabling the tagged ABI with a global sysctl control
> for preventing applications from enabling the relaxed ABI (meant for
> testing user-space prctl() return error checking without reconfiguring
> the kernel). The ABI properties are inherited by threads of the same
> application and fork()'ed children but cleared on execve(). A Kconfig
> option allows the overall disabling of the relaxed ABI.
>
> The PR_SET_TAGGED_ADDR_CTRL will be expanded in the future to handle
> MTE-specific settings like imprecise vs precise exceptions.
>
> Signed-off-by: Catalin Marinas
Reviewed-by: Kees Cook
-Kees
> Signed-off-by: Andrey Konovalov
> ---
> arch/arm64/Kconfig | 9
> arch/arm64/include/asm/processor.h | 8
> arch/arm64/include/asm/thread_info.h | 1 +
> arch/arm64/include/asm/uaccess.h | 4 +-
> arch/arm64/kernel/process.c | 72
> include/uapi/linux/prctl.h | 5 ++
> kernel/sys.c | 12 +
> 7 files changed, 110 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 697ea0510729..55fbaf20af2d 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -1107,6 +1107,15 @@ config ARM64_SW_TTBR0_PAN
> zeroed area and reserved ASID. The user access routines
> restore the valid TTBR0_EL1 temporarily.
>
> +config ARM64_TAGGED_ADDR_ABI
> + bool "Enable the tagged user addresses syscall ABI"
> + default y
> + help
> + When this option is enabled, user applications can opt in to a
> + relaxed ABI via prctl() allowing tagged addresses to be passed
> + to system calls as pointer arguments. For details, see
> + Documentation/arm64/tagged-address-abi.txt.
> +
> menuconfig COMPAT
> bool "Kernel support for 32-bit EL0"
> depends on ARM64_4K_PAGES || EXPERT
> diff --git a/arch/arm64/include/asm/processor.h
> b/arch/arm64/include/asm/processor.h
> index fd5b1a4efc70..ee86070a28d4 100644
> --- a/arch/arm64/include/asm/processor.h
> +++ b/arch/arm64/include/asm/processor.h
> @@ -296,6 +296,14 @@ extern void __init minsigstksz_setup(void);
> /* PR_PAC_RESET_KEYS prctl */
> #define PAC_RESET_KEYS(tsk, arg) ptrauth_prctl_reset_keys(tsk, arg)
>
> +#ifdef CONFIG_ARM64_TAGGED_ADDR_ABI
> +/* PR_{SET,GET}_TAGGED_ADDR_CTRL prctl */
> +long set_tagged_addr_ctrl(unsigned long arg);
> +long get_tagged_addr_ctrl(void);
> +#define SET_TAGGED_ADDR_CTRL(arg)set_tagged_addr_ctrl(arg)
> +#define GET_TAGGED_ADDR_CTRL() get_tagged_addr_ctrl()
> +#endif
> +
> /*
> * For CONFIG_GCC_PLUGIN_STACKLEAK
> *
> diff --git a/arch/arm64/include/asm/thread_info.h
> b/arch/arm64/include/asm/thread_info.h
> index 2372e97db29c..4f81c4f15404 100644
> --- a/arch/arm64/include/asm/thread_info.h
> +++ b/arch/arm64/include/asm/thread_info.h
> @@ -88,6 +88,7 @@ void arch_release_task_struct(struct task_struct *tsk);
> #define TIF_SVE 23 /* Scalable Vector Extension in
> use */
> #define TIF_SVE_VL_INHERIT 24 /* Inherit sve_vl_onexec across exec */
> #define TIF_SSBD 25 /* Wants SSB mitigation */
> +#define TIF_TAGGED_ADDR 26 /* Allow tagged user addresses
> */
>
> #define _TIF_SIGPENDING (1 << TIF_SIGPENDING)
> #define _TIF_NEED_RESCHED(1 << TIF_NEED_RESCHED)
> diff --git a/arch/arm64/include/asm/uaccess.h
> b/arch/arm64/include/asm/uaccess.h
> index a138e3b4f717..097d6bfac0b7 100644
> --- a/arch/arm64/include/asm/uaccess.h
> +++ b/arch/arm64/include/asm/uaccess.h
> @@ -62,7 +62,9 @@ static inline unsigned long __range_ok(const void __user
> *addr, unsigned long si
> {
> unsigned long ret, limit = current_thread_info()->addr_limit;
>
> - addr = untagged_addr(addr);
> + if (IS_ENABLED(CONFIG_ARM64_TAGGED_ADDR_ABI) &&
> + test_thread_flag(TIF_TAGGED_ADDR))
> + addr = untagged_addr(addr);
>
> __chk_user_ptr(addr);
> asm volatile(
> diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
> index 9856395ccdb7..60e70158a4a1 100644
> --- a/arch/arm64/kernel/process.c
> +++ b/arch/arm64/kernel/process.c
> @@ -19,6 +19,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
>
Re: [PATCH v18 10/15] drm/radeon: untag user pointers in radeon_gem_userptr_ioctl
On Mon, Jun 24, 2019 at 04:32:55PM +0200, Andrey Konovalov wrote: > This patch is a part of a series that extends kernel ABI to allow to pass > tagged user pointers (with the top byte set to something else other than > 0x00) as syscall arguments. > > In radeon_gem_userptr_ioctl() an MMU notifier is set up with a (tagged) > userspace pointer. The untagged address should be used so that MMU > notifiers for the untagged address get correctly matched up with the right > BO. This funcation also calls radeon_ttm_tt_pin_userptr(), which uses > provided user pointers for vma lookups, which can only by done with > untagged pointers. > > This patch untags user pointers in radeon_gem_userptr_ioctl(). > > Suggested-by: Felix Kuehling > Acked-by: Felix Kuehling > Signed-off-by: Andrey Konovalov Reviewed-by: Kees Cook -Kees > --- > drivers/gpu/drm/radeon/radeon_gem.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/gpu/drm/radeon/radeon_gem.c > b/drivers/gpu/drm/radeon/radeon_gem.c > index 44617dec8183..90eb78fb5eb2 100644 > --- a/drivers/gpu/drm/radeon/radeon_gem.c > +++ b/drivers/gpu/drm/radeon/radeon_gem.c > @@ -291,6 +291,8 @@ int radeon_gem_userptr_ioctl(struct drm_device *dev, void > *data, > uint32_t handle; > int r; > > + args->addr = untagged_addr(args->addr); > + > if (offset_in_page(args->addr | args->size)) > return -EINVAL; > > -- > 2.22.0.410.gd8fdbe21b5-goog > -- Kees Cook
Re: [PATCH v18 15/15] selftests, arm64: add a selftest for passing tagged pointers to kernel
On Mon, Jun 24, 2019 at 04:33:00PM +0200, Andrey Konovalov wrote:
> This patch is a part of a series that extends kernel ABI to allow to pass
> tagged user pointers (with the top byte set to something else other than
> 0x00) as syscall arguments.
>
> This patch adds a simple test, that calls the uname syscall with a
> tagged user pointer as an argument. Without the kernel accepting tagged
> user pointers the test fails with EFAULT.
>
> Signed-off-by: Andrey Konovalov
Acked-by: Kees Cook
-Kees
> ---
> tools/testing/selftests/arm64/.gitignore | 1 +
> tools/testing/selftests/arm64/Makefile| 11 +++
> .../testing/selftests/arm64/run_tags_test.sh | 12
> tools/testing/selftests/arm64/tags_test.c | 29 +++
> 4 files changed, 53 insertions(+)
> create mode 100644 tools/testing/selftests/arm64/.gitignore
> create mode 100644 tools/testing/selftests/arm64/Makefile
> create mode 100755 tools/testing/selftests/arm64/run_tags_test.sh
> create mode 100644 tools/testing/selftests/arm64/tags_test.c
>
> diff --git a/tools/testing/selftests/arm64/.gitignore
> b/tools/testing/selftests/arm64/.gitignore
> new file mode 100644
> index ..e8fae8d61ed6
> --- /dev/null
> +++ b/tools/testing/selftests/arm64/.gitignore
> @@ -0,0 +1 @@
> +tags_test
> diff --git a/tools/testing/selftests/arm64/Makefile
> b/tools/testing/selftests/arm64/Makefile
> new file mode 100644
> index ..a61b2e743e99
> --- /dev/null
> +++ b/tools/testing/selftests/arm64/Makefile
> @@ -0,0 +1,11 @@
> +# SPDX-License-Identifier: GPL-2.0
> +
> +# ARCH can be overridden by the user for cross compiling
> +ARCH ?= $(shell uname -m 2>/dev/null || echo not)
> +
> +ifneq (,$(filter $(ARCH),aarch64 arm64))
> +TEST_GEN_PROGS := tags_test
> +TEST_PROGS := run_tags_test.sh
> +endif
> +
> +include ../lib.mk
> diff --git a/tools/testing/selftests/arm64/run_tags_test.sh
> b/tools/testing/selftests/arm64/run_tags_test.sh
> new file mode 100755
> index ..745f11379930
> --- /dev/null
> +++ b/tools/testing/selftests/arm64/run_tags_test.sh
> @@ -0,0 +1,12 @@
> +#!/bin/sh
> +# SPDX-License-Identifier: GPL-2.0
> +
> +echo ""
> +echo "running tags test"
> +echo ""
> +./tags_test
> +if [ $? -ne 0 ]; then
> + echo "[FAIL]"
> +else
> + echo "[PASS]"
> +fi
> diff --git a/tools/testing/selftests/arm64/tags_test.c
> b/tools/testing/selftests/arm64/tags_test.c
> new file mode 100644
> index ..22a1b266e373
> --- /dev/null
> +++ b/tools/testing/selftests/arm64/tags_test.c
> @@ -0,0 +1,29 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#define SHIFT_TAG(tag) ((uint64_t)(tag) << 56)
> +#define SET_TAG(ptr, tag)(((uint64_t)(ptr) & ~SHIFT_TAG(0xff)) | \
> + SHIFT_TAG(tag))
> +
> +int main(void)
> +{
> + static int tbi_enabled = 0;
> + struct utsname *ptr, *tagged_ptr;
> + int err;
> +
> + if (prctl(PR_SET_TAGGED_ADDR_CTRL, PR_TAGGED_ADDR_ENABLE, 0, 0, 0) == 0)
> + tbi_enabled = 1;
> + ptr = (struct utsname *)malloc(sizeof(*ptr));
> + if (tbi_enabled)
> + tagged_ptr = (struct utsname *)SET_TAG(ptr, 0x42);
> + err = uname(tagged_ptr);
> + free(ptr);
> +
> + return err;
> +}
> --
> 2.22.0.410.gd8fdbe21b5-goog
>
--
Kees Cook
Re: [PATCH v18 11/15] IB/mlx4: untag user pointers in mlx4_get_umem_mr
On Mon, Jun 24, 2019 at 04:32:56PM +0200, Andrey Konovalov wrote:
> This patch is a part of a series that extends kernel ABI to allow to pass
> tagged user pointers (with the top byte set to something else other than
> 0x00) as syscall arguments.
>
> mlx4_get_umem_mr() uses provided user pointers for vma lookups, which can
> only by done with untagged pointers.
>
> Untag user pointers in this function.
>
> Signed-off-by: Andrey Konovalov
Reviewed-by: Kees Cook
-Kees
> ---
> drivers/infiniband/hw/mlx4/mr.c | 7 ---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/infiniband/hw/mlx4/mr.c b/drivers/infiniband/hw/mlx4/mr.c
> index 355205a28544..13d9f917f249 100644
> --- a/drivers/infiniband/hw/mlx4/mr.c
> +++ b/drivers/infiniband/hw/mlx4/mr.c
> @@ -378,6 +378,7 @@ static struct ib_umem *mlx4_get_umem_mr(struct ib_udata
> *udata, u64 start,
>* again
>*/
> if (!ib_access_writable(access_flags)) {
> + unsigned long untagged_start = untagged_addr(start);
> struct vm_area_struct *vma;
>
> down_read(>mm->mmap_sem);
> @@ -386,9 +387,9 @@ static struct ib_umem *mlx4_get_umem_mr(struct ib_udata
> *udata, u64 start,
>* cover the memory, but for now it requires a single vma to
>* entirely cover the MR to support RO mappings.
>*/
> - vma = find_vma(current->mm, start);
> - if (vma && vma->vm_end >= start + length &&
> - vma->vm_start <= start) {
> + vma = find_vma(current->mm, untagged_start);
> + if (vma && vma->vm_end >= untagged_start + length &&
> + vma->vm_start <= untagged_start) {
> if (vma->vm_flags & VM_WRITE)
> access_flags |= IB_ACCESS_LOCAL_WRITE;
> } else {
> --
> 2.22.0.410.gd8fdbe21b5-goog
>
--
Kees Cook
Re: [PATCH v18 10/15] drm/radeon: untag user pointers in radeon_gem_userptr_ioctl
On Mon, Jun 24, 2019 at 04:32:55PM +0200, Andrey Konovalov wrote: > This patch is a part of a series that extends kernel ABI to allow to pass > tagged user pointers (with the top byte set to something else other than > 0x00) as syscall arguments. > > In radeon_gem_userptr_ioctl() an MMU notifier is set up with a (tagged) > userspace pointer. The untagged address should be used so that MMU > notifiers for the untagged address get correctly matched up with the right > BO. This funcation also calls radeon_ttm_tt_pin_userptr(), which uses > provided user pointers for vma lookups, which can only by done with > untagged pointers. > > This patch untags user pointers in radeon_gem_userptr_ioctl(). > > Suggested-by: Felix Kuehling > Acked-by: Felix Kuehling > Signed-off-by: Andrey Konovalov Reviewed-by: Kees Cook -Kees > --- > drivers/gpu/drm/radeon/radeon_gem.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/gpu/drm/radeon/radeon_gem.c > b/drivers/gpu/drm/radeon/radeon_gem.c > index 44617dec8183..90eb78fb5eb2 100644 > --- a/drivers/gpu/drm/radeon/radeon_gem.c > +++ b/drivers/gpu/drm/radeon/radeon_gem.c > @@ -291,6 +291,8 @@ int radeon_gem_userptr_ioctl(struct drm_device *dev, void > *data, > uint32_t handle; > int r; > > + args->addr = untagged_addr(args->addr); > + > if (offset_in_page(args->addr | args->size)) > return -EINVAL; > > -- > 2.22.0.410.gd8fdbe21b5-goog > -- Kees Cook
Re: [PATCH v17 03/15] arm64: Introduce prctl() options to control the tagged user addresses ABI
On Thu, Jun 13, 2019 at 04:26:32PM +0100, Catalin Marinas wrote: > On Thu, Jun 13, 2019 at 12:02:35PM +0100, Dave P Martin wrote: > > On Wed, Jun 12, 2019 at 01:43:20PM +0200, Andrey Konovalov wrote: > > > +static int zero; > > > +static int one = 1; > > > > !!! > > > > And these can't even be const without a cast. Yuk. > > > > (Not your fault though, but it would be nice to have a proc_dobool() to > > avoid this.) > > I had the same reaction. Maybe for another patch sanitising this pattern > across the kernel. That's actually already happening (via -mm tree last I looked). tl;dr: it ends up using a cast hidden in a macro. It's in linux-next already along with a checkpatch.pl addition to yell about doing what's being done here. ;) https://lore.kernel.org/lkml/20190430180111.10688-1-mcr...@redhat.com/#r -- Kees Cook
Re: [PATCH v16 02/16] arm64: untag user pointers in access_ok and __uaccess_mask_ptr
On Mon, Jun 10, 2019 at 07:53:30PM +0100, Catalin Marinas wrote:
> On Mon, Jun 10, 2019 at 11:07:03AM -0700, Kees Cook wrote:
> > On Mon, Jun 10, 2019 at 06:53:27PM +0100, Catalin Marinas wrote:
> > > diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
> > > index 3767fb21a5b8..fd191c5b92aa 100644
> > > --- a/arch/arm64/kernel/process.c
> > > +++ b/arch/arm64/kernel/process.c
> > > @@ -552,3 +552,18 @@ void arch_setup_new_exec(void)
> > >
> > > ptrauth_thread_init_user(current);
> > > }
> > > +
> > > +/*
> > > + * Enable the relaxed ABI allowing tagged user addresses into the kernel.
> > > + */
> > > +int untagged_uaddr_set_mode(unsigned long arg)
> > > +{
> > > + if (is_compat_task())
> > > + return -ENOTSUPP;
> > > + if (arg)
> > > + return -EINVAL;
> > > +
> > > + set_thread_flag(TIF_UNTAGGED_UADDR);
> > > +
> > > + return 0;
> > > +}
> >
> > I think this should be paired with a flag clearing in copy_thread(),
> > yes? (i.e. each binary needs to opt in)
>
> It indeed needs clearing though not in copy_thread() as that's used on
> clone/fork but rather in flush_thread(), called on the execve() path.
Ah! Yes, thanks.
> And a note to myself: I think PR_UNTAGGED_ADDR (not UADDR) looks better
> in a uapi header, the user doesn't differentiate between uaddr and
> kaddr.
Good point. I would agree. :)
--
Kees Cook
Re: [PATCH v16 02/16] arm64: untag user pointers in access_ok and __uaccess_mask_ptr
On Mon, Jun 10, 2019 at 06:53:27PM +0100, Catalin Marinas wrote:
> On Mon, Jun 03, 2019 at 06:55:04PM +0200, Andrey Konovalov wrote:
> > diff --git a/arch/arm64/include/asm/uaccess.h
> > b/arch/arm64/include/asm/uaccess.h
> > index e5d5f31c6d36..9164ecb5feca 100644
> > --- a/arch/arm64/include/asm/uaccess.h
> > +++ b/arch/arm64/include/asm/uaccess.h
> > @@ -94,7 +94,7 @@ static inline unsigned long __range_ok(const void __user
> > *addr, unsigned long si
> > return ret;
> > }
> >
> > -#define access_ok(addr, size) __range_ok(addr, size)
> > +#define access_ok(addr, size) __range_ok(untagged_addr(addr), size)
>
> I'm going to propose an opt-in method here (RFC for now). We can't have
> a check in untagged_addr() since this is already used throughout the
> kernel for both user and kernel addresses (khwasan) but we can add one
> in __range_ok(). The same prctl() option will be used for controlling
> the precise/imprecise mode of MTE later on. We can use a TIF_ flag here
> assuming that this will be called early on and any cloned thread will
> inherit this.
>
> Anyway, it's easier to paste some diff than explain but Vincenzo can
> fold them into his ABI patches that should really go together with
> these. I added a couple of MTE definitions for prctl() as an example,
> not used currently:
>
> --8<-
> diff --git a/arch/arm64/include/asm/processor.h
> b/arch/arm64/include/asm/processor.h
> index fcd0e691b1ea..2d4cb7e4edab 100644
> --- a/arch/arm64/include/asm/processor.h
> +++ b/arch/arm64/include/asm/processor.h
> @@ -307,6 +307,10 @@ extern void __init minsigstksz_setup(void);
> /* PR_PAC_RESET_KEYS prctl */
> #define PAC_RESET_KEYS(tsk, arg) ptrauth_prctl_reset_keys(tsk, arg)
>
> +/* PR_UNTAGGED_UADDR prctl */
> +int untagged_uaddr_set_mode(unsigned long arg);
> +#define SET_UNTAGGED_UADDR_MODE(arg) untagged_uaddr_set_mode(arg)
> +
> /*
> * For CONFIG_GCC_PLUGIN_STACKLEAK
> *
> diff --git a/arch/arm64/include/asm/thread_info.h
> b/arch/arm64/include/asm/thread_info.h
> index c285d1ce7186..89ce3c49 100644
> --- a/arch/arm64/include/asm/thread_info.h
> +++ b/arch/arm64/include/asm/thread_info.h
> @@ -101,6 +101,7 @@ void arch_release_task_struct(struct task_struct *tsk);
> #define TIF_SVE 23 /* Scalable Vector Extension in
> use */
> #define TIF_SVE_VL_INHERIT 24 /* Inherit sve_vl_onexec across exec */
> #define TIF_SSBD 25 /* Wants SSB mitigation */
> +#define TIF_UNTAGGED_UADDR 26
>
> #define _TIF_SIGPENDING (1 << TIF_SIGPENDING)
> #define _TIF_NEED_RESCHED(1 << TIF_NEED_RESCHED)
> @@ -116,6 +117,7 @@ void arch_release_task_struct(struct task_struct *tsk);
> #define _TIF_FSCHECK (1 << TIF_FSCHECK)
> #define _TIF_32BIT (1 << TIF_32BIT)
> #define _TIF_SVE (1 << TIF_SVE)
> +#define _TIF_UNTAGGED_UADDR (1 << TIF_UNTAGGED_UADDR)
>
> #define _TIF_WORK_MASK (_TIF_NEED_RESCHED | _TIF_SIGPENDING | \
>_TIF_NOTIFY_RESUME | _TIF_FOREIGN_FPSTATE | \
> diff --git a/arch/arm64/include/asm/uaccess.h
> b/arch/arm64/include/asm/uaccess.h
> index 9164ecb5feca..54f5bbaebbc4 100644
> --- a/arch/arm64/include/asm/uaccess.h
> +++ b/arch/arm64/include/asm/uaccess.h
> @@ -73,6 +73,9 @@ static inline unsigned long __range_ok(const void __user
> *addr, unsigned long si
> {
> unsigned long ret, limit = current_thread_info()->addr_limit;
>
> + if (test_thread_flag(TIF_UNTAGGED_UADDR))
> + addr = untagged_addr(addr);
> +
> __chk_user_ptr(addr);
> asm volatile(
> // A + B <= C + 1 for all A,B,C, in four easy steps:
> @@ -94,7 +97,7 @@ static inline unsigned long __range_ok(const void __user
> *addr, unsigned long si
> return ret;
> }
>
> -#define access_ok(addr, size)__range_ok(untagged_addr(addr), size)
> +#define access_ok(addr, size)__range_ok(addr, size)
> #define user_addr_maxget_fs
>
> #define _ASM_EXTABLE(from, to)
> \
> diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
> index 3767fb21a5b8..fd191c5b92aa 100644
> --- a/arch/arm64/kernel/process.c
> +++ b/arch/arm64/kernel/process.c
> @@ -552,3 +552,18 @@ void arch_setup_new_exec(void)
>
> ptrauth_thread_init_user(current);
> }
> +
> +/*
> + * Enable the relaxed ABI allowing tagged user addresses into the kernel.
> + */
> +int untagged_uaddr_set_mode(unsigned long arg)
> +{
> + if (is_compat_task())
> + return -ENOTSUPP;
> + if (arg)
> + return -EINVAL;
> +
> + set_thread_flag(TIF_UNTAGGED_UADDR);
> +
> + return 0;
> +}
I think this should be paired with a flag clearing in copy_thread(),
yes? (i.e. each binary needs to opt in)
--
Kees Cook
Re: [PATCH v16 14/16] tee, arm64: untag user pointers in tee_shm_register
On Mon, Jun 03, 2019 at 06:55:16PM +0200, Andrey Konovalov wrote: > This patch is a part of a series that extends arm64 kernel ABI to allow to > pass tagged user pointers (with the top byte set to something else other > than 0x00) as syscall arguments. > > tee_shm_register()->optee_shm_unregister()->check_mem_type() uses provided > user pointers for vma lookups (via __check_mem_type()), which can only by > done with untagged pointers. > > Untag user pointers in this function. > > Signed-off-by: Andrey Konovalov "tee: shm: untag user pointers in tee_shm_register" Reviewed-by: Kees Cook -Kees > --- > drivers/tee/tee_shm.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/tee/tee_shm.c b/drivers/tee/tee_shm.c > index 49fd7312e2aa..96945f4cefb8 100644 > --- a/drivers/tee/tee_shm.c > +++ b/drivers/tee/tee_shm.c > @@ -263,6 +263,7 @@ struct tee_shm *tee_shm_register(struct tee_context *ctx, > unsigned long addr, > shm->teedev = teedev; > shm->ctx = ctx; > shm->id = -1; > + addr = untagged_addr(addr); > start = rounddown(addr, PAGE_SIZE); > shm->offset = addr - start; > shm->size = length; > -- > 2.22.0.rc1.311.g5d7573a151-goog > -- Kees Cook
Re: [PATCH v16 09/16] fs, arm64: untag user pointers in fs/userfaultfd.c
On Mon, Jun 03, 2019 at 06:55:11PM +0200, Andrey Konovalov wrote:
> This patch is a part of a series that extends arm64 kernel ABI to allow to
> pass tagged user pointers (with the top byte set to something else other
> than 0x00) as syscall arguments.
>
> userfaultfd code use provided user pointers for vma lookups, which can
> only by done with untagged pointers.
>
> Untag user pointers in validate_range().
>
> Signed-off-by: Andrey Konovalov
"userfaultfd: untag user pointers"
Reviewed-by: Kees Cook
-Kees
> ---
> fs/userfaultfd.c | 22 --
> 1 file changed, 12 insertions(+), 10 deletions(-)
>
> diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c
> index 3b30301c90ec..24d68c3b5ee2 100644
> --- a/fs/userfaultfd.c
> +++ b/fs/userfaultfd.c
> @@ -1263,21 +1263,23 @@ static __always_inline void wake_userfault(struct
> userfaultfd_ctx *ctx,
> }
>
> static __always_inline int validate_range(struct mm_struct *mm,
> - __u64 start, __u64 len)
> + __u64 *start, __u64 len)
> {
> __u64 task_size = mm->task_size;
>
> - if (start & ~PAGE_MASK)
> + *start = untagged_addr(*start);
> +
> + if (*start & ~PAGE_MASK)
> return -EINVAL;
> if (len & ~PAGE_MASK)
> return -EINVAL;
> if (!len)
> return -EINVAL;
> - if (start < mmap_min_addr)
> + if (*start < mmap_min_addr)
> return -EINVAL;
> - if (start >= task_size)
> + if (*start >= task_size)
> return -EINVAL;
> - if (len > task_size - start)
> + if (len > task_size - *start)
> return -EINVAL;
> return 0;
> }
> @@ -1327,7 +1329,7 @@ static int userfaultfd_register(struct userfaultfd_ctx
> *ctx,
> goto out;
> }
>
> - ret = validate_range(mm, uffdio_register.range.start,
> + ret = validate_range(mm, _register.range.start,
>uffdio_register.range.len);
> if (ret)
> goto out;
> @@ -1516,7 +1518,7 @@ static int userfaultfd_unregister(struct
> userfaultfd_ctx *ctx,
> if (copy_from_user(_unregister, buf, sizeof(uffdio_unregister)))
> goto out;
>
> - ret = validate_range(mm, uffdio_unregister.start,
> + ret = validate_range(mm, _unregister.start,
>uffdio_unregister.len);
> if (ret)
> goto out;
> @@ -1667,7 +1669,7 @@ static int userfaultfd_wake(struct userfaultfd_ctx *ctx,
> if (copy_from_user(_wake, buf, sizeof(uffdio_wake)))
> goto out;
>
> - ret = validate_range(ctx->mm, uffdio_wake.start, uffdio_wake.len);
> + ret = validate_range(ctx->mm, _wake.start, uffdio_wake.len);
> if (ret)
> goto out;
>
> @@ -1707,7 +1709,7 @@ static int userfaultfd_copy(struct userfaultfd_ctx *ctx,
> sizeof(uffdio_copy)-sizeof(__s64)))
> goto out;
>
> - ret = validate_range(ctx->mm, uffdio_copy.dst, uffdio_copy.len);
> + ret = validate_range(ctx->mm, _copy.dst, uffdio_copy.len);
> if (ret)
> goto out;
> /*
> @@ -1763,7 +1765,7 @@ static int userfaultfd_zeropage(struct userfaultfd_ctx
> *ctx,
> sizeof(uffdio_zeropage)-sizeof(__s64)))
> goto out;
>
> - ret = validate_range(ctx->mm, uffdio_zeropage.range.start,
> + ret = validate_range(ctx->mm, _zeropage.range.start,
>uffdio_zeropage.range.len);
> if (ret)
> goto out;
> --
> 2.22.0.rc1.311.g5d7573a151-goog
>
--
Kees Cook
Re: [PATCH v16 08/16] fs, arm64: untag user pointers in copy_mount_options
On Mon, Jun 03, 2019 at 06:55:10PM +0200, Andrey Konovalov wrote: > This patch is a part of a series that extends arm64 kernel ABI to allow to > pass tagged user pointers (with the top byte set to something else other > than 0x00) as syscall arguments. > > In copy_mount_options a user address is being subtracted from TASK_SIZE. > If the address is lower than TASK_SIZE, the size is calculated to not > allow the exact_copy_from_user() call to cross TASK_SIZE boundary. > However if the address is tagged, then the size will be calculated > incorrectly. > > Untag the address before subtracting. > > Reviewed-by: Catalin Marinas > Signed-off-by: Andrey Konovalov One thing I just noticed in the commit titles... "arm64" is in the prefix, but these are arch-indep areas. Should the ", arm64" be left out? I would expect, instead: fs/namespace: untag user pointers in copy_mount_options Reviewed-by: Kees Cook -Kees > --- > fs/namespace.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/namespace.c b/fs/namespace.c > index b26778bdc236..2e85712a19ed 100644 > --- a/fs/namespace.c > +++ b/fs/namespace.c > @@ -2993,7 +2993,7 @@ void *copy_mount_options(const void __user * data) >* the remainder of the page. >*/ > /* copy_from_user cannot cross TASK_SIZE ! */ > - size = TASK_SIZE - (unsigned long)data; > + size = TASK_SIZE - (unsigned long)untagged_addr(data); > if (size > PAGE_SIZE) > size = PAGE_SIZE; > > -- > 2.22.0.rc1.311.g5d7573a151-goog > -- Kees Cook
Re: [PATCH v16 07/16] mm, arm64: untag user pointers in get_vaddr_frames
On Mon, Jun 03, 2019 at 06:55:09PM +0200, Andrey Konovalov wrote: > This patch is a part of a series that extends arm64 kernel ABI to allow to > pass tagged user pointers (with the top byte set to something else other > than 0x00) as syscall arguments. > > get_vaddr_frames uses provided user pointers for vma lookups, which can > only by done with untagged pointers. Instead of locating and changing > all callers of this function, perform untagging in it. > > Signed-off-by: Andrey Konovalov Reviewed-by: Kees Cook -Kees > --- > mm/frame_vector.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/mm/frame_vector.c b/mm/frame_vector.c > index c64dca6e27c2..c431ca81dad5 100644 > --- a/mm/frame_vector.c > +++ b/mm/frame_vector.c > @@ -46,6 +46,8 @@ int get_vaddr_frames(unsigned long start, unsigned int > nr_frames, > if (WARN_ON_ONCE(nr_frames > vec->nr_allocated)) > nr_frames = vec->nr_allocated; > > + start = untagged_addr(start); > + > down_read(>mmap_sem); > locked = 1; > vma = find_vma_intersection(mm, start, start + 1); > -- > 2.22.0.rc1.311.g5d7573a151-goog > -- Kees Cook
Re: [PATCH v16 06/16] mm, arm64: untag user pointers in mm/gup.c
On Mon, Jun 03, 2019 at 06:55:08PM +0200, Andrey Konovalov wrote: > This patch is a part of a series that extends arm64 kernel ABI to allow to > pass tagged user pointers (with the top byte set to something else other > than 0x00) as syscall arguments. > > mm/gup.c provides a kernel interface that accepts user addresses and > manipulates user pages directly (for example get_user_pages, that is used > by the futex syscall). Since a user can provided tagged addresses, we need > to handle this case. > > Add untagging to gup.c functions that use user addresses for vma lookups. > > Reviewed-by: Catalin Marinas > Signed-off-by: Andrey Konovalov Reviewed-by: Kees Cook -Kees > --- > mm/gup.c | 4 > 1 file changed, 4 insertions(+) > > diff --git a/mm/gup.c b/mm/gup.c > index ddde097cf9e4..c37df3d455a2 100644 > --- a/mm/gup.c > +++ b/mm/gup.c > @@ -802,6 +802,8 @@ static long __get_user_pages(struct task_struct *tsk, > struct mm_struct *mm, > if (!nr_pages) > return 0; > > + start = untagged_addr(start); > + > VM_BUG_ON(!!pages != !!(gup_flags & FOLL_GET)); > > /* > @@ -964,6 +966,8 @@ int fixup_user_fault(struct task_struct *tsk, struct > mm_struct *mm, > struct vm_area_struct *vma; > vm_fault_t ret, major = 0; > > + address = untagged_addr(address); > + > if (unlocked) > fault_flags |= FAULT_FLAG_ALLOW_RETRY; > > -- > 2.22.0.rc1.311.g5d7573a151-goog > -- Kees Cook
Re: [PATCH v16 15/16] vfio/type1, arm64: untag user pointers in vaddr_get_pfn
On Mon, Jun 03, 2019 at 06:55:17PM +0200, Andrey Konovalov wrote:
> This patch is a part of a series that extends arm64 kernel ABI to allow to
> pass tagged user pointers (with the top byte set to something else other
> than 0x00) as syscall arguments.
>
> vaddr_get_pfn() uses provided user pointers for vma lookups, which can
> only by done with untagged pointers.
>
> Untag user pointers in this function.
>
> Signed-off-by: Andrey Konovalov
Reviewed-by: Kees Cook
-Kees
> ---
> drivers/vfio/vfio_iommu_type1.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> index 3ddc375e7063..528e39a1c2dd 100644
> --- a/drivers/vfio/vfio_iommu_type1.c
> +++ b/drivers/vfio/vfio_iommu_type1.c
> @@ -384,6 +384,8 @@ static int vaddr_get_pfn(struct mm_struct *mm, unsigned
> long vaddr,
>
> down_read(>mmap_sem);
>
> + vaddr = untagged_addr(vaddr);
> +
> vma = find_vma_intersection(mm, vaddr, vaddr + 1);
>
> if (vma && vma->vm_flags & VM_PFNMAP) {
> --
> 2.22.0.rc1.311.g5d7573a151-goog
>
--
Kees Cook
Re: [PATCH v16 16/16] selftests, arm64: add a selftest for passing tagged pointers to kernel
On Mon, Jun 03, 2019 at 06:55:18PM +0200, Andrey Konovalov wrote:
> This patch is a part of a series that extends arm64 kernel ABI to allow to
> pass tagged user pointers (with the top byte set to something else other
> than 0x00) as syscall arguments.
>
> This patch adds a simple test, that calls the uname syscall with a
> tagged user pointer as an argument. Without the kernel accepting tagged
> user pointers the test fails with EFAULT.
>
> Signed-off-by: Andrey Konovalov
I'm adding Shuah to CC in case she has some suggestions about the new
selftest.
Reviewed-by: Kees Cook
-Kees
> ---
> tools/testing/selftests/arm64/.gitignore | 1 +
> tools/testing/selftests/arm64/Makefile| 22 ++
> .../testing/selftests/arm64/run_tags_test.sh | 12 ++
> tools/testing/selftests/arm64/tags_lib.c | 42 +++
> tools/testing/selftests/arm64/tags_test.c | 18
> 5 files changed, 95 insertions(+)
> create mode 100644 tools/testing/selftests/arm64/.gitignore
> create mode 100644 tools/testing/selftests/arm64/Makefile
> create mode 100755 tools/testing/selftests/arm64/run_tags_test.sh
> create mode 100644 tools/testing/selftests/arm64/tags_lib.c
> create mode 100644 tools/testing/selftests/arm64/tags_test.c
>
> diff --git a/tools/testing/selftests/arm64/.gitignore
> b/tools/testing/selftests/arm64/.gitignore
> new file mode 100644
> index ..e8fae8d61ed6
> --- /dev/null
> +++ b/tools/testing/selftests/arm64/.gitignore
> @@ -0,0 +1 @@
> +tags_test
> diff --git a/tools/testing/selftests/arm64/Makefile
> b/tools/testing/selftests/arm64/Makefile
> new file mode 100644
> index ..9dee18727923
> --- /dev/null
> +++ b/tools/testing/selftests/arm64/Makefile
> @@ -0,0 +1,22 @@
> +# SPDX-License-Identifier: GPL-2.0
> +
> +include ../lib.mk
> +
> +# ARCH can be overridden by the user for cross compiling
> +ARCH ?= $(shell uname -m 2>/dev/null || echo not)
> +
> +ifneq (,$(filter $(ARCH),aarch64 arm64))
> +
> +TEST_CUSTOM_PROGS := $(OUTPUT)/tags_test
> +
> +$(OUTPUT)/tags_test: tags_test.c $(OUTPUT)/tags_lib.so
> + $(CC) -o $@ $(CFLAGS) $(LDFLAGS) $<
> +
> +$(OUTPUT)/tags_lib.so: tags_lib.c
> + $(CC) -o $@ -shared $(CFLAGS) $(LDFLAGS) $^
> +
> +TEST_PROGS := run_tags_test.sh
> +
> +all: $(TEST_CUSTOM_PROGS)
> +
> +endif
> diff --git a/tools/testing/selftests/arm64/run_tags_test.sh
> b/tools/testing/selftests/arm64/run_tags_test.sh
> new file mode 100755
> index ..2bbe0cd4220b
> --- /dev/null
> +++ b/tools/testing/selftests/arm64/run_tags_test.sh
> @@ -0,0 +1,12 @@
> +#!/bin/sh
> +# SPDX-License-Identifier: GPL-2.0
> +
> +echo ""
> +echo "running tags test"
> +echo ""
> +LD_PRELOAD=./tags_lib.so ./tags_test
> +if [ $? -ne 0 ]; then
> + echo "[FAIL]"
> +else
> + echo "[PASS]"
> +fi
> diff --git a/tools/testing/selftests/arm64/tags_lib.c
> b/tools/testing/selftests/arm64/tags_lib.c
> new file mode 100644
> index ..8a674509216e
> --- /dev/null
> +++ b/tools/testing/selftests/arm64/tags_lib.c
> @@ -0,0 +1,42 @@
> +#include
> +
> +#define TAG_SHIFT(56)
> +#define TAG_MASK (0xffUL << TAG_SHIFT)
> +
> +void *__libc_malloc(size_t size);
> +void __libc_free(void *ptr);
> +void *__libc_realloc(void *ptr, size_t size);
> +void *__libc_calloc(size_t nmemb, size_t size);
> +
> +static void *tag_ptr(void *ptr)
> +{
> + unsigned long tag = rand() & 0xff;
> + if (!ptr)
> + return ptr;
> + return (void *)((unsigned long)ptr | (tag << TAG_SHIFT));
> +}
> +
> +static void *untag_ptr(void *ptr)
> +{
> + return (void *)((unsigned long)ptr & ~TAG_MASK);
> +}
> +
> +void *malloc(size_t size)
> +{
> + return tag_ptr(__libc_malloc(size));
> +}
> +
> +void free(void *ptr)
> +{
> + __libc_free(untag_ptr(ptr));
> +}
> +
> +void *realloc(void *ptr, size_t size)
> +{
> + return tag_ptr(__libc_realloc(untag_ptr(ptr), size));
> +}
> +
> +void *calloc(size_t nmemb, size_t size)
> +{
> + return tag_ptr(__libc_calloc(nmemb, size));
> +}
> diff --git a/tools/testing/selftests/arm64/tags_test.c
> b/tools/testing/selftests/arm64/tags_test.c
> new file mode 100644
> index 0000..263b302874ed
> --- /dev/null
> +++ b/tools/testing/selftests/arm64/tags_test.c
> @@ -0,0 +1,18 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +int main(void)
> +{
> + struct utsname *ptr;
> + int err;
> +
> + ptr = (struct utsname *)malloc(sizeof(*ptr));
> + err = uname(ptr);
> + free(ptr);
> + return err;
> +}
> --
> 2.22.0.rc1.311.g5d7573a151-goog
>
--
Kees Cook
Re: [PATCH v16 13/16] media/v4l2-core, arm64: untag user pointers in videobuf_dma_contig_user_get
On Mon, Jun 03, 2019 at 06:55:15PM +0200, Andrey Konovalov wrote:
> This patch is a part of a series that extends arm64 kernel ABI to allow to
> pass tagged user pointers (with the top byte set to something else other
> than 0x00) as syscall arguments.
>
> videobuf_dma_contig_user_get() uses provided user pointers for vma
> lookups, which can only by done with untagged pointers.
>
> Untag the pointers in this function.
>
> Acked-by: Mauro Carvalho Chehab
> Signed-off-by: Andrey Konovalov
Reviewed-by: Kees Cook
-Kees
> ---
> drivers/media/v4l2-core/videobuf-dma-contig.c | 9 +
> 1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/videobuf-dma-contig.c
> b/drivers/media/v4l2-core/videobuf-dma-contig.c
> index e1bf50df4c70..8a1ddd146b17 100644
> --- a/drivers/media/v4l2-core/videobuf-dma-contig.c
> +++ b/drivers/media/v4l2-core/videobuf-dma-contig.c
> @@ -160,6 +160,7 @@ static void videobuf_dma_contig_user_put(struct
> videobuf_dma_contig_memory *mem)
> static int videobuf_dma_contig_user_get(struct videobuf_dma_contig_memory
> *mem,
> struct videobuf_buffer *vb)
> {
> + unsigned long untagged_baddr = untagged_addr(vb->baddr);
> struct mm_struct *mm = current->mm;
> struct vm_area_struct *vma;
> unsigned long prev_pfn, this_pfn;
> @@ -167,22 +168,22 @@ static int videobuf_dma_contig_user_get(struct
> videobuf_dma_contig_memory *mem,
> unsigned int offset;
> int ret;
>
> - offset = vb->baddr & ~PAGE_MASK;
> + offset = untagged_baddr & ~PAGE_MASK;
> mem->size = PAGE_ALIGN(vb->size + offset);
> ret = -EINVAL;
>
> down_read(>mmap_sem);
>
> - vma = find_vma(mm, vb->baddr);
> + vma = find_vma(mm, untagged_baddr);
> if (!vma)
> goto out_up;
>
> - if ((vb->baddr + mem->size) > vma->vm_end)
> + if ((untagged_baddr + mem->size) > vma->vm_end)
> goto out_up;
>
> pages_done = 0;
> prev_pfn = 0; /* kill warning */
> - user_address = vb->baddr;
> + user_address = untagged_baddr;
>
> while (pages_done < (mem->size >> PAGE_SHIFT)) {
> ret = follow_pfn(vma, user_address, _pfn);
> --
> 2.22.0.rc1.311.g5d7573a151-goog
>
--
Kees Cook
Re: [PATCH v16 02/16] arm64: untag user pointers in access_ok and __uaccess_mask_ptr
On Mon, Jun 03, 2019 at 06:55:04PM +0200, Andrey Konovalov wrote: > This patch is a part of a series that extends arm64 kernel ABI to allow to > pass tagged user pointers (with the top byte set to something else other > than 0x00) as syscall arguments. > > copy_from_user (and a few other similar functions) are used to copy data > from user memory into the kernel memory or vice versa. Since a user can > provided a tagged pointer to one of the syscalls that use copy_from_user, > we need to correctly handle such pointers. > > Do this by untagging user pointers in access_ok and in __uaccess_mask_ptr, > before performing access validity checks. > > Note, that this patch only temporarily untags the pointers to perform the > checks, but then passes them as is into the kernel internals. > > Reviewed-by: Catalin Marinas > Signed-off-by: Andrey Konovalov Reviewed-by: Kees Cook -Kees > --- > arch/arm64/include/asm/uaccess.h | 10 ++ > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/arch/arm64/include/asm/uaccess.h > b/arch/arm64/include/asm/uaccess.h > index e5d5f31c6d36..9164ecb5feca 100644 > --- a/arch/arm64/include/asm/uaccess.h > +++ b/arch/arm64/include/asm/uaccess.h > @@ -94,7 +94,7 @@ static inline unsigned long __range_ok(const void __user > *addr, unsigned long si > return ret; > } > > -#define access_ok(addr, size)__range_ok(addr, size) > +#define access_ok(addr, size)__range_ok(untagged_addr(addr), size) > #define user_addr_maxget_fs > > #define _ASM_EXTABLE(from, to) > \ > @@ -226,7 +226,8 @@ static inline void uaccess_enable_not_uao(void) > > /* > * Sanitise a uaccess pointer such that it becomes NULL if above the > - * current addr_limit. > + * current addr_limit. In case the pointer is tagged (has the top byte set), > + * untag the pointer before checking. > */ > #define uaccess_mask_ptr(ptr) (__typeof__(ptr))__uaccess_mask_ptr(ptr) > static inline void __user *__uaccess_mask_ptr(const void __user *ptr) > @@ -234,10 +235,11 @@ static inline void __user *__uaccess_mask_ptr(const > void __user *ptr) > void __user *safe_ptr; > > asm volatile( > - " bicsxzr, %1, %2\n" > + " bicsxzr, %3, %2\n" > " csel%0, %1, xzr, eq\n" > : "=" (safe_ptr) > - : "r" (ptr), "r" (current_thread_info()->addr_limit) > + : "r" (ptr), "r" (current_thread_info()->addr_limit), > + "r" (untagged_addr(ptr)) > : "cc"); > > csdb(); > -- > 2.22.0.rc1.311.g5d7573a151-goog > -- Kees Cook
Re: [PATCH v16 05/16] arm64: untag user pointers passed to memory syscalls
On Mon, Jun 03, 2019 at 06:55:07PM +0200, Andrey Konovalov wrote:
> This patch is a part of a series that extends arm64 kernel ABI to allow to
> pass tagged user pointers (with the top byte set to something else other
> than 0x00) as syscall arguments.
>
> This patch allows tagged pointers to be passed to the following memory
> syscalls: get_mempolicy, madvise, mbind, mincore, mlock, mlock2, mprotect,
> mremap, msync, munlock.
>
> Signed-off-by: Andrey Konovalov
Reviewed-by: Kees Cook
-Kees
> ---
> mm/madvise.c | 2 ++
> mm/mempolicy.c | 3 +++
> mm/mincore.c | 2 ++
> mm/mlock.c | 4
> mm/mprotect.c | 2 ++
> mm/mremap.c| 2 ++
> mm/msync.c | 2 ++
> 7 files changed, 17 insertions(+)
>
> diff --git a/mm/madvise.c b/mm/madvise.c
> index 628022e674a7..39b82f8a698f 100644
> --- a/mm/madvise.c
> +++ b/mm/madvise.c
> @@ -810,6 +810,8 @@ SYSCALL_DEFINE3(madvise, unsigned long, start, size_t,
> len_in, int, behavior)
> size_t len;
> struct blk_plug plug;
>
> + start = untagged_addr(start);
> +
> if (!madvise_behavior_valid(behavior))
> return error;
>
> diff --git a/mm/mempolicy.c b/mm/mempolicy.c
> index 01600d80ae01..78e0a88b2680 100644
> --- a/mm/mempolicy.c
> +++ b/mm/mempolicy.c
> @@ -1360,6 +1360,7 @@ static long kernel_mbind(unsigned long start, unsigned
> long len,
> int err;
> unsigned short mode_flags;
>
> + start = untagged_addr(start);
> mode_flags = mode & MPOL_MODE_FLAGS;
> mode &= ~MPOL_MODE_FLAGS;
> if (mode >= MPOL_MAX)
> @@ -1517,6 +1518,8 @@ static int kernel_get_mempolicy(int __user *policy,
> int uninitialized_var(pval);
> nodemask_t nodes;
>
> + addr = untagged_addr(addr);
> +
> if (nmask != NULL && maxnode < nr_node_ids)
> return -EINVAL;
>
> diff --git a/mm/mincore.c b/mm/mincore.c
> index c3f058bd0faf..64c322ed845c 100644
> --- a/mm/mincore.c
> +++ b/mm/mincore.c
> @@ -249,6 +249,8 @@ SYSCALL_DEFINE3(mincore, unsigned long, start, size_t,
> len,
> unsigned long pages;
> unsigned char *tmp;
>
> + start = untagged_addr(start);
> +
> /* Check the start address: needs to be page-aligned.. */
> if (start & ~PAGE_MASK)
> return -EINVAL;
> diff --git a/mm/mlock.c b/mm/mlock.c
> index 080f3b36415b..e82609eaa428 100644
> --- a/mm/mlock.c
> +++ b/mm/mlock.c
> @@ -674,6 +674,8 @@ static __must_check int do_mlock(unsigned long start,
> size_t len, vm_flags_t fla
> unsigned long lock_limit;
> int error = -ENOMEM;
>
> + start = untagged_addr(start);
> +
> if (!can_do_mlock())
> return -EPERM;
>
> @@ -735,6 +737,8 @@ SYSCALL_DEFINE2(munlock, unsigned long, start, size_t,
> len)
> {
> int ret;
>
> + start = untagged_addr(start);
> +
> len = PAGE_ALIGN(len + (offset_in_page(start)));
> start &= PAGE_MASK;
>
> diff --git a/mm/mprotect.c b/mm/mprotect.c
> index bf38dfbbb4b4..19f981b733bc 100644
> --- a/mm/mprotect.c
> +++ b/mm/mprotect.c
> @@ -465,6 +465,8 @@ static int do_mprotect_pkey(unsigned long start, size_t
> len,
> const bool rier = (current->personality & READ_IMPLIES_EXEC) &&
> (prot & PROT_READ);
>
> + start = untagged_addr(start);
> +
> prot &= ~(PROT_GROWSDOWN|PROT_GROWSUP);
> if (grows == (PROT_GROWSDOWN|PROT_GROWSUP)) /* can't be both */
> return -EINVAL;
> diff --git a/mm/mremap.c b/mm/mremap.c
> index fc241d23cd97..1d98281f7204 100644
> --- a/mm/mremap.c
> +++ b/mm/mremap.c
> @@ -606,6 +606,8 @@ SYSCALL_DEFINE5(mremap, unsigned long, addr, unsigned
> long, old_len,
> LIST_HEAD(uf_unmap_early);
> LIST_HEAD(uf_unmap);
>
> + addr = untagged_addr(addr);
> +
> if (flags & ~(MREMAP_FIXED | MREMAP_MAYMOVE))
> return ret;
>
> diff --git a/mm/msync.c b/mm/msync.c
> index ef30a429623a..c3bd3e75f687 100644
> --- a/mm/msync.c
> +++ b/mm/msync.c
> @@ -37,6 +37,8 @@ SYSCALL_DEFINE3(msync, unsigned long, start, size_t, len,
> int, flags)
> int unmapped_error = 0;
> int error = -EINVAL;
>
> + start = untagged_addr(start);
> +
> if (flags & ~(MS_ASYNC | MS_INVALIDATE | MS_SYNC))
> goto out;
> if (offset_in_page(start))
> --
> 2.22.0.rc1.311.g5d7573a151-goog
>
--
Kees Cook
Re: [PATCH v16 04/16] mm: untag user pointers in do_pages_move
On Mon, Jun 03, 2019 at 06:55:06PM +0200, Andrey Konovalov wrote: > This patch is a part of a series that extends arm64 kernel ABI to allow to > pass tagged user pointers (with the top byte set to something else other > than 0x00) as syscall arguments. > > do_pages_move() is used in the implementation of the move_pages syscall. > > Untag user pointers in this function. > > Reviewed-by: Catalin Marinas > Signed-off-by: Andrey Konovalov Reviewed-by: Kees Cook -Kees > --- > mm/migrate.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/mm/migrate.c b/mm/migrate.c > index f2ecc2855a12..3930bb6fa656 100644 > --- a/mm/migrate.c > +++ b/mm/migrate.c > @@ -1617,6 +1617,7 @@ static int do_pages_move(struct mm_struct *mm, > nodemask_t task_nodes, > if (get_user(node, nodes + i)) > goto out_flush; > addr = (unsigned long)p; > + addr = untagged_addr(addr); > > err = -ENODEV; > if (node < 0 || node >= MAX_NUMNODES) > -- > 2.22.0.rc1.311.g5d7573a151-goog > -- Kees Cook
Re: [PATCH v16 03/16] lib, arm64: untag user pointers in strn*_user
On Mon, Jun 03, 2019 at 06:55:05PM +0200, Andrey Konovalov wrote:
> This patch is a part of a series that extends arm64 kernel ABI to allow to
> pass tagged user pointers (with the top byte set to something else other
> than 0x00) as syscall arguments.
>
> strncpy_from_user and strnlen_user accept user addresses as arguments, and
> do not go through the same path as copy_from_user and others, so here we
> need to handle the case of tagged user addresses separately.
>
> Untag user pointers passed to these functions.
>
> Note, that this patch only temporarily untags the pointers to perform
> validity checks, but then uses them as is to perform user memory accesses.
>
> Reviewed-by: Catalin Marinas
> Signed-off-by: Andrey Konovalov
Acked-by: Kees Cook
-Kees
> ---
> lib/strncpy_from_user.c | 3 ++-
> lib/strnlen_user.c | 3 ++-
> 2 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/lib/strncpy_from_user.c b/lib/strncpy_from_user.c
> index 023ba9f3b99f..dccb95af6003 100644
> --- a/lib/strncpy_from_user.c
> +++ b/lib/strncpy_from_user.c
> @@ -6,6 +6,7 @@
> #include
> #include
> #include
> +#include
>
> #include
> #include
> @@ -108,7 +109,7 @@ long strncpy_from_user(char *dst, const char __user *src,
> long count)
> return 0;
>
> max_addr = user_addr_max();
> - src_addr = (unsigned long)src;
> + src_addr = (unsigned long)untagged_addr(src);
> if (likely(src_addr < max_addr)) {
> unsigned long max = max_addr - src_addr;
> long retval;
> diff --git a/lib/strnlen_user.c b/lib/strnlen_user.c
> index 7f2db3fe311f..28ff554a1be8 100644
> --- a/lib/strnlen_user.c
> +++ b/lib/strnlen_user.c
> @@ -2,6 +2,7 @@
> #include
> #include
> #include
> +#include
>
> #include
>
> @@ -109,7 +110,7 @@ long strnlen_user(const char __user *str, long count)
> return 0;
>
> max_addr = user_addr_max();
> - src_addr = (unsigned long)str;
> + src_addr = (unsigned long)untagged_addr(str);
> if (likely(src_addr < max_addr)) {
> unsigned long max = max_addr - src_addr;
> long retval;
> --
> 2.22.0.rc1.311.g5d7573a151-goog
>
--
Kees Cook
Re: [PATCH v15 00/17] arm64: untag user pointers passed to the kernel
On Tue, May 28, 2019 at 06:02:45PM +0100, Catalin Marinas wrote:
> On Thu, May 23, 2019 at 02:31:16PM -0700, Kees Cook wrote:
> > syzkaller already attempts to randomly inject non-canonical and
> > 0x addresses for user pointers in syscalls in an effort to
> > find bugs like CVE-2017-5123 where waitid() via unchecked put_user() was
> > able to write directly to kernel memory[1].
> >
> > It seems that using TBI by default and not allowing a switch back to
> > "normal" ABI without a reboot actually means that userspace cannot inject
> > kernel pointers into syscalls any more, since they'll get universally
> > stripped now. Is my understanding correct, here? i.e. exploiting
> > CVE-2017-5123 would be impossible under TBI?
> >
> > If so, then I think we should commit to the TBI ABI and have a boot
> > flag to disable it, but NOT have a process flag, as that would allow
> > attackers to bypass the masking. The only flag should be "TBI or MTE".
> >
> > If so, can I get top byte masking for other architectures too? Like,
> > just to strip high bits off userspace addresses? ;)
>
> Just for fun, hack/attempt at your idea which should not interfere with
> TBI. Only briefly tested on arm64 (and the s390 __TYPE_IS_PTR macro is
> pretty weird ;)):
OMG, this is amazing and bonkers. I love it.
> --8<-
> diff --git a/arch/s390/include/asm/compat.h b/arch/s390/include/asm/compat.h
> index 63b46e30b2c3..338455a74eff 100644
> --- a/arch/s390/include/asm/compat.h
> +++ b/arch/s390/include/asm/compat.h
> @@ -11,9 +11,6 @@
>
> #include
>
> -#define __TYPE_IS_PTR(t) (!__builtin_types_compatible_p( \
> - typeof(0?(__force t)0:0ULL), u64))
> -
> #define __SC_DELOUSE(t,v) ({ \
> BUILD_BUG_ON(sizeof(t) > 4 && !__TYPE_IS_PTR(t)); \
> (__force t)(__TYPE_IS_PTR(t) ? ((v) & 0x7fff) : (v)); \
> diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
> index e2870fe1be5b..b1b9fe8502da 100644
> --- a/include/linux/syscalls.h
> +++ b/include/linux/syscalls.h
> @@ -119,8 +119,15 @@ struct io_uring_params;
> #define __TYPE_IS_L(t) (__TYPE_AS(t, 0L))
> #define __TYPE_IS_UL(t) (__TYPE_AS(t, 0UL))
> #define __TYPE_IS_LL(t) (__TYPE_AS(t, 0LL) || __TYPE_AS(t, 0ULL))
> +#define __TYPE_IS_PTR(t) (!__builtin_types_compatible_p(typeof(0 ? (__force
> t)0 : 0ULL), u64))
> #define __SC_LONG(t, a) __typeof(__builtin_choose_expr(__TYPE_IS_LL(t), 0LL,
> 0L)) a
> +#ifdef CONFIG_64BIT
> +#define __SC_CAST(t, a) (__TYPE_IS_PTR(t) \
> + ? (__force t) ((__u64)a & ~(1UL << 55)) \
> + : (__force t) a)
> +#else
> #define __SC_CAST(t, a) (__force t) a
> +#endif
> #define __SC_ARGS(t, a) a
> #define __SC_TEST(t, a) (void)BUILD_BUG_ON_ZERO(!__TYPE_IS_LL(t) &&
> sizeof(t) > sizeof(long))
I'm laughing, I'm crying. Now I have to go look at the disassembly to
see how this actually looks. (I mean, it _does_ solve my specific case
of the waitid() flaw, but wouldn't help with pointers deeper in structs,
etc, though TBI does, I think still help with that. I have to catch back
up on the thread...) Anyway, if it's not too expensive it'd block
reachability for those kinds of flaws.
I wonder what my chances are of actually getting this landed? :)
(Though, I guess I need to find a "VA width" macro instead of a raw 55.)
Thanks for thinking of __SC_CAST() and __TYPE_IS_PTR() together. Really
made my day. :)
--
Kees Cook
Re: [PATCH v15 00/17] arm64: untag user pointers passed to the kernel
On Thu, May 23, 2019 at 06:43:46PM +0100, Catalin Marinas wrote: > On Thu, May 23, 2019 at 09:38:19AM -0700, Kees Cook wrote: > > What on this front would you be comfortable with? Given it's a new > > feature isn't it sufficient to have a CONFIG (and/or boot option)? > > I'd rather avoid re-building kernels. A boot option would do, unless we > see value in a per-process (inherited) personality or prctl. The I think I've convinced myself that the normal<->TBI ABI control should be a boot parameter. More below... > > What about testing tools that intentionally insert high bits for syscalls > > and are _expecting_ them to fail? It seems the TBI series will break them? > > In that case, do we need to opt into TBI as well? > > If there are such tools, then we may need a per-process control. It's > basically an ABI change. syzkaller already attempts to randomly inject non-canonical and 0x addresses for user pointers in syscalls in an effort to find bugs like CVE-2017-5123 where waitid() via unchecked put_user() was able to write directly to kernel memory[1]. It seems that using TBI by default and not allowing a switch back to "normal" ABI without a reboot actually means that userspace cannot inject kernel pointers into syscalls any more, since they'll get universally stripped now. Is my understanding correct, here? i.e. exploiting CVE-2017-5123 would be impossible under TBI? If so, then I think we should commit to the TBI ABI and have a boot flag to disable it, but NOT have a process flag, as that would allow attackers to bypass the masking. The only flag should be "TBI or MTE". If so, can I get top byte masking for other architectures too? Like, just to strip high bits off userspace addresses? ;) (Oh, in looking I see this is implemented with sign-extension... why not just a mask? So it'll either be valid userspace address or forced into the non-canonical range?) [1] https://salls.github.io/Linux-Kernel-CVE-2017-5123/ > > Alright, the tl;dr appears to be: > > - you want more assurances that we can find __user stripping in the > > kernel more easily. (But this seems like a parallel problem.) > > Yes, and that we found all (most) cases now. The reason I don't see it > as a parallel problem is that, as maintainer, I promise an ABI to user > and I'd rather stick to it. I don't want, for example, ncurses to stop > working because of some ioctl() rejecting tagged pointers. But this is what I don't understand: it would need to be ncurses _using TBI_, that would stop working (having started to work before, but then regress due to a newly added one-off bug). Regular ncurses will be fine because it's not using TBI. So The Golden Rule isn't violated, and by definition, it's a specific regression caused by some bug (since TBI would have had to have worked _before_ in the situation to be considered a regression now). Which describes the normal path for kernel development... add feature, find corner cases where it doesn't work, fix them, encounter new regressions, fix those, repeat forever. > If it's just the occasional one-off bug I'm fine to deal with it. But > no-one convinced me yet that this is the case. You believe there still to be some systemic cases that haven't been found yet? And even if so -- isn't it better to work on that incrementally? > As for the generic driver code (filesystems or other subsystems), > without some clear direction for developers, together with static > checking/sparse, on how user pointers are cast to longs (one example), > it would become my responsibility to identify and fix them up with any > kernel release. This series is not providing such guidance, just adding > untagged_addr() in some places that we think matter. What about adding a nice bit of .rst documentation that describes the situation and shows how to use untagged_addr(). This is the kind of kernel-wide change that "everyone" needs to know about, and shouldn't be the arch maintainer's sole responsibility to fix. > > - we might need to opt in to TBI with a prctl() > > Yes, although still up for discussion. I think I've talked myself out of it. I say boot param only! :) So what do you say to these next steps: - change untagged_addr() to use a static branch that is controlled with a boot parameter. - add, say, Documentation/core-api/user-addresses.rst to describe proper care and handling of user space pointers with untagged_addr(), with examples based on all the cases seen so far in this series. - continue work to improve static analysis. Thanks for wading through this with me! :) -- Kees Cook
Re: [PATCH v15 00/17] arm64: untag user pointers passed to the kernel
h opting into MTE. I do expect, though, this will feel redundant in a couple years as everything will immediately opt-in. But, okay, this is therefore an issue for the MTE series. > The current plan is that a future binary issues a prctl(), after > checking the HWCAP_MTE bit (as I replied to Elliot, the MTE instructions > are not in the current NOP space). I'd expect this to be done by the > libc or dynamic loader under the assumption that the binaries it loads > do _not_ use the top pointer byte for anything else. With hwasan > compiled objects this gets more confusing (any ELF note to identify > them?). Okay, sounds fine. > (there is also the risk of existing applications using TBI already but > I'm not aware of any still using this feature other than hwasan) Correct. Alright, the tl;dr appears to be: - you want more assurances that we can find __user stripping in the kernel more easily. (But this seems like a parallel problem.) - we might need to opt in to TBI with a prctl() - all other concerns are for the future MTE series (though it sounds like HWCAP_MTE and a prctl() solve those issues too). Is this accurate? What do you see as the blockers for this series at this point? -- Kees Cook
