All of lore.kernel.org
 help / color / mirror / Atom feed
From: Catalin Marinas <catalin.marinas@arm.com>
To: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
Cc: Mark Rutland <Mark.Rutland@arm.com>,
	Kate Stewart <kstewart@linuxfoundation.org>,
	"linux-doc@vger.kernel.org" <linux-doc@vger.kernel.org>,
	Will Deacon <Will.Deacon@arm.com>,
	Linux Memory Management List <linux-mm@kvack.org>,
	"linux-kselftest@vger.kernel.org"
	<linux-kselftest@vger.kernel.org>,
	Chintan Pandya <cpandya@codeaurora.org>,
	Shuah Khan <shuah@kernel.org>, Ingo Molnar <mingo@kernel.org>,
	"linux-arch@vger.kernel.org" <linux-arch@vger.kernel.org>,
	Jacob Bramley <Jacob.Bramley@arm.com>,
	Dmitry Vyukov <dvyukov@google.com>,
	Evgeniy Stepanov <eugenis@google.com>,
	Kees Cook <keescook@chromium.org>,
	Ruben Ayrapetyan <Ruben.Ayrapetyan@arm.com>,
	Andrey Konovalov <andreyknvl@google.com>,
	Ramana Radhakrishnan <ramana.radhakrishnan@arm.com>,
	Al Viro <viro@zeniv.linux.org.uk>, nd <nd@arm.com>,
	Linux ARM <linux-arm-kernel@lists.infradead.org>,
	Kostya Serebryany <kcc@google.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	LKML <linux-kernel@vger.kernel.org>,
	Lee Smith <Lee.Smith@arm.com>,
	Andrew Morton <akpm@linux-foundation.org>,
	Robin Murphy <Robin.Murphy@arm.com>,
	"Kirill A . Shutemov" <kirill.shutemov@linux.intel.com>
Subject: Re: [PATCH v4 0/7] arm64: untag user pointers passed to the kernel
Date: Thu, 28 Jun 2018 15:48:59 +0100	[thread overview]
Message-ID: <20180628144858.2fu7kq56cxhp2kpg@armageddon.cambridge.arm.com> (raw)
In-Reply-To: <20180628104610.czsnq4w3lfhxrn53@ltop.local>

On Thu, Jun 28, 2018 at 12:46:11PM +0200, Luc Van Oostenryck wrote:
> On Thu, Jun 28, 2018 at 11:27:42AM +0100, Catalin Marinas wrote:
> > On Thu, Jun 28, 2018 at 08:17:59AM +0200, Luc Van Oostenryck wrote:
> > > On Wed, Jun 27, 2018 at 06:17:58PM +0100, Catalin Marinas wrote:
> > > > sparse is indeed an option. The current implementation doesn't warn on
> > > > an explicit cast from (void __user *) to (unsigned long) since that's a
> > > > valid thing in the kernel. I couldn't figure out if there's any other
> > > > __attribute__ that could be used to warn of such conversion.
> > > 
> > > sparse doesn't have such attribute but would an new option that would warn
> > > on such cast be a solution for your case?
> > 
> > I can't tell for sure whether such sparse option would be the full
> > solution but detecting explicit __user pointer casts to long is a good
> > starting point. So far this patchset pretty much relies on detecting
> > a syscall failure and trying to figure out why, patching the kernel. It
> > doesn't really scale.
> 
> OK, I'll add such an option this evening.

That's great, thanks. I think this should cover casting pointers to any
integer types, not just "unsigned long" (e.g. long long).

The only downside is that with this patchset the untagging can be done
after the conversion to ulong (get_user_pages()) as that's where the
problem was noticed. With a new sparse feature, we'd have to annotate
the conversion sites (not sure how many until we run the tool though).

> > As a side note, we have cases in the user-kernel ABI where the user
> > address type is "unsigned long": mmap() and friends. My feedback on an
> > early version of this patchset was to always require untagged pointers
> > coming from user space on such syscalls, so no need for explicit
> > untagging.
> 
> Mmmm yes.
> I tend to favor a sort of opposite approach. When we have an address
> that must not be dereferenced as-such (and sometimes when the address
> can be from both __user & __kernel space) I prefer to use a ulong
> which will force the use of the required operation before being
> able to do any sort of dereferencing and this won't need horrible
> casts with __force (it, of course, all depends on the full context).

I agree. That's what the kernel uses in functions like get_user_pages()
which take ulong as an argument. Similarly mmap() and friends don't
expect the pointer to be dereferenced, hence the ulong argument. The
interesting part that the man page (and the C library header
declaration) shows such address argument as void *. We could add a
syscall wrapper in the arch code, only that it doesn't feel consistent
with the "rule" that ulong addresses are not actually tagged pointers.

-- 
Catalin

WARNING: multiple messages have this Message-ID
From: Catalin Marinas <catalin.marinas@arm.com>
To: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
Cc: Mark Rutland <Mark.Rutland@arm.com>,
	Kate Stewart <kstewart@linuxfoundation.org>,
	"linux-doc@vger.kernel.org" <linux-doc@vger.kernel.org>,
	Will Deacon <Will.Deacon@arm.com>,
	Linux Memory Management List <linux-mm@kvack.org>,
	"linux-kselftest@vger.kernel.org"
	<linux-kselftest@vger.kernel.org>,
	Chintan Pandya <cpandya@codeaurora.org>,
	Shuah Khan <shuah@kernel.org>, Ingo Molnar <mingo@kernel.org>,
	"linux-arch@vger.kernel.org" <linux-arch@vger.kernel.org>,
	Jacob Bramley <Jacob.Bramley@arm.com>,
	Dmitry Vyukov <dvyukov@google.com>,
	Evgeniy Stepanov <eugenis@google.com>,
	Kees Cook <keescook@chromium.org>,
	Ruben Ayrapetyan <Ruben.Ayrapetyan@arm.com>,
	Andrey Konovalov <andreyknvl@google.com>,
	Ramana Radhakrishnan <ramana.radhakrishnan@arm.com>,
	Al Viro <viro@zeniv.linux.org.uk>, nd <nd@arm.com>,
	Linux ARM <linux-arm-kernel@lists.infradead.org>,
	Kostya Serebryany <kcc@google.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	LKML <linux-kernel@vger.kernel.org>,
	Lee Smith <Lee.Smith@arm.com>,
	Andrew Morton <akpm@linux-foundation.org>,
	Robin Murphy <Robin.Murphy@arm.com>,
	"Kirill A . Shutemov" <kirill.shutemov@linux.intel.com>
Subject: Re: [PATCH v4 0/7] arm64: untag user pointers passed to the kernel
Date: Thu, 28 Jun 2018 15:48:59 +0100	[thread overview]
Message-ID: <20180628144858.2fu7kq56cxhp2kpg@armageddon.cambridge.arm.com> (raw)
In-Reply-To: <20180628104610.czsnq4w3lfhxrn53@ltop.local>

On Thu, Jun 28, 2018 at 12:46:11PM +0200, Luc Van Oostenryck wrote:
> On Thu, Jun 28, 2018 at 11:27:42AM +0100, Catalin Marinas wrote:
> > On Thu, Jun 28, 2018 at 08:17:59AM +0200, Luc Van Oostenryck wrote:
> > > On Wed, Jun 27, 2018 at 06:17:58PM +0100, Catalin Marinas wrote:
> > > > sparse is indeed an option. The current implementation doesn't warn on
> > > > an explicit cast from (void __user *) to (unsigned long) since that's a
> > > > valid thing in the kernel. I couldn't figure out if there's any other
> > > > __attribute__ that could be used to warn of such conversion.
> > > 
> > > sparse doesn't have such attribute but would an new option that would warn
> > > on such cast be a solution for your case?
> > 
> > I can't tell for sure whether such sparse option would be the full
> > solution but detecting explicit __user pointer casts to long is a good
> > starting point. So far this patchset pretty much relies on detecting
> > a syscall failure and trying to figure out why, patching the kernel. It
> > doesn't really scale.
> 
> OK, I'll add such an option this evening.

That's great, thanks. I think this should cover casting pointers to any
integer types, not just "unsigned long" (e.g. long long).

The only downside is that with this patchset the untagging can be done
after the conversion to ulong (get_user_pages()) as that's where the
problem was noticed. With a new sparse feature, we'd have to annotate
the conversion sites (not sure how many until we run the tool though).

> > As a side note, we have cases in the user-kernel ABI where the user
> > address type is "unsigned long": mmap() and friends. My feedback on an
> > early version of this patchset was to always require untagged pointers
> > coming from user space on such syscalls, so no need for explicit
> > untagging.
> 
> Mmmm yes.
> I tend to favor a sort of opposite approach. When we have an address
> that must not be dereferenced as-such (and sometimes when the address
> can be from both __user & __kernel space) I prefer to use a ulong
> which will force the use of the required operation before being
> able to do any sort of dereferencing and this won't need horrible
> casts with __force (it, of course, all depends on the full context).

I agree. That's what the kernel uses in functions like get_user_pages()
which take ulong as an argument. Similarly mmap() and friends don't
expect the pointer to be dereferenced, hence the ulong argument. The
interesting part that the man page (and the C library header
declaration) shows such address argument as void *. We could add a
syscall wrapper in the arch code, only that it doesn't feel consistent
with the "rule" that ulong addresses are not actually tagged pointers.

-- 
Catalin
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

WARNING: multiple messages have this Message-ID
From: catalin.marinas at arm.com (Catalin Marinas)
Subject: [PATCH v4 0/7] arm64: untag user pointers passed to the kernel
Date: Thu, 28 Jun 2018 15:48:59 +0100	[thread overview]
Message-ID: <20180628144858.2fu7kq56cxhp2kpg@armageddon.cambridge.arm.com> (raw)
In-Reply-To: <20180628104610.czsnq4w3lfhxrn53@ltop.local>

On Thu, Jun 28, 2018 at 12:46:11PM +0200, Luc Van Oostenryck wrote:
> On Thu, Jun 28, 2018 at 11:27:42AM +0100, Catalin Marinas wrote:
> > On Thu, Jun 28, 2018 at 08:17:59AM +0200, Luc Van Oostenryck wrote:
> > > On Wed, Jun 27, 2018 at 06:17:58PM +0100, Catalin Marinas wrote:
> > > > sparse is indeed an option. The current implementation doesn't warn on
> > > > an explicit cast from (void __user *) to (unsigned long) since that's a
> > > > valid thing in the kernel. I couldn't figure out if there's any other
> > > > __attribute__ that could be used to warn of such conversion.
> > > 
> > > sparse doesn't have such attribute but would an new option that would warn
> > > on such cast be a solution for your case?
> > 
> > I can't tell for sure whether such sparse option would be the full
> > solution but detecting explicit __user pointer casts to long is a good
> > starting point. So far this patchset pretty much relies on detecting
> > a syscall failure and trying to figure out why, patching the kernel. It
> > doesn't really scale.
> 
> OK, I'll add such an option this evening.

That's great, thanks. I think this should cover casting pointers to any
integer types, not just "unsigned long" (e.g. long long).

The only downside is that with this patchset the untagging can be done
after the conversion to ulong (get_user_pages()) as that's where the
problem was noticed. With a new sparse feature, we'd have to annotate
the conversion sites (not sure how many until we run the tool though).

> > As a side note, we have cases in the user-kernel ABI where the user
> > address type is "unsigned long": mmap() and friends. My feedback on an
> > early version of this patchset was to always require untagged pointers
> > coming from user space on such syscalls, so no need for explicit
> > untagging.
> 
> Mmmm yes.
> I tend to favor a sort of opposite approach. When we have an address
> that must not be dereferenced as-such (and sometimes when the address
> can be from both __user & __kernel space) I prefer to use a ulong
> which will force the use of the required operation before being
> able to do any sort of dereferencing and this won't need horrible
> casts with __force (it, of course, all depends on the full context).

I agree. That's what the kernel uses in functions like get_user_pages()
which take ulong as an argument. Similarly mmap() and friends don't
expect the pointer to be dereferenced, hence the ulong argument. The
interesting part that the man page (and the C library header
declaration) shows such address argument as void *. We could add a
syscall wrapper in the arch code, only that it doesn't feel consistent
with the "rule" that ulong addresses are not actually tagged pointers.

-- 
Catalin
--
To unsubscribe from this list: send the line "unsubscribe linux-kselftest" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

WARNING: multiple messages have this Message-ID
From: catalin.marinas@arm.com (Catalin Marinas)
Subject: [PATCH v4 0/7] arm64: untag user pointers passed to the kernel
Date: Thu, 28 Jun 2018 15:48:59 +0100	[thread overview]
Message-ID: <20180628144858.2fu7kq56cxhp2kpg@armageddon.cambridge.arm.com> (raw)
Message-ID: <20180628144859.ZpLd_kbdbbM1_s-BjXp_y8WOG86allTpb7gGeHecM8U@z> (raw)
In-Reply-To: <20180628104610.czsnq4w3lfhxrn53@ltop.local>

On Thu, Jun 28, 2018@12:46:11PM +0200, Luc Van Oostenryck wrote:
> On Thu, Jun 28, 2018@11:27:42AM +0100, Catalin Marinas wrote:
> > On Thu, Jun 28, 2018@08:17:59AM +0200, Luc Van Oostenryck wrote:
> > > On Wed, Jun 27, 2018@06:17:58PM +0100, Catalin Marinas wrote:
> > > > sparse is indeed an option. The current implementation doesn't warn on
> > > > an explicit cast from (void __user *) to (unsigned long) since that's a
> > > > valid thing in the kernel. I couldn't figure out if there's any other
> > > > __attribute__ that could be used to warn of such conversion.
> > > 
> > > sparse doesn't have such attribute but would an new option that would warn
> > > on such cast be a solution for your case?
> > 
> > I can't tell for sure whether such sparse option would be the full
> > solution but detecting explicit __user pointer casts to long is a good
> > starting point. So far this patchset pretty much relies on detecting
> > a syscall failure and trying to figure out why, patching the kernel. It
> > doesn't really scale.
> 
> OK, I'll add such an option this evening.

That's great, thanks. I think this should cover casting pointers to any
integer types, not just "unsigned long" (e.g. long long).

The only downside is that with this patchset the untagging can be done
after the conversion to ulong (get_user_pages()) as that's where the
problem was noticed. With a new sparse feature, we'd have to annotate
the conversion sites (not sure how many until we run the tool though).

> > As a side note, we have cases in the user-kernel ABI where the user
> > address type is "unsigned long": mmap() and friends. My feedback on an
> > early version of this patchset was to always require untagged pointers
> > coming from user space on such syscalls, so no need for explicit
> > untagging.
> 
> Mmmm yes.
> I tend to favor a sort of opposite approach. When we have an address
> that must not be dereferenced as-such (and sometimes when the address
> can be from both __user & __kernel space) I prefer to use a ulong
> which will force the use of the required operation before being
> able to do any sort of dereferencing and this won't need horrible
> casts with __force (it, of course, all depends on the full context).

I agree. That's what the kernel uses in functions like get_user_pages()
which take ulong as an argument. Similarly mmap() and friends don't
expect the pointer to be dereferenced, hence the ulong argument. The
interesting part that the man page (and the C library header
declaration) shows such address argument as void *. We could add a
syscall wrapper in the arch code, only that it doesn't feel consistent
with the "rule" that ulong addresses are not actually tagged pointers.

-- 
Catalin
--
To unsubscribe from this list: send the line "unsubscribe linux-kselftest" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

WARNING: multiple messages have this Message-ID
From: Catalin Marinas <catalin.marinas@arm.com>
To: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
Cc: Mark Rutland <Mark.Rutland@arm.com>,
	Kate Stewart <kstewart@linuxfoundation.org>,
	"linux-doc@vger.kernel.org" <linux-doc@vger.kernel.org>,
	Will Deacon <Will.Deacon@arm.com>,
	Linux Memory Management List <linux-mm@kvack.org>,
	"linux-kselftest@vger.kernel.org"
	<linux-kselftest@vger.kernel.org>,
	Chintan Pandya <cpandya@codeaurora.org>,
	Shuah Khan <shuah@kernel.org>, Ingo Molnar <mingo@kernel.org>,
	"linux-arch@vger.kernel.org" <linux-arch@vger.kernel.org>,
	Jacob Bramley <Jacob.Bramley@arm.com>,
	Dmitry Vyukov <dvyukov@google.com>,
	Evgeniy Stepanov <eugenis@google.com>,
	Kees Cook <keescook@chromium.org>,
	Ruben Ayrapetyan <Ruben.Ayrapetyan@arm.com>,
	Andrey Konovalov <andreyknvl@google.com>,
	Ramana Radhakrishnan <ramana.radhakrishnan@arm.com>,
	Al Viro <viro@zeniv.li>
Subject: Re: [PATCH v4 0/7] arm64: untag user pointers passed to the kernel
Date: Thu, 28 Jun 2018 15:48:59 +0100	[thread overview]
Message-ID: <20180628144858.2fu7kq56cxhp2kpg@armageddon.cambridge.arm.com> (raw)
In-Reply-To: <20180628104610.czsnq4w3lfhxrn53@ltop.local>

On Thu, Jun 28, 2018 at 12:46:11PM +0200, Luc Van Oostenryck wrote:
> On Thu, Jun 28, 2018 at 11:27:42AM +0100, Catalin Marinas wrote:
> > On Thu, Jun 28, 2018 at 08:17:59AM +0200, Luc Van Oostenryck wrote:
> > > On Wed, Jun 27, 2018 at 06:17:58PM +0100, Catalin Marinas wrote:
> > > > sparse is indeed an option. The current implementation doesn't warn on
> > > > an explicit cast from (void __user *) to (unsigned long) since that's a
> > > > valid thing in the kernel. I couldn't figure out if there's any other
> > > > __attribute__ that could be used to warn of such conversion.
> > > 
> > > sparse doesn't have such attribute but would an new option that would warn
> > > on such cast be a solution for your case?
> > 
> > I can't tell for sure whether such sparse option would be the full
> > solution but detecting explicit __user pointer casts to long is a good
> > starting point. So far this patchset pretty much relies on detecting
> > a syscall failure and trying to figure out why, patching the kernel. It
> > doesn't really scale.
> 
> OK, I'll add such an option this evening.

That's great, thanks. I think this should cover casting pointers to any
integer types, not just "unsigned long" (e.g. long long).

The only downside is that with this patchset the untagging can be done
after the conversion to ulong (get_user_pages()) as that's where the
problem was noticed. With a new sparse feature, we'd have to annotate
the conversion sites (not sure how many until we run the tool though).

> > As a side note, we have cases in the user-kernel ABI where the user
> > address type is "unsigned long": mmap() and friends. My feedback on an
> > early version of this patchset was to always require untagged pointers
> > coming from user space on such syscalls, so no need for explicit
> > untagging.
> 
> Mmmm yes.
> I tend to favor a sort of opposite approach. When we have an address
> that must not be dereferenced as-such (and sometimes when the address
> can be from both __user & __kernel space) I prefer to use a ulong
> which will force the use of the required operation before being
> able to do any sort of dereferencing and this won't need horrible
> casts with __force (it, of course, all depends on the full context).

I agree. That's what the kernel uses in functions like get_user_pages()
which take ulong as an argument. Similarly mmap() and friends don't
expect the pointer to be dereferenced, hence the ulong argument. The
interesting part that the man page (and the C library header
declaration) shows such address argument as void *. We could add a
syscall wrapper in the arch code, only that it doesn't feel consistent
with the "rule" that ulong addresses are not actually tagged pointers.

-- 
Catalin

WARNING: multiple messages have this Message-ID
From: Catalin Marinas <catalin.marinas@arm.com>
To: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
Cc: Mark Rutland <Mark.Rutland@arm.com>,
	Kate Stewart <kstewart@linuxfoundation.org>,
	"linux-doc@vger.kernel.org" <linux-doc@vger.kernel.org>,
	Will Deacon <Will.Deacon@arm.com>,
	Linux Memory Management List <linux-mm@kvack.org>,
	"linux-kselftest@vger.kernel.org"
	<linux-kselftest@vger.kernel.org>,
	Chintan Pandya <cpandya@codeaurora.org>,
	Shuah Khan <shuah@kernel.org>, Ingo Molnar <mingo@kernel.org>,
	"linux-arch@vger.kernel.org" <linux-arch@vger.kernel.org>,
	Jacob Bramley <Jacob.Bramley@arm.com>,
	Dmitry Vyukov <dvyukov@google.com>,
	Evgeniy Stepanov <eugenis@google.com>,
	Kees Cook <keescook@chromium.org>,
	Ruben Ayrapetyan <Ruben.Ayrapetyan@arm.com>,
	Andrey Konovalov <andreyknvl@google.com>,
	Ramana Radhakrishnan <ramana.radhakrishnan@arm.com>,
	Al Viro <viro@zeniv.linux.org.uk>nd <nd@arm.com>,
	Linux ARM <linux-arm-kernel@lists.infradead.org>,
	Kostya Serebryany <kcc@google.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	LKML <linux-kernel@vger.kernel.org>,
	Lee Smith <Lee.Smith@arm.com>,
	Andrew Morton <akpm@linux-foundation.org>,
	Robin Murphy <Robin.Murphy@arm.com>,
	"Kirill A . Shutemov" <kirill.shutemov@linux.intel.com>
Subject: Re: [PATCH v4 0/7] arm64: untag user pointers passed to the kernel
Date: Thu, 28 Jun 2018 15:48:59 +0100	[thread overview]
Message-ID: <20180628144858.2fu7kq56cxhp2kpg@armageddon.cambridge.arm.com> (raw)
Message-ID: <20180628144859.tPcvqUevQ8or-ztVp7m9MLi7NwPtc2kR2tvcH51uK5E@z> (raw)
In-Reply-To: <20180628104610.czsnq4w3lfhxrn53@ltop.local>

On Thu, Jun 28, 2018 at 12:46:11PM +0200, Luc Van Oostenryck wrote:
> On Thu, Jun 28, 2018 at 11:27:42AM +0100, Catalin Marinas wrote:
> > On Thu, Jun 28, 2018 at 08:17:59AM +0200, Luc Van Oostenryck wrote:
> > > On Wed, Jun 27, 2018 at 06:17:58PM +0100, Catalin Marinas wrote:
> > > > sparse is indeed an option. The current implementation doesn't warn on
> > > > an explicit cast from (void __user *) to (unsigned long) since that's a
> > > > valid thing in the kernel. I couldn't figure out if there's any other
> > > > __attribute__ that could be used to warn of such conversion.
> > > 
> > > sparse doesn't have such attribute but would an new option that would warn
> > > on such cast be a solution for your case?
> > 
> > I can't tell for sure whether such sparse option would be the full
> > solution but detecting explicit __user pointer casts to long is a good
> > starting point. So far this patchset pretty much relies on detecting
> > a syscall failure and trying to figure out why, patching the kernel. It
> > doesn't really scale.
> 
> OK, I'll add such an option this evening.

That's great, thanks. I think this should cover casting pointers to any
integer types, not just "unsigned long" (e.g. long long).

The only downside is that with this patchset the untagging can be done
after the conversion to ulong (get_user_pages()) as that's where the
problem was noticed. With a new sparse feature, we'd have to annotate
the conversion sites (not sure how many until we run the tool though).

> > As a side note, we have cases in the user-kernel ABI where the user
> > address type is "unsigned long": mmap() and friends. My feedback on an
> > early version of this patchset was to always require untagged pointers
> > coming from user space on such syscalls, so no need for explicit
> > untagging.
> 
> Mmmm yes.
> I tend to favor a sort of opposite approach. When we have an address
> that must not be dereferenced as-such (and sometimes when the address
> can be from both __user & __kernel space) I prefer to use a ulong
> which will force the use of the required operation before being
> able to do any sort of dereferencing and this won't need horrible
> casts with __force (it, of course, all depends on the full context).

I agree. That's what the kernel uses in functions like get_user_pages()
which take ulong as an argument. Similarly mmap() and friends don't
expect the pointer to be dereferenced, hence the ulong argument. The
interesting part that the man page (and the C library header
declaration) shows such address argument as void *. We could add a
syscall wrapper in the arch code, only that it doesn't feel consistent
with the "rule" that ulong addresses are not actually tagged pointers.

-- 
Catalin

WARNING: multiple messages have this Message-ID
From: catalin.marinas@arm.com (Catalin Marinas)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH v4 0/7] arm64: untag user pointers passed to the kernel
Date: Thu, 28 Jun 2018 15:48:59 +0100	[thread overview]
Message-ID: <20180628144858.2fu7kq56cxhp2kpg@armageddon.cambridge.arm.com> (raw)
In-Reply-To: <20180628104610.czsnq4w3lfhxrn53@ltop.local>

On Thu, Jun 28, 2018 at 12:46:11PM +0200, Luc Van Oostenryck wrote:
> On Thu, Jun 28, 2018 at 11:27:42AM +0100, Catalin Marinas wrote:
> > On Thu, Jun 28, 2018 at 08:17:59AM +0200, Luc Van Oostenryck wrote:
> > > On Wed, Jun 27, 2018 at 06:17:58PM +0100, Catalin Marinas wrote:
> > > > sparse is indeed an option. The current implementation doesn't warn on
> > > > an explicit cast from (void __user *) to (unsigned long) since that's a
> > > > valid thing in the kernel. I couldn't figure out if there's any other
> > > > __attribute__ that could be used to warn of such conversion.
> > > 
> > > sparse doesn't have such attribute but would an new option that would warn
> > > on such cast be a solution for your case?
> > 
> > I can't tell for sure whether such sparse option would be the full
> > solution but detecting explicit __user pointer casts to long is a good
> > starting point. So far this patchset pretty much relies on detecting
> > a syscall failure and trying to figure out why, patching the kernel. It
> > doesn't really scale.
> 
> OK, I'll add such an option this evening.

That's great, thanks. I think this should cover casting pointers to any
integer types, not just "unsigned long" (e.g. long long).

The only downside is that with this patchset the untagging can be done
after the conversion to ulong (get_user_pages()) as that's where the
problem was noticed. With a new sparse feature, we'd have to annotate
the conversion sites (not sure how many until we run the tool though).

> > As a side note, we have cases in the user-kernel ABI where the user
> > address type is "unsigned long": mmap() and friends. My feedback on an
> > early version of this patchset was to always require untagged pointers
> > coming from user space on such syscalls, so no need for explicit
> > untagging.
> 
> Mmmm yes.
> I tend to favor a sort of opposite approach. When we have an address
> that must not be dereferenced as-such (and sometimes when the address
> can be from both __user & __kernel space) I prefer to use a ulong
> which will force the use of the required operation before being
> able to do any sort of dereferencing and this won't need horrible
> casts with __force (it, of course, all depends on the full context).

I agree. That's what the kernel uses in functions like get_user_pages()
which take ulong as an argument. Similarly mmap() and friends don't
expect the pointer to be dereferenced, hence the ulong argument. The
interesting part that the man page (and the C library header
declaration) shows such address argument as void *. We could add a
syscall wrapper in the arch code, only that it doesn't feel consistent
with the "rule" that ulong addresses are not actually tagged pointers.

-- 
Catalin

  reply	other threads:[~2018-06-28 15:13 UTC|newest]

Thread overview: 195+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-06-20 15:24 Andrey Konovalov
2018-06-20 15:24 ` Andrey Konovalov
2018-06-20 15:24 ` Andrey Konovalov
2018-06-20 15:24 ` Andrey Konovalov
2018-06-20 15:24 ` Andrey Konovalov
2018-06-20 15:24 ` andreyknvl
2018-06-20 15:24 ` Andrey Konovalov
2018-06-20 15:24 ` [PATCH v4 1/7] arm64: add type casts to untagged_addr macro Andrey Konovalov
2018-06-20 15:24   ` Andrey Konovalov
2018-06-20 15:24   ` Andrey Konovalov
2018-06-20 15:24   ` Andrey Konovalov
2018-06-20 15:24   ` andreyknvl
2018-06-20 15:24   ` Andrey Konovalov
2018-06-20 15:24 ` [PATCH v4 2/7] uaccess: add untagged_addr definition for other arches Andrey Konovalov
2018-06-20 15:24   ` Andrey Konovalov
2018-06-20 15:24   ` Andrey Konovalov
2018-06-20 15:24   ` andreyknvl
2018-06-20 15:24   ` Andrey Konovalov
2018-06-20 15:24 ` [PATCH v4 3/7] arm64: untag user addresses in access_ok and __uaccess_mask_ptr Andrey Konovalov
2018-06-20 15:24   ` Andrey Konovalov
2018-06-20 15:24   ` Andrey Konovalov
2018-06-20 15:24   ` andreyknvl
2018-06-20 15:24   ` Andrey Konovalov
2018-06-20 15:24 ` [PATCH v4 4/7] mm, arm64: untag user addresses in mm/gup.c Andrey Konovalov
2018-06-20 15:24   ` Andrey Konovalov
2018-06-20 15:24   ` Andrey Konovalov
2018-06-20 15:24   ` andreyknvl
2018-06-20 15:24   ` Andrey Konovalov
2018-06-20 15:24 ` [PATCH v4 5/7] lib, arm64: untag addrs passed to strncpy_from_user and strnlen_user Andrey Konovalov
2018-06-20 15:24   ` Andrey Konovalov
2018-06-20 15:24   ` Andrey Konovalov
2018-06-20 15:24   ` andreyknvl
2018-06-20 15:24   ` Andrey Konovalov
2018-06-20 15:24 ` [PATCH v4 6/7] arm64: update Documentation/arm64/tagged-pointers.txt Andrey Konovalov
2018-06-20 15:24   ` Andrey Konovalov
2018-06-20 15:24   ` Andrey Konovalov
2018-06-20 15:24   ` andreyknvl
2018-06-20 15:24   ` Andrey Konovalov
2018-06-20 15:24 ` [PATCH v4 7/7] selftests, arm64: add a selftest for passing tagged pointers to kernel Andrey Konovalov
2018-06-20 15:24   ` Andrey Konovalov
2018-06-20 15:24   ` Andrey Konovalov
2018-06-20 15:24   ` andreyknvl
2018-06-20 15:24   ` Andrey Konovalov
2018-06-26 12:47 ` [PATCH v4 0/7] arm64: untag user pointers passed to the kernel Andrey Konovalov
2018-06-26 12:47   ` Andrey Konovalov
2018-06-26 12:47   ` Andrey Konovalov
2018-06-26 12:47   ` andreyknvl
2018-06-26 12:47   ` Andrey Konovalov
2018-06-26 17:29   ` Catalin Marinas
2018-06-26 17:29     ` Catalin Marinas
2018-06-26 17:29     ` Catalin Marinas
2018-06-26 17:29     ` Catalin Marinas
2018-06-26 17:29     ` catalin.marinas
2018-06-26 17:29     ` Catalin Marinas
2018-06-27 15:05     ` Andrey Konovalov
2018-06-27 15:05       ` Andrey Konovalov
2018-06-27 15:05       ` Andrey Konovalov
2018-06-27 15:05       ` Andrey Konovalov
2018-06-27 15:05       ` andreyknvl
2018-06-27 15:05       ` Andrey Konovalov
2018-06-27 15:08       ` Ramana Radhakrishnan
2018-06-27 15:08         ` Ramana Radhakrishnan
2018-06-27 15:08         ` Ramana Radhakrishnan
2018-06-27 15:08         ` Ramana Radhakrishnan
2018-06-27 15:08         ` Ramana Radhakrishnan
2018-06-27 15:08         ` ramana.radhakrishnan
2018-06-27 15:08         ` Ramana Radhakrishnan
2018-06-27 17:17         ` Catalin Marinas
2018-06-27 17:17           ` Catalin Marinas
2018-06-27 17:17           ` Catalin Marinas
2018-06-27 17:17           ` Catalin Marinas
2018-06-27 17:17           ` Catalin Marinas
2018-06-27 17:17           ` catalin.marinas
2018-06-27 17:17           ` Catalin Marinas
2018-06-28  6:17           ` Luc Van Oostenryck
2018-06-28  6:17             ` Luc Van Oostenryck
2018-06-28  6:17             ` Luc Van Oostenryck
2018-06-28  6:17             ` Luc Van Oostenryck
2018-06-28  6:17             ` Luc Van Oostenryck
2018-06-28  6:17             ` luc.vanoostenryck
2018-06-28  6:17             ` Luc Van Oostenryck
2018-06-28 10:27             ` Catalin Marinas
2018-06-28 10:27               ` Catalin Marinas
2018-06-28 10:27               ` Catalin Marinas
2018-06-28 10:27               ` Catalin Marinas
2018-06-28 10:27               ` Catalin Marinas
2018-06-28 10:27               ` catalin.marinas
2018-06-28 10:27               ` Catalin Marinas
2018-06-28 10:46               ` Luc Van Oostenryck
2018-06-28 10:46                 ` Luc Van Oostenryck
2018-06-28 10:46                 ` Luc Van Oostenryck
2018-06-28 10:46                 ` Luc Van Oostenryck
2018-06-28 10:46                 ` Luc Van Oostenryck
2018-06-28 10:46                 ` luc.vanoostenryck
2018-06-28 10:46                 ` Luc Van Oostenryck
2018-06-28 14:48                 ` Catalin Marinas [this message]
2018-06-28 14:48                   ` Catalin Marinas
2018-06-28 14:48                   ` Catalin Marinas
2018-06-28 14:48                   ` Catalin Marinas
2018-06-28 14:48                   ` Catalin Marinas
2018-06-28 14:48                   ` catalin.marinas
2018-06-28 14:48                   ` Catalin Marinas
2018-06-28 15:28                   ` Luc Van Oostenryck
2018-06-28 15:28                     ` Luc Van Oostenryck
2018-06-28 15:28                     ` Luc Van Oostenryck
2018-06-28 15:28                     ` Luc Van Oostenryck
2018-06-28 15:28                     ` Luc Van Oostenryck
2018-06-28 15:28                     ` luc.vanoostenryck
2018-06-28 15:28                     ` Luc Van Oostenryck
2018-06-29 15:27                   ` David Laight
2018-06-29 15:27                     ` David Laight
2018-06-29 15:27                     ` David Laight
2018-06-29 15:27                     ` David Laight
2018-06-29 15:27                     ` David Laight
2018-06-29 15:27                     ` David.Laight
2018-06-29 15:27                     ` David Laight
2018-06-28 23:21               ` [PATCH] sparse: stricter warning for explicit cast to ulong Luc Van Oostenryck
2018-06-28 23:21                 ` Luc Van Oostenryck
2018-06-28 23:21                 ` Luc Van Oostenryck
2018-06-28 23:21                 ` Luc Van Oostenryck
2018-06-28 23:21                 ` luc.vanoostenryck
2018-06-28 23:21                 ` Luc Van Oostenryck
2018-06-28 23:21                 ` Luc Van Oostenryck
2018-06-28 19:30       ` [PATCH v4 0/7] arm64: untag user pointers passed to the kernel Andrey Konovalov
2018-06-28 19:30         ` Andrey Konovalov
2018-06-28 19:30         ` Andrey Konovalov
2018-06-28 19:30         ` Andrey Konovalov
2018-06-28 19:30         ` andreyknvl
2018-06-28 19:30         ` Andrey Konovalov
2018-06-29 15:19         ` Andrey Konovalov
2018-06-29 15:19           ` Andrey Konovalov
2018-06-29 15:19           ` Andrey Konovalov
2018-06-29 15:19           ` Andrey Konovalov
2018-06-29 15:19           ` andreyknvl
2018-06-29 15:19           ` Andrey Konovalov
2018-06-29 15:20           ` Andrey Konovalov
2018-06-29 15:20             ` Andrey Konovalov
2018-06-29 15:20             ` Andrey Konovalov
2018-06-29 15:20             ` Andrey Konovalov
2018-06-29 15:20             ` andreyknvl
2018-06-29 15:20             ` Andrey Konovalov
2018-07-16 11:25         ` Andrey Konovalov
2018-07-16 11:25           ` Andrey Konovalov
2018-07-16 11:25           ` Andrey Konovalov
2018-07-16 11:25           ` Andrey Konovalov
2018-07-16 11:25           ` andreyknvl
2018-07-16 11:25           ` Andrey Konovalov
2018-07-31 13:23           ` Andrey Konovalov
2018-07-31 13:23             ` Andrey Konovalov
2018-07-31 13:23             ` Andrey Konovalov
2018-07-31 13:23             ` Andrey Konovalov
2018-07-31 13:23             ` andreyknvl
2018-07-31 13:23             ` Andrey Konovalov
2018-08-01 17:42           ` Catalin Marinas
2018-08-01 17:42             ` Catalin Marinas
2018-08-01 17:42             ` Catalin Marinas
2018-08-01 17:42             ` Catalin Marinas
2018-08-01 17:42             ` catalin.marinas
2018-08-01 17:42             ` Catalin Marinas
2018-08-02 15:00             ` Andrey Konovalov
2018-08-02 15:00               ` Andrey Konovalov
2018-08-02 15:00               ` Andrey Konovalov
2018-08-02 15:00               ` Andrey Konovalov
2018-08-02 15:00               ` andreyknvl
2018-08-02 15:00               ` Andrey Konovalov
2018-08-03 14:59               ` Andrey Konovalov
2018-08-03 14:59                 ` Andrey Konovalov
2018-08-03 14:59                 ` Andrey Konovalov
2018-08-03 14:59                 ` Andrey Konovalov
2018-08-03 14:59                 ` andreyknvl
2018-08-03 14:59                 ` Andrey Konovalov
2018-08-03 15:09                 ` Greg Kroah-Hartman
2018-08-03 15:09                   ` Greg Kroah-Hartman
2018-08-03 15:09                   ` Greg Kroah-Hartman
2018-08-03 15:09                   ` Greg Kroah-Hartman
2018-08-03 15:09                   ` gregkh
2018-08-03 15:09                   ` Greg Kroah-Hartman
2018-08-03 16:43                   ` Matthew Wilcox
2018-08-03 16:43                     ` Matthew Wilcox
2018-08-03 16:43                     ` Matthew Wilcox
2018-08-03 16:43                     ` Matthew Wilcox
2018-08-03 16:43                     ` willy
2018-08-03 16:43                     ` Matthew Wilcox
2018-08-03 16:54                     ` Andrey Konovalov
2018-08-03 16:54                       ` Andrey Konovalov
2018-08-03 16:54                       ` Andrey Konovalov
2018-08-03 16:54                       ` Andrey Konovalov
2018-08-03 16:54                       ` andreyknvl
2018-08-03 16:54                       ` Andrey Konovalov
2018-08-06 19:12                   ` Luc Van Oostenryck
2018-08-06 19:12                     ` Luc Van Oostenryck
2018-08-06 19:12                     ` Luc Van Oostenryck
2018-08-06 19:12                     ` Luc Van Oostenryck
2018-08-06 19:12                     ` luc.vanoostenryck
2018-08-06 19:12                     ` Luc Van Oostenryck

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20180628144858.2fu7kq56cxhp2kpg@armageddon.cambridge.arm.com \
    --to=catalin.marinas@arm.com \
    --cc=Jacob.Bramley@arm.com \
    --cc=Lee.Smith@arm.com \
    --cc=Mark.Rutland@arm.com \
    --cc=Robin.Murphy@arm.com \
    --cc=Ruben.Ayrapetyan@arm.com \
    --cc=Will.Deacon@arm.com \
    --cc=akpm@linux-foundation.org \
    --cc=andreyknvl@google.com \
    --cc=cpandya@codeaurora.org \
    --cc=dvyukov@google.com \
    --cc=eugenis@google.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=kcc@google.com \
    --cc=keescook@chromium.org \
    --cc=kirill.shutemov@linux.intel.com \
    --cc=kstewart@linuxfoundation.org \
    --cc=linux-arch@vger.kernel.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-doc@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-kselftest@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=luc.vanoostenryck@gmail.com \
    --cc=mingo@kernel.org \
    --cc=nd@arm.com \
    --cc=ramana.radhakrishnan@arm.com \
    --cc=shuah@kernel.org \
    --cc=viro@zeniv.linux.org.uk \
    --subject='Re: [PATCH v4 0/7] arm64: untag user pointers passed to the kernel' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.