* [PATCH 8/9] media: docs: v4l2-controls: document file to include
From: Luca Ceresoli @ 2019-06-13 14:18 UTC (permalink / raw)
To: linux-media
Cc: Luca Ceresoli, Mauro Carvalho Chehab, Hans Verkuil, linux-kernel,
linux-doc
In-Reply-To: <20190613141826.26775-1-luca@lucaceresoli.net>
The tutorial in this section is almost complete, add the one missing bit.
Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>
---
Documentation/media/kapi/v4l2-controls.rst | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/Documentation/media/kapi/v4l2-controls.rst b/Documentation/media/kapi/v4l2-controls.rst
index 45541e05a0e7..407617b1d0ce 100644
--- a/Documentation/media/kapi/v4l2-controls.rst
+++ b/Documentation/media/kapi/v4l2-controls.rst
@@ -50,6 +50,10 @@ Basic usage for V4L2 and sub-device drivers
1) Prepare the driver:
+.. code-block:: c
+
+ #include <media/v4l2-ctrls.h>
+
1.1) Add the handler to your driver's top-level struct:
For V4L2 drivers:
--
2.21.0
^ permalink raw reply related
* [PATCH 2/9] media: docs: v4l2-controls: remove repeated ioctl names
From: Luca Ceresoli @ 2019-06-13 14:18 UTC (permalink / raw)
To: linux-media
Cc: Luca Ceresoli, Mauro Carvalho Chehab, Hans Verkuil, linux-kernel,
linux-doc
In-Reply-To: <20190613141826.26775-1-luca@lucaceresoli.net>
Mentioning :ref:`VIDIOC_QUERYCTRL` renders all the three related ioctls.
Explicitly adding VIDIOC_QUERY_EXT_CTRL and VIDIOC_QUERYMENU will make
them render twice, so remove them
Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>
---
Documentation/media/uapi/v4l/extended-controls.rst | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/Documentation/media/uapi/v4l/extended-controls.rst b/Documentation/media/uapi/v4l/extended-controls.rst
index 0968aa9cd167..802a405aa535 100644
--- a/Documentation/media/uapi/v4l/extended-controls.rst
+++ b/Documentation/media/uapi/v4l/extended-controls.rst
@@ -96,9 +96,7 @@ if this control consists of more than one element.
#. It is important to realize that due to the flexibility of controls it is
necessary to check whether the control you want to set actually is
supported in the driver and what the valid range of values is. So use
- the :ref:`VIDIOC_QUERYCTRL` (or :ref:`VIDIOC_QUERY_EXT_CTRL
- <VIDIOC_QUERYCTRL>`) and :ref:`VIDIOC_QUERYMENU <VIDIOC_QUERYCTRL>`
- ioctls to check this.
+ :ref:`VIDIOC_QUERYCTRL` to check this.
#. It is possible that some of the menu indices in a control of
type ``V4L2_CTRL_TYPE_MENU`` may not be supported (``VIDIOC_QUERYMENU``
--
2.21.0
^ permalink raw reply related
* [PATCH 5/9] media: docs: v4l2-controls: rearrange control initialization sequence
From: Luca Ceresoli @ 2019-06-13 14:18 UTC (permalink / raw)
To: linux-media
Cc: Luca Ceresoli, Mauro Carvalho Chehab, Hans Verkuil, linux-kernel,
linux-doc
In-Reply-To: <20190613141826.26775-1-luca@lucaceresoli.net>
The code snippet showing how to add controls to the driver’s top-level
struct is present twice, but only the second time it is split in the V4L2
and subdev cases. Consolidate everything at the beginning.
Also remove the "Where foo->bar is of type struct baz" sentences, this
obvious from the code.
Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>
---
Documentation/media/kapi/v4l2-controls.rst | 40 +++++++++-------------
1 file changed, 17 insertions(+), 23 deletions(-)
diff --git a/Documentation/media/kapi/v4l2-controls.rst b/Documentation/media/kapi/v4l2-controls.rst
index 0c3f486727ed..5281c9b1fd66 100644
--- a/Documentation/media/kapi/v4l2-controls.rst
+++ b/Documentation/media/kapi/v4l2-controls.rst
@@ -52,15 +52,29 @@ Basic usage for V4L2 and sub-device drivers
1.1) Add the handler to your driver's top-level struct:
+For V4L2 drivers:
+
.. code-block:: none
struct foo_dev {
+ ...
+ struct v4l2_device v4l2_dev;
...
struct v4l2_ctrl_handler ctrl_handler;
...
};
- struct foo_dev *foo;
+For sub-device drivers:
+
+.. code-block:: none
+
+ struct foo_dev {
+ ...
+ struct v4l2_subdev sd;
+ ...
+ struct v4l2_ctrl_handler ctrl_handler;
+ ...
+ };
1.2) Initialize the handler:
@@ -74,43 +88,23 @@ information. It is a hint only.
1.3) Hook the control handler into the driver:
-1.3.1) For V4L2 drivers do this:
+For V4L2 drivers:
.. code-block:: none
- struct foo_dev {
- ...
- struct v4l2_device v4l2_dev;
- ...
- struct v4l2_ctrl_handler ctrl_handler;
- ...
- };
-
foo->v4l2_dev.ctrl_handler = &foo->ctrl_handler;
-Where foo->v4l2_dev is of type struct v4l2_device.
-
Finally, remove all control functions from your v4l2_ioctl_ops (if any):
vidioc_queryctrl, vidioc_query_ext_ctrl, vidioc_querymenu, vidioc_g_ctrl,
vidioc_s_ctrl, vidioc_g_ext_ctrls, vidioc_try_ext_ctrls and vidioc_s_ext_ctrls.
Those are now no longer needed.
-1.3.2) For sub-device drivers do this:
+For sub-device drivers:
.. code-block:: none
- struct foo_dev {
- ...
- struct v4l2_subdev sd;
- ...
- struct v4l2_ctrl_handler ctrl_handler;
- ...
- };
-
foo->sd.ctrl_handler = &foo->ctrl_handler;
-Where foo->sd is of type struct v4l2_subdev.
-
1.4) Clean up the handler at the end:
.. code-block:: none
--
2.21.0
^ permalink raw reply related
* Re: [PATCH v4 1/2] arm64: Define Documentation/arm64/tagged-address-abi.txt
From: Vincenzo Frascino @ 2019-06-13 14:03 UTC (permalink / raw)
To: Szabolcs Nagy, Catalin Marinas
Cc: nd, linux-arm-kernel@lists.infradead.org,
linux-doc@vger.kernel.org, linux-mm@kvack.org,
linux-arch@vger.kernel.org, linux-kselftest@vger.kernel.org,
linux-kernel@vger.kernel.org, Will Deacon, Andrey Konovalov,
Alexander Viro
In-Reply-To: <8e3c9537-de10-0d0d-f5bb-c33bde92443f@arm.com>
On 13/06/2019 13:28, Szabolcs Nagy wrote:
> On 13/06/2019 12:16, Vincenzo Frascino wrote:
>> Hi Szabolcs,
>>
>> thank you for your review.
>>
>> On 13/06/2019 11:14, Szabolcs Nagy wrote:
>>> On 13/06/2019 10:20, Catalin Marinas wrote:
>>>> Hi Szabolcs,
>>>>
>>>> On Wed, Jun 12, 2019 at 05:30:34PM +0100, Szabolcs Nagy wrote:
>>>>> On 12/06/2019 15:21, Vincenzo Frascino wrote:
>>>>>> +2. ARM64 Tagged Address ABI
>>>>>> +---------------------------
>>>>>> +
>>>>>> +From the kernel syscall interface prospective, we define, for the purposes
>>>>> ^^^^^^^^^^^
>>>>> perspective
>>>>>
>>>>>> +of this document, a "valid tagged pointer" as a pointer that either it has
>>>>>> +a zero value set in the top byte or it has a non-zero value, it is in memory
>>>>>> +ranges privately owned by a userspace process and it is obtained in one of
>>>>>> +the following ways:
>>>>>> + - mmap() done by the process itself, where either:
>>>>>> + * flags = MAP_PRIVATE | MAP_ANONYMOUS
>>>>>> + * flags = MAP_PRIVATE and the file descriptor refers to a regular
>>>>>> + file or "/dev/zero"
>>>>>
>>>>> this does not make it clear if MAP_FIXED or other flags are valid
>>>>> (there are many map flags i don't know, but at least fixed should work
>>>>> and stack/growsdown. i'd expect anything that's not incompatible with
>>>>> private|anon to work).
>>>>
>>>> Just to clarify, this document tries to define the memory ranges from
>>>> where tagged addresses can be passed into the kernel in the context
>>>> of TBI only (not MTE); that is for hwasan support. FIXED or GROWSDOWN
>>>> should not affect this.
>>>
>>> yes, so either the text should list MAP_* flags that don't affect
>>> the pointer tagging semantics or specify private|anon mapping
>>> with different wording.
>>>
>>
>> Good point. Could you please propose a wording that would be suitable for this case?
>
> i don't know all the MAP_ magic, but i think it's enough to change
> the "flags =" to
>
> * flags have MAP_PRIVATE and MAP_ANONYMOUS set or
> * flags have MAP_PRIVATE set and the file descriptor refers to...
>
>
Fine by me. I will add it the next iterations.
>>>>>> + - a mapping below sbrk(0) done by the process itself
>>>>>
>>>>> doesn't the mmap rule cover this?
>>>>
>>>> IIUC it doesn't cover it as that's memory mapped by the kernel
>>>> automatically on access vs a pointer returned by mmap(). The statement
>>>> above talks about how the address is obtained by the user.
>>>
>>> ok i read 'mapping below sbrk' as an mmap (possibly MAP_FIXED)
>>> that happens to be below the heap area.
>>>
>>> i think "below sbrk(0)" is not the best term to use: there
>>> may be address range below the heap area that can be mmapped
>>> and thus below sbrk(0) and sbrk is a posix api not a linux
>>> syscall, the libc can implement it with mmap or whatever.
>>>
>>> i'm not sure what the right term for 'heap area' is
>>> (the address range between syscall(__NR_brk,0) at
>>> program startup and its current value?)
>>>
>>
>> I used sbrk(0) with the meaning of "end of the process's data segment" not
>> implying that this is a syscall, but just as a useful way to identify the mapping.
>> I agree that it is a posix function implemented by libc but when it is used with
>> 0 finds the current location of the program break, which can be changed by brk()
>> and depending on the new address passed to this syscall can have the effect of
>> allocating or deallocating memory.
>>
>> Will changing sbrk(0) with "end of the process's data segment" make it more clear?
>
> i don't understand what's the relevance of the *end*
> of the data segment.
>
> i'd expect the text to say something about the address
> range of the data segment.
>
> i can do
>
> mmap((void*)65536, 65536, PROT_READ|PROT_WRITE, MAP_FIXED|MAP_SHARED|MAP_ANON, -1, 0);
>
> and it will be below the end of the data segment.
>
As far as I understand the data segment "lives" below the program break, hence
it is a way of describing the range from which the user can obtain a valid
tagged pointer.
Said that, I am not really sure on how do you want me to document this (my aim
is for this to be clear to the userspace developers). Could you please propose
something?
>>
>> I will add what you are suggesting about the heap area.
>>
--
Regards,
Vincenzo
^ permalink raw reply
* Re: [RFC 1/7] tee: optee: allow kernel pages to register as shm
From: Jarkko Sakkinen @ 2019-06-13 15:12 UTC (permalink / raw)
To: Sumit Garg
Cc: keyrings, linux-integrity, linux-security-module, jens.wiklander,
corbet, dhowells, jejb, zohar, jmorris, serge, ard.biesheuvel,
daniel.thompson, linux-doc, linux-kernel, tee-dev
In-Reply-To: <1560421833-27414-2-git-send-email-sumit.garg@linaro.org>
On Thu, Jun 13, 2019 at 04:00:27PM +0530, Sumit Garg wrote:
> Kernel pages are marked as normal type memory only so allow kernel pages
> to be registered as shared memory with OP-TEE.
>
> Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
Just out of pure interest why this was not allowed before?
/Jarkko
^ permalink raw reply
* Re: [PATCH v7 22/27] binfmt_elf: Extract .note.gnu.property from an ELF file
From: Dave Martin @ 2019-06-13 13:26 UTC (permalink / raw)
To: Yu-cheng Yu
Cc: Florian Weimer, x86, H. Peter Anvin, Thomas Gleixner, Ingo Molnar,
linux-kernel, linux-doc, linux-mm, linux-arch, linux-api,
Arnd Bergmann, Andy Lutomirski, Balbir Singh, Borislav Petkov,
Cyrill Gorcunov, Dave Hansen, Eugene Syromiatnikov, H.J. Lu,
Jann Horn, Jonathan Corbet, Kees Cook, Mike Kravetz, Nadav Amit,
Oleg Nesterov, Pavel Machek, Peter Zijlstra, Randy Dunlap,
Ravi V. Shankar, Vedvyas Shanbhogue
In-Reply-To: <b8fb6626a6ae415fac4d5daa86225e4c68d56673.camel@intel.com>
On Wed, Jun 12, 2019 at 12:04:01PM -0700, Yu-cheng Yu wrote:
> On Wed, 2019-06-12 at 10:32 +0100, Dave Martin wrote:
> > On Tue, Jun 11, 2019 at 12:31:34PM -0700, Yu-cheng Yu wrote:
> > > On Tue, 2019-06-11 at 12:41 +0100, Dave Martin wrote:
> > > > On Mon, Jun 10, 2019 at 07:24:43PM +0200, Florian Weimer wrote:
> > > > > * Yu-cheng Yu:
> > > > >
> > > > > > To me, looking at PT_GNU_PROPERTY and not trying to support anything
> > > > > > is a
> > > > > > logical choice. And it breaks only a limited set of toolchains.
> > > > > >
> > > > > > I will simplify the parser and leave this patch as-is for anyone who
> > > > > > wants
> > > > > > to
> > > > > > back-port. Are there any objections or concerns?
> > > > >
> > > > > Red Hat Enterprise Linux 8 does not use PT_GNU_PROPERTY and is probably
> > > > > the largest collection of CET-enabled binaries that exists today.
> > > >
> > > > For clarity, RHEL is actively parsing these properties today?
> > > >
> > > > > My hope was that we would backport the upstream kernel patches for CET,
> > > > > port the glibc dynamic loader to the new kernel interface, and be ready
> > > > > to run with CET enabled in principle (except that porting userspace
> > > > > libraries such as OpenSSL has not really started upstream, so many
> > > > > processes where CET is particularly desirable will still run without
> > > > > it).
> > > > >
> > > > > I'm not sure if it is a good idea to port the legacy support if it's not
> > > > > part of the mainline kernel because it comes awfully close to creating
> > > > > our own private ABI.
> > > >
> > > > I guess we can aim to factor things so that PT_NOTE scanning is
> > > > available as a fallback on arches for which the absence of
> > > > PT_GNU_PROPERTY is not authoritative.
> > >
> > > We can probably check PT_GNU_PROPERTY first, and fallback (based on ld-linux
> > > version?) to PT_NOTE scanning?
> >
> > For arm64, we can check for PT_GNU_PROPERTY and then give up
> > unconditionally.
> >
> > For x86, we would fall back to PT_NOTE scanning, but this will add a bit
> > of cost to binaries that don't have NT_GNU_PROPERTY_TYPE_0. The ld.so
> > version doesn't tell you what ELF ABI a given executable conforms to.
> >
> > Since this sounds like it's largely a distro-specific issue, maybe there
> > could be a Kconfig option to turn the fallback PT_NOTE scanning on?
>
> Yes, I will make it a Kconfig option.
OK, that works for me. This would also help keep the PT_NOTE scanning
separate from the rest of the code.
For arm64 we could then unconditionally select/deselect that option,
where x86 could leave it configurable either way.
Cheers
---Dave
^ permalink raw reply
* Re: [PATCH v4 1/2] arm64: Define Documentation/arm64/tagged-address-abi.txt
From: Dave Martin @ 2019-06-13 13:23 UTC (permalink / raw)
To: Catalin Marinas
Cc: linux-arch, linux-doc, Szabolcs Nagy, Andrey Konovalov,
Will Deacon, linux-kernel, linux-mm, Alexander Viro,
linux-kselftest, Vincenzo Frascino, linux-arm-kernel
In-Reply-To: <20190613122821.GS28951@C02TF0J2HF1T.local>
On Thu, Jun 13, 2019 at 01:28:21PM +0100, Catalin Marinas wrote:
> On Thu, Jun 13, 2019 at 12:37:32PM +0100, Dave P Martin wrote:
> > On Thu, Jun 13, 2019 at 11:15:34AM +0100, Vincenzo Frascino wrote:
> > > On 12/06/2019 16:35, Catalin Marinas wrote:
> > > > On Wed, Jun 12, 2019 at 03:21:10PM +0100, Vincenzo Frascino wrote:
> > > >> + - PR_GET_TAGGED_ADDR_CTRL: can be used to check the status of the Tagged
> > > >> + Address ABI.
> [...]
> > Is there a canonical way to detect whether this whole API/ABI is
> > available? (i.e., try to call this prctl / check for an HWCAP bit,
> > etc.)
>
> The canonical way is a prctl() call. HWCAP doesn't make sense since it's
> not a hardware feature. If you really want a different way of detecting
> this (which I don't think it's worth), we can reinstate the AT_FLAGS
> bit.
Sure, I think this probably makes sense -- I'm still getting my around
which parts of the design are directly related to MTE and which aren't.
I was a bit concerned about the interaction between
PR_SET_TAGGED_ADDR_CTRL and the sysctl: the caller might conclude that
this API is unavailable when actually tagged addresses are stuck on.
I'm not sure whether this matters, but it's a bit weird.
One option would be to change the semantics, so that the sysctl just
forbids turning tagging from off to on. Alternatively, we could return
a different error code to distinguish this case.
Or we just leave it as proposed.
Cheers
---Dave
^ permalink raw reply
* Re: [RFC 1/7] tee: optee: allow kernel pages to register as shm
From: Jarkko Sakkinen @ 2019-06-13 15:17 UTC (permalink / raw)
To: Sumit Garg
Cc: keyrings, linux-integrity, linux-security-module, jens.wiklander,
corbet, dhowells, jejb, zohar, jmorris, serge, ard.biesheuvel,
daniel.thompson, linux-doc, linux-kernel, tee-dev
In-Reply-To: <20190613151257.GA18488@linux.intel.com>
On Thu, Jun 13, 2019 at 06:12:57PM +0300, Jarkko Sakkinen wrote:
> On Thu, Jun 13, 2019 at 04:00:27PM +0530, Sumit Garg wrote:
> > Kernel pages are marked as normal type memory only so allow kernel pages
> > to be registered as shared memory with OP-TEE.
> >
> > Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
>
> Just out of pure interest why this was not allowed before?
Please spare me and ignore that one :-) Obviouslly because it
was not used.
Acked-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
/Jarkko
^ permalink raw reply
* Re: [RFC 1/7] tee: optee: allow kernel pages to register as shm
From: Jarkko Sakkinen @ 2019-06-13 15:17 UTC (permalink / raw)
To: Sumit Garg
Cc: keyrings, linux-integrity, linux-security-module, jens.wiklander,
corbet, dhowells, jejb, zohar, jmorris, serge, ard.biesheuvel,
daniel.thompson, linux-doc, linux-kernel, tee-dev
In-Reply-To: <20190613151714.GC18488@linux.intel.com>
On Thu, Jun 13, 2019 at 06:17:14PM +0300, Jarkko Sakkinen wrote:
> On Thu, Jun 13, 2019 at 06:12:57PM +0300, Jarkko Sakkinen wrote:
> > On Thu, Jun 13, 2019 at 04:00:27PM +0530, Sumit Garg wrote:
> > > Kernel pages are marked as normal type memory only so allow kernel pages
> > > to be registered as shared memory with OP-TEE.
> > >
> > > Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
> >
> > Just out of pure interest why this was not allowed before?
>
> Please spare me and ignore that one :-) Obviouslly because it
> was not used.
>
> Acked-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Actually,
Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
/Jarkko
^ permalink raw reply
* Re: [PATCH v4 1/3] lib/test_kasan: Add bitops tests
From: Marco Elver @ 2019-06-13 13:00 UTC (permalink / raw)
To: Andrey Ryabinin
Cc: Peter Zijlstra, Dmitry Vyukov, Alexander Potapenko,
Andrey Konovalov, Mark Rutland, H. Peter Anvin, Jonathan Corbet,
Thomas Gleixner, Ingo Molnar, Borislav Petkov,
the arch/x86 maintainers, Arnd Bergmann, Josh Poimboeuf,
open list:DOCUMENTATION, LKML, linux-arch, kasan-dev
In-Reply-To: <6cc5e12d-1492-d9b7-3ea7-6381407439d7@virtuozzo.com>
On Thu, 13 Jun 2019 at 14:49, Andrey Ryabinin <aryabinin@virtuozzo.com> wrote:
>
>
>
> On 6/13/19 3:30 PM, Marco Elver wrote:
> > This adds bitops tests to the test_kasan module. In a follow-up patch,
> > support for bitops instrumentation will be added.
> >
> > Signed-off-by: Marco Elver <elver@google.com>
> > Acked-by: Mark Rutland <mark.rutland@arm.com>
> > ---
>
> Reviewed-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
>
>
>
>
> > +static noinline void __init kasan_bitops(void)
> > +{
> > + /*
> > + * Allocate 1 more byte, which causes kzalloc to round up to 16-bytes;
> > + * this way we do not actually corrupt other memory, in case
> > + * instrumentation is not working as intended.
>
> This sound like working instrumentation somehow save us from corrupting memory. In fact it doesn't,
> it only reports corruption.
Thanks, I removed the confusing wording. Sent v5.
> > + */
> > + long *bits = kzalloc(sizeof(*bits) + 1, GFP_KERNEL);
> > + if (!bits)
> > + return;
> > +
>
> --
> You received this message because you are subscribed to the Google Groups "kasan-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to kasan-dev+unsubscribe@googlegroups.com.
> To post to this group, send email to kasan-dev@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/kasan-dev/6cc5e12d-1492-d9b7-3ea7-6381407439d7%40virtuozzo.com.
> For more options, visit https://groups.google.com/d/optout.
^ permalink raw reply
* [PATCH v5 3/3] asm-generic, x86: Add bitops instrumentation for KASAN
From: Marco Elver @ 2019-06-13 12:59 UTC (permalink / raw)
To: peterz, aryabinin, dvyukov, glider, andreyknvl, mark.rutland, hpa
Cc: corbet, tglx, mingo, bp, x86, arnd, jpoimboe, linux-doc,
linux-kernel, linux-arch, kasan-dev, Marco Elver
In-Reply-To: <20190613125950.197667-1-elver@google.com>
This adds a new header to asm-generic to allow optionally instrumenting
architecture-specific asm implementations of bitops.
This change includes the required change for x86 as reference and
changes the kernel API doc to point to bitops-instrumented.h instead.
Rationale: the functions in x86's bitops.h are no longer the kernel API
functions, but instead the arch_ prefixed functions, which are then
instrumented via bitops-instrumented.h.
Other architectures can similarly add support for asm implementations of
bitops.
The documentation text was derived from x86 and existing bitops
asm-generic versions: 1) references to x86 have been removed; 2) as a
result, some of the text had to be reworded for clarity and consistency.
Tested: using lib/test_kasan with bitops tests (pre-requisite patch).
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=198439
Signed-off-by: Marco Elver <elver@google.com>
Acked-by: Mark Rutland <mark.rutland@arm.com>
Reviewed-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
---
Changes in v3:
* Remove references to 'x86' in API documentation; as a result, had to
reword doc text for clarify and consistency.
* Remove #ifdef, since it is assumed that if asm-generic bitops
implementations are used, bitops-instrumented.h is not needed.
Changes in v2:
* Instrument word-sized accesses, as specified by the interface.
---
Documentation/core-api/kernel-api.rst | 2 +-
arch/x86/include/asm/bitops.h | 189 ++++------------
include/asm-generic/bitops-instrumented.h | 263 ++++++++++++++++++++++
3 files changed, 302 insertions(+), 152 deletions(-)
create mode 100644 include/asm-generic/bitops-instrumented.h
diff --git a/Documentation/core-api/kernel-api.rst b/Documentation/core-api/kernel-api.rst
index a29c99d13331..65266fa1b706 100644
--- a/Documentation/core-api/kernel-api.rst
+++ b/Documentation/core-api/kernel-api.rst
@@ -51,7 +51,7 @@ The Linux kernel provides more basic utility functions.
Bit Operations
--------------
-.. kernel-doc:: arch/x86/include/asm/bitops.h
+.. kernel-doc:: include/asm-generic/bitops-instrumented.h
:internal:
Bitmap Operations
diff --git a/arch/x86/include/asm/bitops.h b/arch/x86/include/asm/bitops.h
index 8e790ec219a5..ba15d53c1ca7 100644
--- a/arch/x86/include/asm/bitops.h
+++ b/arch/x86/include/asm/bitops.h
@@ -49,23 +49,8 @@
#define CONST_MASK_ADDR(nr, addr) WBYTE_ADDR((void *)(addr) + ((nr)>>3))
#define CONST_MASK(nr) (1 << ((nr) & 7))
-/**
- * set_bit - Atomically set a bit in memory
- * @nr: the bit to set
- * @addr: the address to start counting from
- *
- * This function is atomic and may not be reordered. See __set_bit()
- * if you do not require the atomic guarantees.
- *
- * Note: there are no guarantees that this function will not be reordered
- * on non x86 architectures, so if you are writing portable code,
- * make sure not to rely on its reordering guarantees.
- *
- * Note that @nr may be almost arbitrarily large; this function is not
- * restricted to acting on a single-word quantity.
- */
static __always_inline void
-set_bit(long nr, volatile unsigned long *addr)
+arch_set_bit(long nr, volatile unsigned long *addr)
{
if (IS_IMMEDIATE(nr)) {
asm volatile(LOCK_PREFIX "orb %1,%0"
@@ -78,32 +63,14 @@ set_bit(long nr, volatile unsigned long *addr)
}
}
-/**
- * __set_bit - Set a bit in memory
- * @nr: the bit to set
- * @addr: the address to start counting from
- *
- * Unlike set_bit(), this function is non-atomic and may be reordered.
- * If it's called on the same region of memory simultaneously, the effect
- * may be that only one operation succeeds.
- */
-static __always_inline void __set_bit(long nr, volatile unsigned long *addr)
+static __always_inline void
+arch___set_bit(long nr, volatile unsigned long *addr)
{
asm volatile(__ASM_SIZE(bts) " %1,%0" : : ADDR, "Ir" (nr) : "memory");
}
-/**
- * clear_bit - Clears a bit in memory
- * @nr: Bit to clear
- * @addr: Address to start counting from
- *
- * clear_bit() is atomic and may not be reordered. However, it does
- * not contain a memory barrier, so if it is used for locking purposes,
- * you should call smp_mb__before_atomic() and/or smp_mb__after_atomic()
- * in order to ensure changes are visible on other processors.
- */
static __always_inline void
-clear_bit(long nr, volatile unsigned long *addr)
+arch_clear_bit(long nr, volatile unsigned long *addr)
{
if (IS_IMMEDIATE(nr)) {
asm volatile(LOCK_PREFIX "andb %1,%0"
@@ -115,26 +82,21 @@ clear_bit(long nr, volatile unsigned long *addr)
}
}
-/*
- * clear_bit_unlock - Clears a bit in memory
- * @nr: Bit to clear
- * @addr: Address to start counting from
- *
- * clear_bit() is atomic and implies release semantics before the memory
- * operation. It can be used for an unlock.
- */
-static __always_inline void clear_bit_unlock(long nr, volatile unsigned long *addr)
+static __always_inline void
+arch_clear_bit_unlock(long nr, volatile unsigned long *addr)
{
barrier();
- clear_bit(nr, addr);
+ arch_clear_bit(nr, addr);
}
-static __always_inline void __clear_bit(long nr, volatile unsigned long *addr)
+static __always_inline void
+arch___clear_bit(long nr, volatile unsigned long *addr)
{
asm volatile(__ASM_SIZE(btr) " %1,%0" : : ADDR, "Ir" (nr) : "memory");
}
-static __always_inline bool clear_bit_unlock_is_negative_byte(long nr, volatile unsigned long *addr)
+static __always_inline bool
+arch_clear_bit_unlock_is_negative_byte(long nr, volatile unsigned long *addr)
{
bool negative;
asm volatile(LOCK_PREFIX "andb %2,%1"
@@ -143,48 +105,23 @@ static __always_inline bool clear_bit_unlock_is_negative_byte(long nr, volatile
: "ir" ((char) ~(1 << nr)) : "memory");
return negative;
}
+#define arch_clear_bit_unlock_is_negative_byte \
+ arch_clear_bit_unlock_is_negative_byte
-// Let everybody know we have it
-#define clear_bit_unlock_is_negative_byte clear_bit_unlock_is_negative_byte
-
-/*
- * __clear_bit_unlock - Clears a bit in memory
- * @nr: Bit to clear
- * @addr: Address to start counting from
- *
- * __clear_bit() is non-atomic and implies release semantics before the memory
- * operation. It can be used for an unlock if no other CPUs can concurrently
- * modify other bits in the word.
- */
-static __always_inline void __clear_bit_unlock(long nr, volatile unsigned long *addr)
+static __always_inline void
+arch___clear_bit_unlock(long nr, volatile unsigned long *addr)
{
- __clear_bit(nr, addr);
+ arch___clear_bit(nr, addr);
}
-/**
- * __change_bit - Toggle a bit in memory
- * @nr: the bit to change
- * @addr: the address to start counting from
- *
- * Unlike change_bit(), this function is non-atomic and may be reordered.
- * If it's called on the same region of memory simultaneously, the effect
- * may be that only one operation succeeds.
- */
-static __always_inline void __change_bit(long nr, volatile unsigned long *addr)
+static __always_inline void
+arch___change_bit(long nr, volatile unsigned long *addr)
{
asm volatile(__ASM_SIZE(btc) " %1,%0" : : ADDR, "Ir" (nr) : "memory");
}
-/**
- * change_bit - Toggle a bit in memory
- * @nr: Bit to change
- * @addr: Address to start counting from
- *
- * change_bit() is atomic and may not be reordered.
- * Note that @nr may be almost arbitrarily large; this function is not
- * restricted to acting on a single-word quantity.
- */
-static __always_inline void change_bit(long nr, volatile unsigned long *addr)
+static __always_inline void
+arch_change_bit(long nr, volatile unsigned long *addr)
{
if (IS_IMMEDIATE(nr)) {
asm volatile(LOCK_PREFIX "xorb %1,%0"
@@ -196,42 +133,20 @@ static __always_inline void change_bit(long nr, volatile unsigned long *addr)
}
}
-/**
- * test_and_set_bit - Set a bit and return its old value
- * @nr: Bit to set
- * @addr: Address to count from
- *
- * This operation is atomic and cannot be reordered.
- * It also implies a memory barrier.
- */
-static __always_inline bool test_and_set_bit(long nr, volatile unsigned long *addr)
+static __always_inline bool
+arch_test_and_set_bit(long nr, volatile unsigned long *addr)
{
return GEN_BINARY_RMWcc(LOCK_PREFIX __ASM_SIZE(bts), *addr, c, "Ir", nr);
}
-/**
- * test_and_set_bit_lock - Set a bit and return its old value for lock
- * @nr: Bit to set
- * @addr: Address to count from
- *
- * This is the same as test_and_set_bit on x86.
- */
static __always_inline bool
-test_and_set_bit_lock(long nr, volatile unsigned long *addr)
+arch_test_and_set_bit_lock(long nr, volatile unsigned long *addr)
{
- return test_and_set_bit(nr, addr);
+ return arch_test_and_set_bit(nr, addr);
}
-/**
- * __test_and_set_bit - Set a bit and return its old value
- * @nr: Bit to set
- * @addr: Address to count from
- *
- * This operation is non-atomic and can be reordered.
- * If two examples of this operation race, one can appear to succeed
- * but actually fail. You must protect multiple accesses with a lock.
- */
-static __always_inline bool __test_and_set_bit(long nr, volatile unsigned long *addr)
+static __always_inline bool
+arch___test_and_set_bit(long nr, volatile unsigned long *addr)
{
bool oldbit;
@@ -242,28 +157,13 @@ static __always_inline bool __test_and_set_bit(long nr, volatile unsigned long *
return oldbit;
}
-/**
- * test_and_clear_bit - Clear a bit and return its old value
- * @nr: Bit to clear
- * @addr: Address to count from
- *
- * This operation is atomic and cannot be reordered.
- * It also implies a memory barrier.
- */
-static __always_inline bool test_and_clear_bit(long nr, volatile unsigned long *addr)
+static __always_inline bool
+arch_test_and_clear_bit(long nr, volatile unsigned long *addr)
{
return GEN_BINARY_RMWcc(LOCK_PREFIX __ASM_SIZE(btr), *addr, c, "Ir", nr);
}
-/**
- * __test_and_clear_bit - Clear a bit and return its old value
- * @nr: Bit to clear
- * @addr: Address to count from
- *
- * This operation is non-atomic and can be reordered.
- * If two examples of this operation race, one can appear to succeed
- * but actually fail. You must protect multiple accesses with a lock.
- *
+/*
* Note: the operation is performed atomically with respect to
* the local CPU, but not other CPUs. Portable code should not
* rely on this behaviour.
@@ -271,7 +171,8 @@ static __always_inline bool test_and_clear_bit(long nr, volatile unsigned long *
* accessed from a hypervisor on the same CPU if running in a VM: don't change
* this without also updating arch/x86/kernel/kvm.c
*/
-static __always_inline bool __test_and_clear_bit(long nr, volatile unsigned long *addr)
+static __always_inline bool
+arch___test_and_clear_bit(long nr, volatile unsigned long *addr)
{
bool oldbit;
@@ -282,8 +183,8 @@ static __always_inline bool __test_and_clear_bit(long nr, volatile unsigned long
return oldbit;
}
-/* WARNING: non atomic and it can be reordered! */
-static __always_inline bool __test_and_change_bit(long nr, volatile unsigned long *addr)
+static __always_inline bool
+arch___test_and_change_bit(long nr, volatile unsigned long *addr)
{
bool oldbit;
@@ -295,15 +196,8 @@ static __always_inline bool __test_and_change_bit(long nr, volatile unsigned lon
return oldbit;
}
-/**
- * test_and_change_bit - Change a bit and return its old value
- * @nr: Bit to change
- * @addr: Address to count from
- *
- * This operation is atomic and cannot be reordered.
- * It also implies a memory barrier.
- */
-static __always_inline bool test_and_change_bit(long nr, volatile unsigned long *addr)
+static __always_inline bool
+arch_test_and_change_bit(long nr, volatile unsigned long *addr)
{
return GEN_BINARY_RMWcc(LOCK_PREFIX __ASM_SIZE(btc), *addr, c, "Ir", nr);
}
@@ -326,16 +220,7 @@ static __always_inline bool variable_test_bit(long nr, volatile const unsigned l
return oldbit;
}
-#if 0 /* Fool kernel-doc since it doesn't do macros yet */
-/**
- * test_bit - Determine whether a bit is set
- * @nr: bit number to test
- * @addr: Address to start counting from
- */
-static bool test_bit(int nr, const volatile unsigned long *addr);
-#endif
-
-#define test_bit(nr, addr) \
+#define arch_test_bit(nr, addr) \
(__builtin_constant_p((nr)) \
? constant_test_bit((nr), (addr)) \
: variable_test_bit((nr), (addr)))
@@ -504,6 +389,8 @@ static __always_inline int fls64(__u64 x)
#include <asm-generic/bitops/const_hweight.h>
+#include <asm-generic/bitops-instrumented.h>
+
#include <asm-generic/bitops/le.h>
#include <asm-generic/bitops/ext2-atomic-setbit.h>
diff --git a/include/asm-generic/bitops-instrumented.h b/include/asm-generic/bitops-instrumented.h
new file mode 100644
index 000000000000..ddd1c6d9d8db
--- /dev/null
+++ b/include/asm-generic/bitops-instrumented.h
@@ -0,0 +1,263 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+/*
+ * This file provides wrappers with sanitizer instrumentation for bit
+ * operations.
+ *
+ * To use this functionality, an arch's bitops.h file needs to define each of
+ * the below bit operations with an arch_ prefix (e.g. arch_set_bit(),
+ * arch___set_bit(), etc.).
+ */
+#ifndef _ASM_GENERIC_BITOPS_INSTRUMENTED_H
+#define _ASM_GENERIC_BITOPS_INSTRUMENTED_H
+
+#include <linux/kasan-checks.h>
+
+/**
+ * set_bit - Atomically set a bit in memory
+ * @nr: the bit to set
+ * @addr: the address to start counting from
+ *
+ * This is a relaxed atomic operation (no implied memory barriers).
+ *
+ * Note that @nr may be almost arbitrarily large; this function is not
+ * restricted to acting on a single-word quantity.
+ */
+static inline void set_bit(long nr, volatile unsigned long *addr)
+{
+ kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
+ arch_set_bit(nr, addr);
+}
+
+/**
+ * __set_bit - Set a bit in memory
+ * @nr: the bit to set
+ * @addr: the address to start counting from
+ *
+ * Unlike set_bit(), this function is non-atomic. If it is called on the same
+ * region of memory concurrently, the effect may be that only one operation
+ * succeeds.
+ */
+static inline void __set_bit(long nr, volatile unsigned long *addr)
+{
+ kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
+ arch___set_bit(nr, addr);
+}
+
+/**
+ * clear_bit - Clears a bit in memory
+ * @nr: Bit to clear
+ * @addr: Address to start counting from
+ *
+ * This is a relaxed atomic operation (no implied memory barriers).
+ */
+static inline void clear_bit(long nr, volatile unsigned long *addr)
+{
+ kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
+ arch_clear_bit(nr, addr);
+}
+
+/**
+ * __clear_bit - Clears a bit in memory
+ * @nr: the bit to clear
+ * @addr: the address to start counting from
+ *
+ * Unlike clear_bit(), this function is non-atomic. If it is called on the same
+ * region of memory concurrently, the effect may be that only one operation
+ * succeeds.
+ */
+static inline void __clear_bit(long nr, volatile unsigned long *addr)
+{
+ kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
+ arch___clear_bit(nr, addr);
+}
+
+/**
+ * clear_bit_unlock - Clear a bit in memory, for unlock
+ * @nr: the bit to set
+ * @addr: the address to start counting from
+ *
+ * This operation is atomic and provides release barrier semantics.
+ */
+static inline void clear_bit_unlock(long nr, volatile unsigned long *addr)
+{
+ kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
+ arch_clear_bit_unlock(nr, addr);
+}
+
+/**
+ * __clear_bit_unlock - Clears a bit in memory
+ * @nr: Bit to clear
+ * @addr: Address to start counting from
+ *
+ * This is a non-atomic operation but implies a release barrier before the
+ * memory operation. It can be used for an unlock if no other CPUs can
+ * concurrently modify other bits in the word.
+ */
+static inline void __clear_bit_unlock(long nr, volatile unsigned long *addr)
+{
+ kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
+ arch___clear_bit_unlock(nr, addr);
+}
+
+/**
+ * change_bit - Toggle a bit in memory
+ * @nr: Bit to change
+ * @addr: Address to start counting from
+ *
+ * This is a relaxed atomic operation (no implied memory barriers).
+ *
+ * Note that @nr may be almost arbitrarily large; this function is not
+ * restricted to acting on a single-word quantity.
+ */
+static inline void change_bit(long nr, volatile unsigned long *addr)
+{
+ kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
+ arch_change_bit(nr, addr);
+}
+
+/**
+ * __change_bit - Toggle a bit in memory
+ * @nr: the bit to change
+ * @addr: the address to start counting from
+ *
+ * Unlike change_bit(), this function is non-atomic. If it is called on the same
+ * region of memory concurrently, the effect may be that only one operation
+ * succeeds.
+ */
+static inline void __change_bit(long nr, volatile unsigned long *addr)
+{
+ kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
+ arch___change_bit(nr, addr);
+}
+
+/**
+ * test_and_set_bit - Set a bit and return its old value
+ * @nr: Bit to set
+ * @addr: Address to count from
+ *
+ * This is an atomic fully-ordered operation (implied full memory barrier).
+ */
+static inline bool test_and_set_bit(long nr, volatile unsigned long *addr)
+{
+ kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
+ return arch_test_and_set_bit(nr, addr);
+}
+
+/**
+ * __test_and_set_bit - Set a bit and return its old value
+ * @nr: Bit to set
+ * @addr: Address to count from
+ *
+ * This operation is non-atomic. If two instances of this operation race, one
+ * can appear to succeed but actually fail.
+ */
+static inline bool __test_and_set_bit(long nr, volatile unsigned long *addr)
+{
+ kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
+ return arch___test_and_set_bit(nr, addr);
+}
+
+/**
+ * test_and_set_bit_lock - Set a bit and return its old value, for lock
+ * @nr: Bit to set
+ * @addr: Address to count from
+ *
+ * This operation is atomic and provides acquire barrier semantics if
+ * the returned value is 0.
+ * It can be used to implement bit locks.
+ */
+static inline bool test_and_set_bit_lock(long nr, volatile unsigned long *addr)
+{
+ kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
+ return arch_test_and_set_bit_lock(nr, addr);
+}
+
+/**
+ * test_and_clear_bit - Clear a bit and return its old value
+ * @nr: Bit to clear
+ * @addr: Address to count from
+ *
+ * This is an atomic fully-ordered operation (implied full memory barrier).
+ */
+static inline bool test_and_clear_bit(long nr, volatile unsigned long *addr)
+{
+ kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
+ return arch_test_and_clear_bit(nr, addr);
+}
+
+/**
+ * __test_and_clear_bit - Clear a bit and return its old value
+ * @nr: Bit to clear
+ * @addr: Address to count from
+ *
+ * This operation is non-atomic. If two instances of this operation race, one
+ * can appear to succeed but actually fail.
+ */
+static inline bool __test_and_clear_bit(long nr, volatile unsigned long *addr)
+{
+ kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
+ return arch___test_and_clear_bit(nr, addr);
+}
+
+/**
+ * test_and_change_bit - Change a bit and return its old value
+ * @nr: Bit to change
+ * @addr: Address to count from
+ *
+ * This is an atomic fully-ordered operation (implied full memory barrier).
+ */
+static inline bool test_and_change_bit(long nr, volatile unsigned long *addr)
+{
+ kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
+ return arch_test_and_change_bit(nr, addr);
+}
+
+/**
+ * __test_and_change_bit - Change a bit and return its old value
+ * @nr: Bit to change
+ * @addr: Address to count from
+ *
+ * This operation is non-atomic. If two instances of this operation race, one
+ * can appear to succeed but actually fail.
+ */
+static inline bool __test_and_change_bit(long nr, volatile unsigned long *addr)
+{
+ kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
+ return arch___test_and_change_bit(nr, addr);
+}
+
+/**
+ * test_bit - Determine whether a bit is set
+ * @nr: bit number to test
+ * @addr: Address to start counting from
+ */
+static inline bool test_bit(long nr, const volatile unsigned long *addr)
+{
+ kasan_check_read(addr + BIT_WORD(nr), sizeof(long));
+ return arch_test_bit(nr, addr);
+}
+
+#if defined(arch_clear_bit_unlock_is_negative_byte)
+/**
+ * clear_bit_unlock_is_negative_byte - Clear a bit in memory and test if bottom
+ * byte is negative, for unlock.
+ * @nr: the bit to clear
+ * @addr: the address to start counting from
+ *
+ * This operation is atomic and provides release barrier semantics.
+ *
+ * This is a bit of a one-trick-pony for the filemap code, which clears
+ * PG_locked and tests PG_waiters,
+ */
+static inline bool
+clear_bit_unlock_is_negative_byte(long nr, volatile unsigned long *addr)
+{
+ kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
+ return arch_clear_bit_unlock_is_negative_byte(nr, addr);
+}
+/* Let everybody know we have it. */
+#define clear_bit_unlock_is_negative_byte clear_bit_unlock_is_negative_byte
+#endif
+
+#endif /* _ASM_GENERIC_BITOPS_INSTRUMENTED_H */
--
2.22.0.rc2.383.gf4fbbf30c2-goog
^ permalink raw reply related
* [PATCH v5 2/3] x86: Use static_cpu_has in uaccess region to avoid instrumentation
From: Marco Elver @ 2019-06-13 12:59 UTC (permalink / raw)
To: peterz, aryabinin, dvyukov, glider, andreyknvl, mark.rutland, hpa
Cc: corbet, tglx, mingo, bp, x86, arnd, jpoimboe, linux-doc,
linux-kernel, linux-arch, kasan-dev, Marco Elver
In-Reply-To: <20190613125950.197667-1-elver@google.com>
This patch is a pre-requisite for enabling KASAN bitops instrumentation;
using static_cpu_has instead of boot_cpu_has avoids instrumentation of
test_bit inside the uaccess region. With instrumentation, the KASAN
check would otherwise be flagged by objtool.
For consistency, kernel/signal.c was changed to mirror this change,
however, is never instrumented with KASAN (currently unsupported under
x86 32bit).
Signed-off-by: Marco Elver <elver@google.com>
Suggested-by: H. Peter Anvin <hpa@zytor.com>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
---
Changes in v3:
* Use static_cpu_has instead of moving boot_cpu_has outside uaccess
region.
Changes in v2:
* Replaces patch: 'tools/objtool: add kasan_check_* to uaccess
whitelist'
---
arch/x86/ia32/ia32_signal.c | 2 +-
arch/x86/kernel/signal.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/x86/ia32/ia32_signal.c b/arch/x86/ia32/ia32_signal.c
index 629d1ee05599..1cee10091b9f 100644
--- a/arch/x86/ia32/ia32_signal.c
+++ b/arch/x86/ia32/ia32_signal.c
@@ -358,7 +358,7 @@ int ia32_setup_rt_frame(int sig, struct ksignal *ksig,
put_user_ex(ptr_to_compat(&frame->uc), &frame->puc);
/* Create the ucontext. */
- if (boot_cpu_has(X86_FEATURE_XSAVE))
+ if (static_cpu_has(X86_FEATURE_XSAVE))
put_user_ex(UC_FP_XSTATE, &frame->uc.uc_flags);
else
put_user_ex(0, &frame->uc.uc_flags);
diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
index 364813cea647..52eb1d551aed 100644
--- a/arch/x86/kernel/signal.c
+++ b/arch/x86/kernel/signal.c
@@ -391,7 +391,7 @@ static int __setup_rt_frame(int sig, struct ksignal *ksig,
put_user_ex(&frame->uc, &frame->puc);
/* Create the ucontext. */
- if (boot_cpu_has(X86_FEATURE_XSAVE))
+ if (static_cpu_has(X86_FEATURE_XSAVE))
put_user_ex(UC_FP_XSTATE, &frame->uc.uc_flags);
else
put_user_ex(0, &frame->uc.uc_flags);
--
2.22.0.rc2.383.gf4fbbf30c2-goog
^ permalink raw reply related
* [PATCH v5 1/3] lib/test_kasan: Add bitops tests
From: Marco Elver @ 2019-06-13 12:59 UTC (permalink / raw)
To: peterz, aryabinin, dvyukov, glider, andreyknvl, mark.rutland, hpa
Cc: corbet, tglx, mingo, bp, x86, arnd, jpoimboe, linux-doc,
linux-kernel, linux-arch, kasan-dev, Marco Elver
In-Reply-To: <20190613125950.197667-1-elver@google.com>
This adds bitops tests to the test_kasan module. In a follow-up patch,
support for bitops instrumentation will be added.
Signed-off-by: Marco Elver <elver@google.com>
Acked-by: Mark Rutland <mark.rutland@arm.com>
Reviewed-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
---
Changes in v5:
* Remove incorrect comment.
Changes in v4:
* Remove "within-bounds" tests.
* Allocate sizeof(*bite) + 1, to not actually corrupt other memory in
case instrumentation isn't working.
* Clarify that accesses operate on whole longs, which causes OOB
regardless of the bit accessed beyond the first long in the test.
Changes in v3:
* Use kzalloc instead of kmalloc.
* Use sizeof(*bits).
Changes in v2:
* Use BITS_PER_LONG.
* Use heap allocated memory for test, as newer compilers (correctly)
warn on OOB stack access.
---
lib/test_kasan.c | 81 ++++++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 78 insertions(+), 3 deletions(-)
diff --git a/lib/test_kasan.c b/lib/test_kasan.c
index 7de2702621dc..267f31a61870 100644
--- a/lib/test_kasan.c
+++ b/lib/test_kasan.c
@@ -11,16 +11,17 @@
#define pr_fmt(fmt) "kasan test: %s " fmt, __func__
+#include <linux/bitops.h>
#include <linux/delay.h>
+#include <linux/kasan.h>
#include <linux/kernel.h>
-#include <linux/mman.h>
#include <linux/mm.h>
+#include <linux/mman.h>
+#include <linux/module.h>
#include <linux/printk.h>
#include <linux/slab.h>
#include <linux/string.h>
#include <linux/uaccess.h>
-#include <linux/module.h>
-#include <linux/kasan.h>
/*
* Note: test functions are marked noinline so that their names appear in
@@ -623,6 +624,79 @@ static noinline void __init kasan_strings(void)
strnlen(ptr, 1);
}
+static noinline void __init kasan_bitops(void)
+{
+ /*
+ * Allocate 1 more byte, which causes kzalloc to round up to 16-bytes;
+ * this way we do not actually corrupt other memory.
+ */
+ long *bits = kzalloc(sizeof(*bits) + 1, GFP_KERNEL);
+ if (!bits)
+ return;
+
+ /*
+ * Below calls try to access bit within allocated memory; however, the
+ * below accesses are still out-of-bounds, since bitops are defined to
+ * operate on the whole long the bit is in.
+ */
+ pr_info("out-of-bounds in set_bit\n");
+ set_bit(BITS_PER_LONG, bits);
+
+ pr_info("out-of-bounds in __set_bit\n");
+ __set_bit(BITS_PER_LONG, bits);
+
+ pr_info("out-of-bounds in clear_bit\n");
+ clear_bit(BITS_PER_LONG, bits);
+
+ pr_info("out-of-bounds in __clear_bit\n");
+ __clear_bit(BITS_PER_LONG, bits);
+
+ pr_info("out-of-bounds in clear_bit_unlock\n");
+ clear_bit_unlock(BITS_PER_LONG, bits);
+
+ pr_info("out-of-bounds in __clear_bit_unlock\n");
+ __clear_bit_unlock(BITS_PER_LONG, bits);
+
+ pr_info("out-of-bounds in change_bit\n");
+ change_bit(BITS_PER_LONG, bits);
+
+ pr_info("out-of-bounds in __change_bit\n");
+ __change_bit(BITS_PER_LONG, bits);
+
+ /*
+ * Below calls try to access bit beyond allocated memory.
+ */
+ pr_info("out-of-bounds in test_and_set_bit\n");
+ test_and_set_bit(BITS_PER_LONG + BITS_PER_BYTE, bits);
+
+ pr_info("out-of-bounds in __test_and_set_bit\n");
+ __test_and_set_bit(BITS_PER_LONG + BITS_PER_BYTE, bits);
+
+ pr_info("out-of-bounds in test_and_set_bit_lock\n");
+ test_and_set_bit_lock(BITS_PER_LONG + BITS_PER_BYTE, bits);
+
+ pr_info("out-of-bounds in test_and_clear_bit\n");
+ test_and_clear_bit(BITS_PER_LONG + BITS_PER_BYTE, bits);
+
+ pr_info("out-of-bounds in __test_and_clear_bit\n");
+ __test_and_clear_bit(BITS_PER_LONG + BITS_PER_BYTE, bits);
+
+ pr_info("out-of-bounds in test_and_change_bit\n");
+ test_and_change_bit(BITS_PER_LONG + BITS_PER_BYTE, bits);
+
+ pr_info("out-of-bounds in __test_and_change_bit\n");
+ __test_and_change_bit(BITS_PER_LONG + BITS_PER_BYTE, bits);
+
+ pr_info("out-of-bounds in test_bit\n");
+ (void)test_bit(BITS_PER_LONG + BITS_PER_BYTE, bits);
+
+#if defined(clear_bit_unlock_is_negative_byte)
+ pr_info("out-of-bounds in clear_bit_unlock_is_negative_byte\n");
+ clear_bit_unlock_is_negative_byte(BITS_PER_LONG + BITS_PER_BYTE, bits);
+#endif
+ kfree(bits);
+}
+
static int __init kmalloc_tests_init(void)
{
/*
@@ -664,6 +738,7 @@ static int __init kmalloc_tests_init(void)
kasan_memchr();
kasan_memcmp();
kasan_strings();
+ kasan_bitops();
kasan_restore_multi_shot(multishot);
--
2.22.0.rc2.383.gf4fbbf30c2-goog
^ permalink raw reply related
* [PATCH v5 0/3] Bitops instrumentation for KASAN
From: Marco Elver @ 2019-06-13 12:59 UTC (permalink / raw)
To: peterz, aryabinin, dvyukov, glider, andreyknvl, mark.rutland, hpa
Cc: corbet, tglx, mingo, bp, x86, arnd, jpoimboe, linux-doc,
linux-kernel, linux-arch, kasan-dev, Marco Elver
Previous version:
http://lkml.kernel.org/r/20190613123028.179447-1-elver@google.com
* Only changed lib/test_kasan in this version.
Marco Elver (3):
lib/test_kasan: Add bitops tests
x86: Use static_cpu_has in uaccess region to avoid instrumentation
asm-generic, x86: Add bitops instrumentation for KASAN
Documentation/core-api/kernel-api.rst | 2 +-
arch/x86/ia32/ia32_signal.c | 2 +-
arch/x86/include/asm/bitops.h | 189 ++++------------
arch/x86/kernel/signal.c | 2 +-
include/asm-generic/bitops-instrumented.h | 263 ++++++++++++++++++++++
lib/test_kasan.c | 81 ++++++-
6 files changed, 382 insertions(+), 157 deletions(-)
create mode 100644 include/asm-generic/bitops-instrumented.h
--
2.22.0.rc2.383.gf4fbbf30c2-goog
^ permalink raw reply
* Re: [RFC 2/7] tee: enable support to register kernel memory
From: Jarkko Sakkinen @ 2019-06-13 15:20 UTC (permalink / raw)
To: Sumit Garg
Cc: keyrings, linux-integrity, linux-security-module, jens.wiklander,
corbet, dhowells, jejb, zohar, jmorris, serge, ard.biesheuvel,
daniel.thompson, linux-doc, linux-kernel, tee-dev
In-Reply-To: <1560421833-27414-3-git-send-email-sumit.garg@linaro.org>
On Thu, Jun 13, 2019 at 04:00:28PM +0530, Sumit Garg wrote:
> Enable support to register kernel memory reference with TEE. This change
> will allow TEE bus drivers to register memory references.
>
> Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
/Jarkko
^ permalink raw reply
* Re: [PATCH v4 1/3] lib/test_kasan: Add bitops tests
From: Andrey Ryabinin @ 2019-06-13 12:50 UTC (permalink / raw)
To: Marco Elver, peterz, dvyukov, glider, andreyknvl, mark.rutland,
hpa
Cc: corbet, tglx, mingo, bp, x86, arnd, jpoimboe, linux-doc,
linux-kernel, linux-arch, kasan-dev
In-Reply-To: <20190613123028.179447-2-elver@google.com>
On 6/13/19 3:30 PM, Marco Elver wrote:
> This adds bitops tests to the test_kasan module. In a follow-up patch,
> support for bitops instrumentation will be added.
>
> Signed-off-by: Marco Elver <elver@google.com>
> Acked-by: Mark Rutland <mark.rutland@arm.com>
> ---
Reviewed-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
> +static noinline void __init kasan_bitops(void)
> +{
> + /*
> + * Allocate 1 more byte, which causes kzalloc to round up to 16-bytes;
> + * this way we do not actually corrupt other memory, in case
> + * instrumentation is not working as intended.
This sound like working instrumentation somehow save us from corrupting memory. In fact it doesn't,
it only reports corruption.
> + */
> + long *bits = kzalloc(sizeof(*bits) + 1, GFP_KERNEL);
> + if (!bits)
> + return;
> +
^ permalink raw reply
* [PATCH v4 3/3] asm-generic, x86: Add bitops instrumentation for KASAN
From: Marco Elver @ 2019-06-13 12:30 UTC (permalink / raw)
To: peterz, aryabinin, dvyukov, glider, andreyknvl, mark.rutland, hpa
Cc: corbet, tglx, mingo, bp, x86, arnd, jpoimboe, linux-doc,
linux-kernel, linux-arch, kasan-dev, Marco Elver
In-Reply-To: <20190613123028.179447-1-elver@google.com>
This adds a new header to asm-generic to allow optionally instrumenting
architecture-specific asm implementations of bitops.
This change includes the required change for x86 as reference and
changes the kernel API doc to point to bitops-instrumented.h instead.
Rationale: the functions in x86's bitops.h are no longer the kernel API
functions, but instead the arch_ prefixed functions, which are then
instrumented via bitops-instrumented.h.
Other architectures can similarly add support for asm implementations of
bitops.
The documentation text was derived from x86 and existing bitops
asm-generic versions: 1) references to x86 have been removed; 2) as a
result, some of the text had to be reworded for clarity and consistency.
Tested: using lib/test_kasan with bitops tests (pre-requisite patch).
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=198439
Signed-off-by: Marco Elver <elver@google.com>
Acked-by: Mark Rutland <mark.rutland@arm.com>
Reviewed-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
---
Changes in v3:
* Remove references to 'x86' in API documentation; as a result, had to
reword doc text for clarify and consistency.
* Remove #ifdef, since it is assumed that if asm-generic bitops
implementations are used, bitops-instrumented.h is not needed.
Changes in v2:
* Instrument word-sized accesses, as specified by the interface.
---
Documentation/core-api/kernel-api.rst | 2 +-
arch/x86/include/asm/bitops.h | 189 ++++------------
include/asm-generic/bitops-instrumented.h | 263 ++++++++++++++++++++++
3 files changed, 302 insertions(+), 152 deletions(-)
create mode 100644 include/asm-generic/bitops-instrumented.h
diff --git a/Documentation/core-api/kernel-api.rst b/Documentation/core-api/kernel-api.rst
index a29c99d13331..65266fa1b706 100644
--- a/Documentation/core-api/kernel-api.rst
+++ b/Documentation/core-api/kernel-api.rst
@@ -51,7 +51,7 @@ The Linux kernel provides more basic utility functions.
Bit Operations
--------------
-.. kernel-doc:: arch/x86/include/asm/bitops.h
+.. kernel-doc:: include/asm-generic/bitops-instrumented.h
:internal:
Bitmap Operations
diff --git a/arch/x86/include/asm/bitops.h b/arch/x86/include/asm/bitops.h
index 8e790ec219a5..ba15d53c1ca7 100644
--- a/arch/x86/include/asm/bitops.h
+++ b/arch/x86/include/asm/bitops.h
@@ -49,23 +49,8 @@
#define CONST_MASK_ADDR(nr, addr) WBYTE_ADDR((void *)(addr) + ((nr)>>3))
#define CONST_MASK(nr) (1 << ((nr) & 7))
-/**
- * set_bit - Atomically set a bit in memory
- * @nr: the bit to set
- * @addr: the address to start counting from
- *
- * This function is atomic and may not be reordered. See __set_bit()
- * if you do not require the atomic guarantees.
- *
- * Note: there are no guarantees that this function will not be reordered
- * on non x86 architectures, so if you are writing portable code,
- * make sure not to rely on its reordering guarantees.
- *
- * Note that @nr may be almost arbitrarily large; this function is not
- * restricted to acting on a single-word quantity.
- */
static __always_inline void
-set_bit(long nr, volatile unsigned long *addr)
+arch_set_bit(long nr, volatile unsigned long *addr)
{
if (IS_IMMEDIATE(nr)) {
asm volatile(LOCK_PREFIX "orb %1,%0"
@@ -78,32 +63,14 @@ set_bit(long nr, volatile unsigned long *addr)
}
}
-/**
- * __set_bit - Set a bit in memory
- * @nr: the bit to set
- * @addr: the address to start counting from
- *
- * Unlike set_bit(), this function is non-atomic and may be reordered.
- * If it's called on the same region of memory simultaneously, the effect
- * may be that only one operation succeeds.
- */
-static __always_inline void __set_bit(long nr, volatile unsigned long *addr)
+static __always_inline void
+arch___set_bit(long nr, volatile unsigned long *addr)
{
asm volatile(__ASM_SIZE(bts) " %1,%0" : : ADDR, "Ir" (nr) : "memory");
}
-/**
- * clear_bit - Clears a bit in memory
- * @nr: Bit to clear
- * @addr: Address to start counting from
- *
- * clear_bit() is atomic and may not be reordered. However, it does
- * not contain a memory barrier, so if it is used for locking purposes,
- * you should call smp_mb__before_atomic() and/or smp_mb__after_atomic()
- * in order to ensure changes are visible on other processors.
- */
static __always_inline void
-clear_bit(long nr, volatile unsigned long *addr)
+arch_clear_bit(long nr, volatile unsigned long *addr)
{
if (IS_IMMEDIATE(nr)) {
asm volatile(LOCK_PREFIX "andb %1,%0"
@@ -115,26 +82,21 @@ clear_bit(long nr, volatile unsigned long *addr)
}
}
-/*
- * clear_bit_unlock - Clears a bit in memory
- * @nr: Bit to clear
- * @addr: Address to start counting from
- *
- * clear_bit() is atomic and implies release semantics before the memory
- * operation. It can be used for an unlock.
- */
-static __always_inline void clear_bit_unlock(long nr, volatile unsigned long *addr)
+static __always_inline void
+arch_clear_bit_unlock(long nr, volatile unsigned long *addr)
{
barrier();
- clear_bit(nr, addr);
+ arch_clear_bit(nr, addr);
}
-static __always_inline void __clear_bit(long nr, volatile unsigned long *addr)
+static __always_inline void
+arch___clear_bit(long nr, volatile unsigned long *addr)
{
asm volatile(__ASM_SIZE(btr) " %1,%0" : : ADDR, "Ir" (nr) : "memory");
}
-static __always_inline bool clear_bit_unlock_is_negative_byte(long nr, volatile unsigned long *addr)
+static __always_inline bool
+arch_clear_bit_unlock_is_negative_byte(long nr, volatile unsigned long *addr)
{
bool negative;
asm volatile(LOCK_PREFIX "andb %2,%1"
@@ -143,48 +105,23 @@ static __always_inline bool clear_bit_unlock_is_negative_byte(long nr, volatile
: "ir" ((char) ~(1 << nr)) : "memory");
return negative;
}
+#define arch_clear_bit_unlock_is_negative_byte \
+ arch_clear_bit_unlock_is_negative_byte
-// Let everybody know we have it
-#define clear_bit_unlock_is_negative_byte clear_bit_unlock_is_negative_byte
-
-/*
- * __clear_bit_unlock - Clears a bit in memory
- * @nr: Bit to clear
- * @addr: Address to start counting from
- *
- * __clear_bit() is non-atomic and implies release semantics before the memory
- * operation. It can be used for an unlock if no other CPUs can concurrently
- * modify other bits in the word.
- */
-static __always_inline void __clear_bit_unlock(long nr, volatile unsigned long *addr)
+static __always_inline void
+arch___clear_bit_unlock(long nr, volatile unsigned long *addr)
{
- __clear_bit(nr, addr);
+ arch___clear_bit(nr, addr);
}
-/**
- * __change_bit - Toggle a bit in memory
- * @nr: the bit to change
- * @addr: the address to start counting from
- *
- * Unlike change_bit(), this function is non-atomic and may be reordered.
- * If it's called on the same region of memory simultaneously, the effect
- * may be that only one operation succeeds.
- */
-static __always_inline void __change_bit(long nr, volatile unsigned long *addr)
+static __always_inline void
+arch___change_bit(long nr, volatile unsigned long *addr)
{
asm volatile(__ASM_SIZE(btc) " %1,%0" : : ADDR, "Ir" (nr) : "memory");
}
-/**
- * change_bit - Toggle a bit in memory
- * @nr: Bit to change
- * @addr: Address to start counting from
- *
- * change_bit() is atomic and may not be reordered.
- * Note that @nr may be almost arbitrarily large; this function is not
- * restricted to acting on a single-word quantity.
- */
-static __always_inline void change_bit(long nr, volatile unsigned long *addr)
+static __always_inline void
+arch_change_bit(long nr, volatile unsigned long *addr)
{
if (IS_IMMEDIATE(nr)) {
asm volatile(LOCK_PREFIX "xorb %1,%0"
@@ -196,42 +133,20 @@ static __always_inline void change_bit(long nr, volatile unsigned long *addr)
}
}
-/**
- * test_and_set_bit - Set a bit and return its old value
- * @nr: Bit to set
- * @addr: Address to count from
- *
- * This operation is atomic and cannot be reordered.
- * It also implies a memory barrier.
- */
-static __always_inline bool test_and_set_bit(long nr, volatile unsigned long *addr)
+static __always_inline bool
+arch_test_and_set_bit(long nr, volatile unsigned long *addr)
{
return GEN_BINARY_RMWcc(LOCK_PREFIX __ASM_SIZE(bts), *addr, c, "Ir", nr);
}
-/**
- * test_and_set_bit_lock - Set a bit and return its old value for lock
- * @nr: Bit to set
- * @addr: Address to count from
- *
- * This is the same as test_and_set_bit on x86.
- */
static __always_inline bool
-test_and_set_bit_lock(long nr, volatile unsigned long *addr)
+arch_test_and_set_bit_lock(long nr, volatile unsigned long *addr)
{
- return test_and_set_bit(nr, addr);
+ return arch_test_and_set_bit(nr, addr);
}
-/**
- * __test_and_set_bit - Set a bit and return its old value
- * @nr: Bit to set
- * @addr: Address to count from
- *
- * This operation is non-atomic and can be reordered.
- * If two examples of this operation race, one can appear to succeed
- * but actually fail. You must protect multiple accesses with a lock.
- */
-static __always_inline bool __test_and_set_bit(long nr, volatile unsigned long *addr)
+static __always_inline bool
+arch___test_and_set_bit(long nr, volatile unsigned long *addr)
{
bool oldbit;
@@ -242,28 +157,13 @@ static __always_inline bool __test_and_set_bit(long nr, volatile unsigned long *
return oldbit;
}
-/**
- * test_and_clear_bit - Clear a bit and return its old value
- * @nr: Bit to clear
- * @addr: Address to count from
- *
- * This operation is atomic and cannot be reordered.
- * It also implies a memory barrier.
- */
-static __always_inline bool test_and_clear_bit(long nr, volatile unsigned long *addr)
+static __always_inline bool
+arch_test_and_clear_bit(long nr, volatile unsigned long *addr)
{
return GEN_BINARY_RMWcc(LOCK_PREFIX __ASM_SIZE(btr), *addr, c, "Ir", nr);
}
-/**
- * __test_and_clear_bit - Clear a bit and return its old value
- * @nr: Bit to clear
- * @addr: Address to count from
- *
- * This operation is non-atomic and can be reordered.
- * If two examples of this operation race, one can appear to succeed
- * but actually fail. You must protect multiple accesses with a lock.
- *
+/*
* Note: the operation is performed atomically with respect to
* the local CPU, but not other CPUs. Portable code should not
* rely on this behaviour.
@@ -271,7 +171,8 @@ static __always_inline bool test_and_clear_bit(long nr, volatile unsigned long *
* accessed from a hypervisor on the same CPU if running in a VM: don't change
* this without also updating arch/x86/kernel/kvm.c
*/
-static __always_inline bool __test_and_clear_bit(long nr, volatile unsigned long *addr)
+static __always_inline bool
+arch___test_and_clear_bit(long nr, volatile unsigned long *addr)
{
bool oldbit;
@@ -282,8 +183,8 @@ static __always_inline bool __test_and_clear_bit(long nr, volatile unsigned long
return oldbit;
}
-/* WARNING: non atomic and it can be reordered! */
-static __always_inline bool __test_and_change_bit(long nr, volatile unsigned long *addr)
+static __always_inline bool
+arch___test_and_change_bit(long nr, volatile unsigned long *addr)
{
bool oldbit;
@@ -295,15 +196,8 @@ static __always_inline bool __test_and_change_bit(long nr, volatile unsigned lon
return oldbit;
}
-/**
- * test_and_change_bit - Change a bit and return its old value
- * @nr: Bit to change
- * @addr: Address to count from
- *
- * This operation is atomic and cannot be reordered.
- * It also implies a memory barrier.
- */
-static __always_inline bool test_and_change_bit(long nr, volatile unsigned long *addr)
+static __always_inline bool
+arch_test_and_change_bit(long nr, volatile unsigned long *addr)
{
return GEN_BINARY_RMWcc(LOCK_PREFIX __ASM_SIZE(btc), *addr, c, "Ir", nr);
}
@@ -326,16 +220,7 @@ static __always_inline bool variable_test_bit(long nr, volatile const unsigned l
return oldbit;
}
-#if 0 /* Fool kernel-doc since it doesn't do macros yet */
-/**
- * test_bit - Determine whether a bit is set
- * @nr: bit number to test
- * @addr: Address to start counting from
- */
-static bool test_bit(int nr, const volatile unsigned long *addr);
-#endif
-
-#define test_bit(nr, addr) \
+#define arch_test_bit(nr, addr) \
(__builtin_constant_p((nr)) \
? constant_test_bit((nr), (addr)) \
: variable_test_bit((nr), (addr)))
@@ -504,6 +389,8 @@ static __always_inline int fls64(__u64 x)
#include <asm-generic/bitops/const_hweight.h>
+#include <asm-generic/bitops-instrumented.h>
+
#include <asm-generic/bitops/le.h>
#include <asm-generic/bitops/ext2-atomic-setbit.h>
diff --git a/include/asm-generic/bitops-instrumented.h b/include/asm-generic/bitops-instrumented.h
new file mode 100644
index 000000000000..ddd1c6d9d8db
--- /dev/null
+++ b/include/asm-generic/bitops-instrumented.h
@@ -0,0 +1,263 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+/*
+ * This file provides wrappers with sanitizer instrumentation for bit
+ * operations.
+ *
+ * To use this functionality, an arch's bitops.h file needs to define each of
+ * the below bit operations with an arch_ prefix (e.g. arch_set_bit(),
+ * arch___set_bit(), etc.).
+ */
+#ifndef _ASM_GENERIC_BITOPS_INSTRUMENTED_H
+#define _ASM_GENERIC_BITOPS_INSTRUMENTED_H
+
+#include <linux/kasan-checks.h>
+
+/**
+ * set_bit - Atomically set a bit in memory
+ * @nr: the bit to set
+ * @addr: the address to start counting from
+ *
+ * This is a relaxed atomic operation (no implied memory barriers).
+ *
+ * Note that @nr may be almost arbitrarily large; this function is not
+ * restricted to acting on a single-word quantity.
+ */
+static inline void set_bit(long nr, volatile unsigned long *addr)
+{
+ kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
+ arch_set_bit(nr, addr);
+}
+
+/**
+ * __set_bit - Set a bit in memory
+ * @nr: the bit to set
+ * @addr: the address to start counting from
+ *
+ * Unlike set_bit(), this function is non-atomic. If it is called on the same
+ * region of memory concurrently, the effect may be that only one operation
+ * succeeds.
+ */
+static inline void __set_bit(long nr, volatile unsigned long *addr)
+{
+ kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
+ arch___set_bit(nr, addr);
+}
+
+/**
+ * clear_bit - Clears a bit in memory
+ * @nr: Bit to clear
+ * @addr: Address to start counting from
+ *
+ * This is a relaxed atomic operation (no implied memory barriers).
+ */
+static inline void clear_bit(long nr, volatile unsigned long *addr)
+{
+ kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
+ arch_clear_bit(nr, addr);
+}
+
+/**
+ * __clear_bit - Clears a bit in memory
+ * @nr: the bit to clear
+ * @addr: the address to start counting from
+ *
+ * Unlike clear_bit(), this function is non-atomic. If it is called on the same
+ * region of memory concurrently, the effect may be that only one operation
+ * succeeds.
+ */
+static inline void __clear_bit(long nr, volatile unsigned long *addr)
+{
+ kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
+ arch___clear_bit(nr, addr);
+}
+
+/**
+ * clear_bit_unlock - Clear a bit in memory, for unlock
+ * @nr: the bit to set
+ * @addr: the address to start counting from
+ *
+ * This operation is atomic and provides release barrier semantics.
+ */
+static inline void clear_bit_unlock(long nr, volatile unsigned long *addr)
+{
+ kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
+ arch_clear_bit_unlock(nr, addr);
+}
+
+/**
+ * __clear_bit_unlock - Clears a bit in memory
+ * @nr: Bit to clear
+ * @addr: Address to start counting from
+ *
+ * This is a non-atomic operation but implies a release barrier before the
+ * memory operation. It can be used for an unlock if no other CPUs can
+ * concurrently modify other bits in the word.
+ */
+static inline void __clear_bit_unlock(long nr, volatile unsigned long *addr)
+{
+ kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
+ arch___clear_bit_unlock(nr, addr);
+}
+
+/**
+ * change_bit - Toggle a bit in memory
+ * @nr: Bit to change
+ * @addr: Address to start counting from
+ *
+ * This is a relaxed atomic operation (no implied memory barriers).
+ *
+ * Note that @nr may be almost arbitrarily large; this function is not
+ * restricted to acting on a single-word quantity.
+ */
+static inline void change_bit(long nr, volatile unsigned long *addr)
+{
+ kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
+ arch_change_bit(nr, addr);
+}
+
+/**
+ * __change_bit - Toggle a bit in memory
+ * @nr: the bit to change
+ * @addr: the address to start counting from
+ *
+ * Unlike change_bit(), this function is non-atomic. If it is called on the same
+ * region of memory concurrently, the effect may be that only one operation
+ * succeeds.
+ */
+static inline void __change_bit(long nr, volatile unsigned long *addr)
+{
+ kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
+ arch___change_bit(nr, addr);
+}
+
+/**
+ * test_and_set_bit - Set a bit and return its old value
+ * @nr: Bit to set
+ * @addr: Address to count from
+ *
+ * This is an atomic fully-ordered operation (implied full memory barrier).
+ */
+static inline bool test_and_set_bit(long nr, volatile unsigned long *addr)
+{
+ kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
+ return arch_test_and_set_bit(nr, addr);
+}
+
+/**
+ * __test_and_set_bit - Set a bit and return its old value
+ * @nr: Bit to set
+ * @addr: Address to count from
+ *
+ * This operation is non-atomic. If two instances of this operation race, one
+ * can appear to succeed but actually fail.
+ */
+static inline bool __test_and_set_bit(long nr, volatile unsigned long *addr)
+{
+ kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
+ return arch___test_and_set_bit(nr, addr);
+}
+
+/**
+ * test_and_set_bit_lock - Set a bit and return its old value, for lock
+ * @nr: Bit to set
+ * @addr: Address to count from
+ *
+ * This operation is atomic and provides acquire barrier semantics if
+ * the returned value is 0.
+ * It can be used to implement bit locks.
+ */
+static inline bool test_and_set_bit_lock(long nr, volatile unsigned long *addr)
+{
+ kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
+ return arch_test_and_set_bit_lock(nr, addr);
+}
+
+/**
+ * test_and_clear_bit - Clear a bit and return its old value
+ * @nr: Bit to clear
+ * @addr: Address to count from
+ *
+ * This is an atomic fully-ordered operation (implied full memory barrier).
+ */
+static inline bool test_and_clear_bit(long nr, volatile unsigned long *addr)
+{
+ kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
+ return arch_test_and_clear_bit(nr, addr);
+}
+
+/**
+ * __test_and_clear_bit - Clear a bit and return its old value
+ * @nr: Bit to clear
+ * @addr: Address to count from
+ *
+ * This operation is non-atomic. If two instances of this operation race, one
+ * can appear to succeed but actually fail.
+ */
+static inline bool __test_and_clear_bit(long nr, volatile unsigned long *addr)
+{
+ kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
+ return arch___test_and_clear_bit(nr, addr);
+}
+
+/**
+ * test_and_change_bit - Change a bit and return its old value
+ * @nr: Bit to change
+ * @addr: Address to count from
+ *
+ * This is an atomic fully-ordered operation (implied full memory barrier).
+ */
+static inline bool test_and_change_bit(long nr, volatile unsigned long *addr)
+{
+ kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
+ return arch_test_and_change_bit(nr, addr);
+}
+
+/**
+ * __test_and_change_bit - Change a bit and return its old value
+ * @nr: Bit to change
+ * @addr: Address to count from
+ *
+ * This operation is non-atomic. If two instances of this operation race, one
+ * can appear to succeed but actually fail.
+ */
+static inline bool __test_and_change_bit(long nr, volatile unsigned long *addr)
+{
+ kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
+ return arch___test_and_change_bit(nr, addr);
+}
+
+/**
+ * test_bit - Determine whether a bit is set
+ * @nr: bit number to test
+ * @addr: Address to start counting from
+ */
+static inline bool test_bit(long nr, const volatile unsigned long *addr)
+{
+ kasan_check_read(addr + BIT_WORD(nr), sizeof(long));
+ return arch_test_bit(nr, addr);
+}
+
+#if defined(arch_clear_bit_unlock_is_negative_byte)
+/**
+ * clear_bit_unlock_is_negative_byte - Clear a bit in memory and test if bottom
+ * byte is negative, for unlock.
+ * @nr: the bit to clear
+ * @addr: the address to start counting from
+ *
+ * This operation is atomic and provides release barrier semantics.
+ *
+ * This is a bit of a one-trick-pony for the filemap code, which clears
+ * PG_locked and tests PG_waiters,
+ */
+static inline bool
+clear_bit_unlock_is_negative_byte(long nr, volatile unsigned long *addr)
+{
+ kasan_check_write(addr + BIT_WORD(nr), sizeof(long));
+ return arch_clear_bit_unlock_is_negative_byte(nr, addr);
+}
+/* Let everybody know we have it. */
+#define clear_bit_unlock_is_negative_byte clear_bit_unlock_is_negative_byte
+#endif
+
+#endif /* _ASM_GENERIC_BITOPS_INSTRUMENTED_H */
--
2.22.0.rc2.383.gf4fbbf30c2-goog
^ permalink raw reply related
* Re: [PATCH v3 1/3] lib/test_kasan: Add bitops tests
From: Marco Elver @ 2019-06-13 12:35 UTC (permalink / raw)
To: Andrey Ryabinin
Cc: Peter Zijlstra, Dmitry Vyukov, Alexander Potapenko,
Andrey Konovalov, Mark Rutland, H. Peter Anvin, Jonathan Corbet,
Thomas Gleixner, Ingo Molnar, Borislav Petkov,
the arch/x86 maintainers, Arnd Bergmann, Josh Poimboeuf,
open list:DOCUMENTATION, LKML, linux-arch, kasan-dev
In-Reply-To: <5c35bc08-749f-dbc4-09d0-fcf14b1da1b3@virtuozzo.com>
Thanks, I've sent v4.
On Thu, 13 Jun 2019 at 12:49, Andrey Ryabinin <aryabinin@virtuozzo.com> wrote:
>
>
>
> On 5/31/19 6:08 PM, Marco Elver wrote:
> > This adds bitops tests to the test_kasan module. In a follow-up patch,
> > support for bitops instrumentation will be added.
> >
> > Signed-off-by: Marco Elver <elver@google.com>
> > ---
> > Changes in v3:
> > * Use kzalloc instead of kmalloc.
> > * Use sizeof(*bits).
> >
> > Changes in v2:
> > * Use BITS_PER_LONG.
> > * Use heap allocated memory for test, as newer compilers (correctly)
> > warn on OOB stack access.
> > ---
> > lib/test_kasan.c | 75 ++++++++++++++++++++++++++++++++++++++++++++++--
> > 1 file changed, 72 insertions(+), 3 deletions(-)
> >
> > diff --git a/lib/test_kasan.c b/lib/test_kasan.c
> > index 7de2702621dc..1ef9702327d2 100644
> > --- a/lib/test_kasan.c
> > +++ b/lib/test_kasan.c
> > @@ -11,16 +11,17 @@
> >
> > #define pr_fmt(fmt) "kasan test: %s " fmt, __func__
> >
> > +#include <linux/bitops.h>
> > #include <linux/delay.h>
> > +#include <linux/kasan.h>
> > #include <linux/kernel.h>
> > -#include <linux/mman.h>
> > #include <linux/mm.h>
> > +#include <linux/mman.h>
> > +#include <linux/module.h>
> > #include <linux/printk.h>
> > #include <linux/slab.h>
> > #include <linux/string.h>
> > #include <linux/uaccess.h>
> > -#include <linux/module.h>
> > -#include <linux/kasan.h>
> >
> > /*
> > * Note: test functions are marked noinline so that their names appear in
> > @@ -623,6 +624,73 @@ static noinline void __init kasan_strings(void)
> > strnlen(ptr, 1);
> > }
> >
> > +static noinline void __init kasan_bitops(void)
> > +{
> > + long *bits = kzalloc(sizeof(*bits), GFP_KERNEL);
>
> It would be safer to do kzalloc(sizeof(*bits) + 1, GFP_KERNEL) and change tests accordingly to: set_bit(BITS_PER_LONG + 1, bits) ...
> kmalloc will internally round up allocation to 16-bytes, so we won't be actually corrupting someone elses memory.
>
>
> > + if (!bits)
> > + return;
> > +
> > + pr_info("within-bounds in set_bit");
> > + set_bit(0, bits);
> > +
> > + pr_info("within-bounds in set_bit");
> > + set_bit(BITS_PER_LONG - 1, bits);
>
>
> I'd remove these two. There are plenty of within bounds set_bit() in the kernel so they are well tested already.
>
> --
> You received this message because you are subscribed to the Google Groups "kasan-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to kasan-dev+unsubscribe@googlegroups.com.
> To post to this group, send email to kasan-dev@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/kasan-dev/5c35bc08-749f-dbc4-09d0-fcf14b1da1b3%40virtuozzo.com.
> For more options, visit https://groups.google.com/d/optout.
^ permalink raw reply
* [PATCH v4 2/3] x86: Use static_cpu_has in uaccess region to avoid instrumentation
From: Marco Elver @ 2019-06-13 12:30 UTC (permalink / raw)
To: peterz, aryabinin, dvyukov, glider, andreyknvl, mark.rutland, hpa
Cc: corbet, tglx, mingo, bp, x86, arnd, jpoimboe, linux-doc,
linux-kernel, linux-arch, kasan-dev, Marco Elver
In-Reply-To: <20190613123028.179447-1-elver@google.com>
This patch is a pre-requisite for enabling KASAN bitops instrumentation;
using static_cpu_has instead of boot_cpu_has avoids instrumentation of
test_bit inside the uaccess region. With instrumentation, the KASAN
check would otherwise be flagged by objtool.
For consistency, kernel/signal.c was changed to mirror this change,
however, is never instrumented with KASAN (currently unsupported under
x86 32bit).
Signed-off-by: Marco Elver <elver@google.com>
Suggested-by: H. Peter Anvin <hpa@zytor.com>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
---
Changes in v3:
* Use static_cpu_has instead of moving boot_cpu_has outside uaccess
region.
Changes in v2:
* Replaces patch: 'tools/objtool: add kasan_check_* to uaccess
whitelist'
---
arch/x86/ia32/ia32_signal.c | 2 +-
arch/x86/kernel/signal.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/x86/ia32/ia32_signal.c b/arch/x86/ia32/ia32_signal.c
index 629d1ee05599..1cee10091b9f 100644
--- a/arch/x86/ia32/ia32_signal.c
+++ b/arch/x86/ia32/ia32_signal.c
@@ -358,7 +358,7 @@ int ia32_setup_rt_frame(int sig, struct ksignal *ksig,
put_user_ex(ptr_to_compat(&frame->uc), &frame->puc);
/* Create the ucontext. */
- if (boot_cpu_has(X86_FEATURE_XSAVE))
+ if (static_cpu_has(X86_FEATURE_XSAVE))
put_user_ex(UC_FP_XSTATE, &frame->uc.uc_flags);
else
put_user_ex(0, &frame->uc.uc_flags);
diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
index 364813cea647..52eb1d551aed 100644
--- a/arch/x86/kernel/signal.c
+++ b/arch/x86/kernel/signal.c
@@ -391,7 +391,7 @@ static int __setup_rt_frame(int sig, struct ksignal *ksig,
put_user_ex(&frame->uc, &frame->puc);
/* Create the ucontext. */
- if (boot_cpu_has(X86_FEATURE_XSAVE))
+ if (static_cpu_has(X86_FEATURE_XSAVE))
put_user_ex(UC_FP_XSTATE, &frame->uc.uc_flags);
else
put_user_ex(0, &frame->uc.uc_flags);
--
2.22.0.rc2.383.gf4fbbf30c2-goog
^ permalink raw reply related
* [PATCH v4 1/3] lib/test_kasan: Add bitops tests
From: Marco Elver @ 2019-06-13 12:30 UTC (permalink / raw)
To: peterz, aryabinin, dvyukov, glider, andreyknvl, mark.rutland, hpa
Cc: corbet, tglx, mingo, bp, x86, arnd, jpoimboe, linux-doc,
linux-kernel, linux-arch, kasan-dev, Marco Elver
In-Reply-To: <20190613123028.179447-1-elver@google.com>
This adds bitops tests to the test_kasan module. In a follow-up patch,
support for bitops instrumentation will be added.
Signed-off-by: Marco Elver <elver@google.com>
Acked-by: Mark Rutland <mark.rutland@arm.com>
---
Changes in v4:
* Remove "within-bounds" tests.
* Allocate sizeof(*bite) + 1, to not actually corrupt other memory in
case instrumentation isn't working.
* Clarify that accesses operate on whole longs, which causes OOB
regardless of the bit accessed beyond the first long in the test.
Changes in v3:
* Use kzalloc instead of kmalloc.
* Use sizeof(*bits).
Changes in v2:
* Use BITS_PER_LONG.
* Use heap allocated memory for test, as newer compilers (correctly)
warn on OOB stack access.
---
lib/test_kasan.c | 82 ++++++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 79 insertions(+), 3 deletions(-)
diff --git a/lib/test_kasan.c b/lib/test_kasan.c
index 7de2702621dc..e76a4711d456 100644
--- a/lib/test_kasan.c
+++ b/lib/test_kasan.c
@@ -11,16 +11,17 @@
#define pr_fmt(fmt) "kasan test: %s " fmt, __func__
+#include <linux/bitops.h>
#include <linux/delay.h>
+#include <linux/kasan.h>
#include <linux/kernel.h>
-#include <linux/mman.h>
#include <linux/mm.h>
+#include <linux/mman.h>
+#include <linux/module.h>
#include <linux/printk.h>
#include <linux/slab.h>
#include <linux/string.h>
#include <linux/uaccess.h>
-#include <linux/module.h>
-#include <linux/kasan.h>
/*
* Note: test functions are marked noinline so that their names appear in
@@ -623,6 +624,80 @@ static noinline void __init kasan_strings(void)
strnlen(ptr, 1);
}
+static noinline void __init kasan_bitops(void)
+{
+ /*
+ * Allocate 1 more byte, which causes kzalloc to round up to 16-bytes;
+ * this way we do not actually corrupt other memory, in case
+ * instrumentation is not working as intended.
+ */
+ long *bits = kzalloc(sizeof(*bits) + 1, GFP_KERNEL);
+ if (!bits)
+ return;
+
+ /*
+ * Below calls try to access bit within allocated memory; however, the
+ * below accesses are still out-of-bounds, since bitops are defined to
+ * operate on the whole long the bit is in.
+ */
+ pr_info("out-of-bounds in set_bit\n");
+ set_bit(BITS_PER_LONG, bits);
+
+ pr_info("out-of-bounds in __set_bit\n");
+ __set_bit(BITS_PER_LONG, bits);
+
+ pr_info("out-of-bounds in clear_bit\n");
+ clear_bit(BITS_PER_LONG, bits);
+
+ pr_info("out-of-bounds in __clear_bit\n");
+ __clear_bit(BITS_PER_LONG, bits);
+
+ pr_info("out-of-bounds in clear_bit_unlock\n");
+ clear_bit_unlock(BITS_PER_LONG, bits);
+
+ pr_info("out-of-bounds in __clear_bit_unlock\n");
+ __clear_bit_unlock(BITS_PER_LONG, bits);
+
+ pr_info("out-of-bounds in change_bit\n");
+ change_bit(BITS_PER_LONG, bits);
+
+ pr_info("out-of-bounds in __change_bit\n");
+ __change_bit(BITS_PER_LONG, bits);
+
+ /*
+ * Below calls try to access bit beyond allocated memory.
+ */
+ pr_info("out-of-bounds in test_and_set_bit\n");
+ test_and_set_bit(BITS_PER_LONG + BITS_PER_BYTE, bits);
+
+ pr_info("out-of-bounds in __test_and_set_bit\n");
+ __test_and_set_bit(BITS_PER_LONG + BITS_PER_BYTE, bits);
+
+ pr_info("out-of-bounds in test_and_set_bit_lock\n");
+ test_and_set_bit_lock(BITS_PER_LONG + BITS_PER_BYTE, bits);
+
+ pr_info("out-of-bounds in test_and_clear_bit\n");
+ test_and_clear_bit(BITS_PER_LONG + BITS_PER_BYTE, bits);
+
+ pr_info("out-of-bounds in __test_and_clear_bit\n");
+ __test_and_clear_bit(BITS_PER_LONG + BITS_PER_BYTE, bits);
+
+ pr_info("out-of-bounds in test_and_change_bit\n");
+ test_and_change_bit(BITS_PER_LONG + BITS_PER_BYTE, bits);
+
+ pr_info("out-of-bounds in __test_and_change_bit\n");
+ __test_and_change_bit(BITS_PER_LONG + BITS_PER_BYTE, bits);
+
+ pr_info("out-of-bounds in test_bit\n");
+ (void)test_bit(BITS_PER_LONG + BITS_PER_BYTE, bits);
+
+#if defined(clear_bit_unlock_is_negative_byte)
+ pr_info("out-of-bounds in clear_bit_unlock_is_negative_byte\n");
+ clear_bit_unlock_is_negative_byte(BITS_PER_LONG + BITS_PER_BYTE, bits);
+#endif
+ kfree(bits);
+}
+
static int __init kmalloc_tests_init(void)
{
/*
@@ -664,6 +739,7 @@ static int __init kmalloc_tests_init(void)
kasan_memchr();
kasan_memcmp();
kasan_strings();
+ kasan_bitops();
kasan_restore_multi_shot(multishot);
--
2.22.0.rc2.383.gf4fbbf30c2-goog
^ permalink raw reply related
* [PATCH v4 0/3] Bitops instrumentation for KASAN
From: Marco Elver @ 2019-06-13 12:30 UTC (permalink / raw)
To: peterz, aryabinin, dvyukov, glider, andreyknvl, mark.rutland, hpa
Cc: corbet, tglx, mingo, bp, x86, arnd, jpoimboe, linux-doc,
linux-kernel, linux-arch, kasan-dev, Marco Elver
Previous version:
http://lkml.kernel.org/r/20190531150828.157832-1-elver@google.com
* This version only changes lib/test_kasan.c.
* Remaining files are identical to v3.
Marco Elver (3):
lib/test_kasan: Add bitops tests
x86: Use static_cpu_has in uaccess region to avoid instrumentation
asm-generic, x86: Add bitops instrumentation for KASAN
Documentation/core-api/kernel-api.rst | 2 +-
arch/x86/ia32/ia32_signal.c | 2 +-
arch/x86/include/asm/bitops.h | 189 ++++------------
arch/x86/kernel/signal.c | 2 +-
include/asm-generic/bitops-instrumented.h | 263 ++++++++++++++++++++++
lib/test_kasan.c | 82 ++++++-
6 files changed, 383 insertions(+), 157 deletions(-)
create mode 100644 include/asm-generic/bitops-instrumented.h
--
2.22.0.rc2.383.gf4fbbf30c2-goog
^ permalink raw reply
* Re: [PATCH v4 1/2] arm64: Define Documentation/arm64/tagged-address-abi.txt
From: Szabolcs Nagy @ 2019-06-13 12:28 UTC (permalink / raw)
To: Vincenzo Frascino, Catalin Marinas
Cc: nd, linux-arm-kernel@lists.infradead.org,
linux-doc@vger.kernel.org, linux-mm@kvack.org,
linux-arch@vger.kernel.org, linux-kselftest@vger.kernel.org,
linux-kernel@vger.kernel.org, Will Deacon, Andrey Konovalov,
Alexander Viro
In-Reply-To: <6ebbda37-5dd9-d0d5-d9cb-286c7a5b7f8e@arm.com>
On 13/06/2019 12:16, Vincenzo Frascino wrote:
> Hi Szabolcs,
>
> thank you for your review.
>
> On 13/06/2019 11:14, Szabolcs Nagy wrote:
>> On 13/06/2019 10:20, Catalin Marinas wrote:
>>> Hi Szabolcs,
>>>
>>> On Wed, Jun 12, 2019 at 05:30:34PM +0100, Szabolcs Nagy wrote:
>>>> On 12/06/2019 15:21, Vincenzo Frascino wrote:
>>>>> +2. ARM64 Tagged Address ABI
>>>>> +---------------------------
>>>>> +
>>>>> +From the kernel syscall interface prospective, we define, for the purposes
>>>> ^^^^^^^^^^^
>>>> perspective
>>>>
>>>>> +of this document, a "valid tagged pointer" as a pointer that either it has
>>>>> +a zero value set in the top byte or it has a non-zero value, it is in memory
>>>>> +ranges privately owned by a userspace process and it is obtained in one of
>>>>> +the following ways:
>>>>> + - mmap() done by the process itself, where either:
>>>>> + * flags = MAP_PRIVATE | MAP_ANONYMOUS
>>>>> + * flags = MAP_PRIVATE and the file descriptor refers to a regular
>>>>> + file or "/dev/zero"
>>>>
>>>> this does not make it clear if MAP_FIXED or other flags are valid
>>>> (there are many map flags i don't know, but at least fixed should work
>>>> and stack/growsdown. i'd expect anything that's not incompatible with
>>>> private|anon to work).
>>>
>>> Just to clarify, this document tries to define the memory ranges from
>>> where tagged addresses can be passed into the kernel in the context
>>> of TBI only (not MTE); that is for hwasan support. FIXED or GROWSDOWN
>>> should not affect this.
>>
>> yes, so either the text should list MAP_* flags that don't affect
>> the pointer tagging semantics or specify private|anon mapping
>> with different wording.
>>
>
> Good point. Could you please propose a wording that would be suitable for this case?
i don't know all the MAP_ magic, but i think it's enough to change
the "flags =" to
* flags have MAP_PRIVATE and MAP_ANONYMOUS set or
* flags have MAP_PRIVATE set and the file descriptor refers to...
>>>>> + - a mapping below sbrk(0) done by the process itself
>>>>
>>>> doesn't the mmap rule cover this?
>>>
>>> IIUC it doesn't cover it as that's memory mapped by the kernel
>>> automatically on access vs a pointer returned by mmap(). The statement
>>> above talks about how the address is obtained by the user.
>>
>> ok i read 'mapping below sbrk' as an mmap (possibly MAP_FIXED)
>> that happens to be below the heap area.
>>
>> i think "below sbrk(0)" is not the best term to use: there
>> may be address range below the heap area that can be mmapped
>> and thus below sbrk(0) and sbrk is a posix api not a linux
>> syscall, the libc can implement it with mmap or whatever.
>>
>> i'm not sure what the right term for 'heap area' is
>> (the address range between syscall(__NR_brk,0) at
>> program startup and its current value?)
>>
>
> I used sbrk(0) with the meaning of "end of the process's data segment" not
> implying that this is a syscall, but just as a useful way to identify the mapping.
> I agree that it is a posix function implemented by libc but when it is used with
> 0 finds the current location of the program break, which can be changed by brk()
> and depending on the new address passed to this syscall can have the effect of
> allocating or deallocating memory.
>
> Will changing sbrk(0) with "end of the process's data segment" make it more clear?
i don't understand what's the relevance of the *end*
of the data segment.
i'd expect the text to say something about the address
range of the data segment.
i can do
mmap((void*)65536, 65536, PROT_READ|PROT_WRITE, MAP_FIXED|MAP_SHARED|MAP_ANON, -1, 0);
and it will be below the end of the data segment.
>
> I will add what you are suggesting about the heap area.
>
^ permalink raw reply
* Re: [PATCH v4 1/2] arm64: Define Documentation/arm64/tagged-address-abi.txt
From: Catalin Marinas @ 2019-06-13 12:28 UTC (permalink / raw)
To: Dave Martin
Cc: Vincenzo Frascino, linux-arch, linux-doc, Szabolcs Nagy,
Andrey Konovalov, Will Deacon, linux-kernel, linux-mm,
Alexander Viro, linux-kselftest, linux-arm-kernel
In-Reply-To: <20190613113731.GY28398@e103592.cambridge.arm.com>
On Thu, Jun 13, 2019 at 12:37:32PM +0100, Dave P Martin wrote:
> On Thu, Jun 13, 2019 at 11:15:34AM +0100, Vincenzo Frascino wrote:
> > On 12/06/2019 16:35, Catalin Marinas wrote:
> > > On Wed, Jun 12, 2019 at 03:21:10PM +0100, Vincenzo Frascino wrote:
> > >> + - PR_GET_TAGGED_ADDR_CTRL: can be used to check the status of the Tagged
> > >> + Address ABI.
[...]
> Is there a canonical way to detect whether this whole API/ABI is
> available? (i.e., try to call this prctl / check for an HWCAP bit,
> etc.)
The canonical way is a prctl() call. HWCAP doesn't make sense since it's
not a hardware feature. If you really want a different way of detecting
this (which I don't think it's worth), we can reinstate the AT_FLAGS
bit.
--
Catalin
^ permalink raw reply
* Re: [PATCH v4 1/2] arm64: Define Documentation/arm64/tagged-address-abi.txt
From: Dave Martin @ 2019-06-13 11:37 UTC (permalink / raw)
To: Vincenzo Frascino
Cc: Catalin Marinas, linux-arch, linux-doc, Szabolcs Nagy,
Andrey Konovalov, Will Deacon, linux-kernel, linux-mm,
Alexander Viro, linux-kselftest, linux-arm-kernel
In-Reply-To: <141c740a-94c2-2243-b6d1-b44ffee43791@arm.com>
On Thu, Jun 13, 2019 at 11:15:34AM +0100, Vincenzo Frascino wrote:
> Hi Catalin,
>
> On 12/06/2019 16:35, Catalin Marinas wrote:
> > Hi Vincenzo,
> >
> > Some minor comments below but it looks fine to me overall. Cc'ing
> > Szabolcs as well since I'd like a view from the libc people.
> >
>
> Thanks for this, I saw Szabolcs comments.
>
> > On Wed, Jun 12, 2019 at 03:21:10PM +0100, Vincenzo Frascino wrote:
> >> diff --git a/Documentation/arm64/tagged-address-abi.txt b/Documentation/arm64/tagged-address-abi.txt
> >> new file mode 100644
> >> index 000000000000..96e149e2c55c
> >> --- /dev/null
> >> +++ b/Documentation/arm64/tagged-address-abi.txt
[...]
> >> +Since it is not desirable to relax the ABI to allow tagged user addresses
> >> +into the kernel indiscriminately, arm64 provides a new sysctl interface
> >> +(/proc/sys/abi/tagged_addr) that is used to prevent the applications from
> >> +enabling the relaxed ABI and a new prctl() interface that can be used to
> >> +enable or disable the relaxed ABI.
> >> +
> >> +The sysctl is meant also for testing purposes in order to provide a simple
> >> +way for the userspace to verify the return error checking of the prctl()
> >> +command without having to reconfigure the kernel.
> >> +
> >> +The ABI properties are inherited by threads of the same application and
> >> +fork()'ed children but cleared when a new process is spawn (execve()).
> >
> > "spawned".
I'd just say "cleared by execve()."
"Spawn" suggests (v)fork+exec to me (at least, what's what "spawn" means on
certain other OSes).
> >
> > I guess you could drop these three paragraphs here and mention the
> > inheritance properties when introducing the prctl() below. You can also
> > mention the global sysctl switch after the prctl() was introduced.
> >
>
> I will move the last two (rewording them) to the _section_ 2, but I would still
> prefer the Introduction to give an overview of the solution as well.
>
> >> +
> >> +2. ARM64 Tagged Address ABI
> >> +---------------------------
> >> +
> >> +From the kernel syscall interface prospective, we define, for the purposes
> >> +of this document, a "valid tagged pointer" as a pointer that either it has
> >
> > "either has" (no 'it') sounds slightly better but I'm not a native
> > English speaker either.
> >
> >> +a zero value set in the top byte or it has a non-zero value, it is in memory
> >> +ranges privately owned by a userspace process and it is obtained in one of
> >> +the following ways:
> >> + - mmap() done by the process itself, where either:
> >> + * flags = MAP_PRIVATE | MAP_ANONYMOUS
> >> + * flags = MAP_PRIVATE and the file descriptor refers to a regular
> >> + file or "/dev/zero"
> >> + - a mapping below sbrk(0) done by the process itself
> >> + - any memory mapped by the kernel in the process's address space during
> >> + creation and following the restrictions presented above (i.e. data, bss,
> >> + stack).
> >> +
> >> +The ARM64 Tagged Address ABI is an opt-in feature, and an application can
> >> +control it using the following prctl()s:
> >> + - PR_SET_TAGGED_ADDR_CTRL: can be used to enable the Tagged Address ABI.
> >
> > enable or disable (not sure we need the latter but it doesn't heart).
> >
> > I'd add the arg2 description here as well.
> >
>
> Good point I missed this.
>
> >> + - PR_GET_TAGGED_ADDR_CTRL: can be used to check the status of the Tagged
> >> + Address ABI.
For both prctls, you should also document the zeroed arguments up to
arg5 (unless we get rid of the enforcement and just ignore them).
Is there a canonical way to detect whether this whole API/ABI is
available? (i.e., try to call this prctl / check for an HWCAP bit,
etc.)
[...]
Cheers
---Dave
^ permalink raw reply
* Re: [PATCH v4 1/2] arm64: Define Documentation/arm64/tagged-address-abi.txt
From: Vincenzo Frascino @ 2019-06-13 11:16 UTC (permalink / raw)
To: Szabolcs Nagy, Catalin Marinas
Cc: nd, linux-arm-kernel@lists.infradead.org,
linux-doc@vger.kernel.org, linux-mm@kvack.org,
linux-arch@vger.kernel.org, linux-kselftest@vger.kernel.org,
linux-kernel@vger.kernel.org, Will Deacon, Andrey Konovalov,
Alexander Viro
In-Reply-To: <dee7f192-d0f0-558e-3007-eba805c6f2da@arm.com>
Hi Szabolcs,
thank you for your review.
On 13/06/2019 11:14, Szabolcs Nagy wrote:
> On 13/06/2019 10:20, Catalin Marinas wrote:
>> Hi Szabolcs,
>>
>> On Wed, Jun 12, 2019 at 05:30:34PM +0100, Szabolcs Nagy wrote:
>>> On 12/06/2019 15:21, Vincenzo Frascino wrote:
>>>> +2. ARM64 Tagged Address ABI
>>>> +---------------------------
>>>> +
>>>> +From the kernel syscall interface prospective, we define, for the purposes
>>> ^^^^^^^^^^^
>>> perspective
>>>
>>>> +of this document, a "valid tagged pointer" as a pointer that either it has
>>>> +a zero value set in the top byte or it has a non-zero value, it is in memory
>>>> +ranges privately owned by a userspace process and it is obtained in one of
>>>> +the following ways:
>>>> + - mmap() done by the process itself, where either:
>>>> + * flags = MAP_PRIVATE | MAP_ANONYMOUS
>>>> + * flags = MAP_PRIVATE and the file descriptor refers to a regular
>>>> + file or "/dev/zero"
>>>
>>> this does not make it clear if MAP_FIXED or other flags are valid
>>> (there are many map flags i don't know, but at least fixed should work
>>> and stack/growsdown. i'd expect anything that's not incompatible with
>>> private|anon to work).
>>
>> Just to clarify, this document tries to define the memory ranges from
>> where tagged addresses can be passed into the kernel in the context
>> of TBI only (not MTE); that is for hwasan support. FIXED or GROWSDOWN
>> should not affect this.
>
> yes, so either the text should list MAP_* flags that don't affect
> the pointer tagging semantics or specify private|anon mapping
> with different wording.
>
Good point. Could you please propose a wording that would be suitable for this case?
>>>> + - a mapping below sbrk(0) done by the process itself
>>>
>>> doesn't the mmap rule cover this?
>>
>> IIUC it doesn't cover it as that's memory mapped by the kernel
>> automatically on access vs a pointer returned by mmap(). The statement
>> above talks about how the address is obtained by the user.
>
> ok i read 'mapping below sbrk' as an mmap (possibly MAP_FIXED)
> that happens to be below the heap area.
>
> i think "below sbrk(0)" is not the best term to use: there
> may be address range below the heap area that can be mmapped
> and thus below sbrk(0) and sbrk is a posix api not a linux
> syscall, the libc can implement it with mmap or whatever.
>
> i'm not sure what the right term for 'heap area' is
> (the address range between syscall(__NR_brk,0) at
> program startup and its current value?)
>
I used sbrk(0) with the meaning of "end of the process's data segment" not
implying that this is a syscall, but just as a useful way to identify the mapping.
I agree that it is a posix function implemented by libc but when it is used with
0 finds the current location of the program break, which can be changed by brk()
and depending on the new address passed to this syscall can have the effect of
allocating or deallocating memory.
Will changing sbrk(0) with "end of the process's data segment" make it more clear?
I will add what you are suggesting about the heap area.
>>>> + - any memory mapped by the kernel in the process's address space during
>>>> + creation and following the restrictions presented above (i.e. data, bss,
>>>> + stack).
>>>
>>> OK.
>>>
>>> Can a null pointer have a tag?
>>> (in case NULL is valid to pass to a syscall)
>>
>> Good point. I don't think it can. We may change this for MTE where we
>> give a hint tag but no hint address, however, this document only covers
>> TBI for now.
>
> OK.
>
>>>> +The ARM64 Tagged Address ABI is an opt-in feature, and an application can
>>>> +control it using the following prctl()s:
>>>> + - PR_SET_TAGGED_ADDR_CTRL: can be used to enable the Tagged Address ABI.
>>>> + - PR_GET_TAGGED_ADDR_CTRL: can be used to check the status of the Tagged
>>>> + Address ABI.
>>>> +
>>>> +As a consequence of invoking PR_SET_TAGGED_ADDR_CTRL prctl() by an applications,
>>>> +the ABI guarantees the following behaviours:
>>>> +
>>>> + - Every current or newly introduced syscall can accept any valid tagged
>>>> + pointers.
>>>> +
>>>> + - If a non valid tagged pointer is passed to a syscall then the behaviour
>>>> + is undefined.
>>>> +
>>>> + - Every valid tagged pointer is expected to work as an untagged one.
>>>> +
>>>> + - The kernel preserves any valid tagged pointers and returns them to the
>>>> + userspace unchanged in all the cases except the ones documented in the
>>>> + "Preserving tags" paragraph of tagged-pointers.txt.
>>>
>>> OK.
>>>
>>> i guess pointers of another process are not "valid tagged pointers"
>>> for the current one, so e.g. in ptrace the ptracer has to clear the
>>> tags before PEEK etc.
>>
>> Another good point. Are there any pros/cons here or use-cases? When we
>> add MTE support, should we handle this differently?
>
> i'm not sure what gdb does currently, but it has
> an 'address_significant' hook used at a few places
> that drops the tag on aarch64, so it probably
> avoids passing tagged pointer to ptrace.
>
> i was worried about strace which tries to print
> structs passed to syscalls and follow pointers in
> them which currently would work, but if we allow
> tags in syscalls then it needs some update.
> (i haven't checked the strace code though)
>>>>> +A definition of the meaning of tagged pointers on arm64 can be found in:
>>>> +Documentation/arm64/tagged-pointers.txt.
>>>> +
>>>> +3. ARM64 Tagged Address ABI Exceptions
>>>> +--------------------------------------
>>>> +
>>>> +The behaviours described in paragraph 2, with particular reference to the
>>>> +acceptance by the syscalls of any valid tagged pointer are not applicable
>>>> +to the following cases:
>>>> + - mmap() addr parameter.
>>>> + - mremap() new_address parameter.
>>>> + - prctl_set_mm() struct prctl_map fields.
>>>> + - prctl_set_mm_map() struct prctl_map fields.
>>>
>>> i don't understand the exception: does it mean that passing a tagged
>>> address to these syscalls is undefined?
>>
>> I'd say it's as undefined as it is right now without these patches. We
>> may be able to explain this better in the document.
>>
>
--
Regards,
Vincenzo
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox