diff mbox series

lsm: make security_socket_getpeersec_stream() sockptr_t safe

Message ID 166543910984.474337.2779830480340611497.stgit@olly (mailing list archive)
State Handled Elsewhere
Delegated to: Paul Moore
Headers show
Series lsm: make security_socket_getpeersec_stream() sockptr_t safe | expand

Commit Message

Paul Moore Oct. 10, 2022, 9:58 p.m. UTC
Commit 4ff09db1b79b ("bpf: net: Change sk_getsockopt() to take the
sockptr_t argument") made it possible to call sk_getsockopt()
with both user and kernel address space buffers through the use of
the sockptr_t type.  Unfortunately at the time of conversion the
security_socket_getpeersec_stream() LSM hook was written to only
accept userspace buffers, and in a desire to avoid having to change
the LSM hook the commit author simply passed the sockptr_t's
userspace buffer pointer.  Since the only sk_getsockopt() callers
at the time of conversion which used kernel sockptr_t buffers did
not allow SO_PEERSEC, and hence the
security_socket_getpeersec_stream() hook, this was acceptable but
also very fragile as future changes presented the possibility of
silently passing kernel space pointers to the LSM hook.

There are several ways to protect against this, including careful
code review of future commits, but since relying on code review to
catch bugs is a recipe for disaster and the upstream eBPF maintainer
is "strongly against defensive programming", this patch updates the
LSM hook, and all of the implementations to support sockptr_t and
safely handle both user and kernel space buffers.

Signed-off-by: Paul Moore <paul@paul-moore.com>
---
 include/linux/lsm_hook_defs.h |    2 +-
 include/linux/lsm_hooks.h     |    4 ++--
 include/linux/security.h      |   11 +++++++----
 net/core/sock.c               |    3 ++-
 security/apparmor/lsm.c       |   29 +++++++++++++----------------
 security/security.c           |    6 +++---
 security/selinux/hooks.c      |   13 ++++++-------
 security/smack/smack_lsm.c    |   19 ++++++++++---------
 8 files changed, 44 insertions(+), 43 deletions(-)

Comments

Paul Moore Oct. 10, 2022, 10 p.m. UTC | #1
On Mon, Oct 10, 2022 at 5:58 PM Paul Moore <paul@paul-moore.com> wrote:
>
> Commit 4ff09db1b79b ("bpf: net: Change sk_getsockopt() to take the
> sockptr_t argument") made it possible to call sk_getsockopt()
> with both user and kernel address space buffers through the use of
> the sockptr_t type.  Unfortunately at the time of conversion the
> security_socket_getpeersec_stream() LSM hook was written to only
> accept userspace buffers, and in a desire to avoid having to change
> the LSM hook the commit author simply passed the sockptr_t's
> userspace buffer pointer.  Since the only sk_getsockopt() callers
> at the time of conversion which used kernel sockptr_t buffers did
> not allow SO_PEERSEC, and hence the
> security_socket_getpeersec_stream() hook, this was acceptable but
> also very fragile as future changes presented the possibility of
> silently passing kernel space pointers to the LSM hook.
>
> There are several ways to protect against this, including careful
> code review of future commits, but since relying on code review to
> catch bugs is a recipe for disaster and the upstream eBPF maintainer
> is "strongly against defensive programming", this patch updates the
> LSM hook, and all of the implementations to support sockptr_t and
> safely handle both user and kernel space buffers.
>
> Signed-off-by: Paul Moore <paul@paul-moore.com>
> ---
>  include/linux/lsm_hook_defs.h |    2 +-
>  include/linux/lsm_hooks.h     |    4 ++--
>  include/linux/security.h      |   11 +++++++----
>  net/core/sock.c               |    3 ++-
>  security/apparmor/lsm.c       |   29 +++++++++++++----------------
>  security/security.c           |    6 +++---
>  security/selinux/hooks.c      |   13 ++++++-------
>  security/smack/smack_lsm.c    |   19 ++++++++++---------
>  8 files changed, 44 insertions(+), 43 deletions(-)

Casey and John, could you please look over the Smack and AppArmor bits
of this patch when you get a chance?  I did my best on the conversion,
but I would appreciate a review by the experts :)
Casey Schaufler Oct. 10, 2022, 10:37 p.m. UTC | #2
On 10/10/2022 3:00 PM, Paul Moore wrote:
> On Mon, Oct 10, 2022 at 5:58 PM Paul Moore <paul@paul-moore.com> wrote:
>> Commit 4ff09db1b79b ("bpf: net: Change sk_getsockopt() to take the
>> sockptr_t argument") made it possible to call sk_getsockopt()
>> with both user and kernel address space buffers through the use of
>> the sockptr_t type.  Unfortunately at the time of conversion the
>> security_socket_getpeersec_stream() LSM hook was written to only
>> accept userspace buffers, and in a desire to avoid having to change
>> the LSM hook the commit author simply passed the sockptr_t's
>> userspace buffer pointer.  Since the only sk_getsockopt() callers
>> at the time of conversion which used kernel sockptr_t buffers did
>> not allow SO_PEERSEC, and hence the
>> security_socket_getpeersec_stream() hook, this was acceptable but
>> also very fragile as future changes presented the possibility of
>> silently passing kernel space pointers to the LSM hook.
>>
>> There are several ways to protect against this, including careful
>> code review of future commits, but since relying on code review to
>> catch bugs is a recipe for disaster and the upstream eBPF maintainer
>> is "strongly against defensive programming", this patch updates the
>> LSM hook, and all of the implementations to support sockptr_t and
>> safely handle both user and kernel space buffers.
>>
>> Signed-off-by: Paul Moore <paul@paul-moore.com>
>> ---
>>  include/linux/lsm_hook_defs.h |    2 +-
>>  include/linux/lsm_hooks.h     |    4 ++--
>>  include/linux/security.h      |   11 +++++++----
>>  net/core/sock.c               |    3 ++-
>>  security/apparmor/lsm.c       |   29 +++++++++++++----------------
>>  security/security.c           |    6 +++---
>>  security/selinux/hooks.c      |   13 ++++++-------
>>  security/smack/smack_lsm.c    |   19 ++++++++++---------
>>  8 files changed, 44 insertions(+), 43 deletions(-)
> Casey and John, could you please look over the Smack and AppArmor bits
> of this patch when you get a chance?  I did my best on the conversion,
> but I would appreciate a review by the experts :)

I'm off the grid until 10/20, but will add this to my do-asap stack.
Paul Moore Oct. 10, 2022, 10:44 p.m. UTC | #3
On Mon, Oct 10, 2022 at 6:39 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> On 10/10/2022 3:00 PM, Paul Moore wrote:
> > On Mon, Oct 10, 2022 at 5:58 PM Paul Moore <paul@paul-moore.com> wrote:
> >> Commit 4ff09db1b79b ("bpf: net: Change sk_getsockopt() to take the
> >> sockptr_t argument") made it possible to call sk_getsockopt()
> >> with both user and kernel address space buffers through the use of
> >> the sockptr_t type.  Unfortunately at the time of conversion the
> >> security_socket_getpeersec_stream() LSM hook was written to only
> >> accept userspace buffers, and in a desire to avoid having to change
> >> the LSM hook the commit author simply passed the sockptr_t's
> >> userspace buffer pointer.  Since the only sk_getsockopt() callers
> >> at the time of conversion which used kernel sockptr_t buffers did
> >> not allow SO_PEERSEC, and hence the
> >> security_socket_getpeersec_stream() hook, this was acceptable but
> >> also very fragile as future changes presented the possibility of
> >> silently passing kernel space pointers to the LSM hook.
> >>
> >> There are several ways to protect against this, including careful
> >> code review of future commits, but since relying on code review to
> >> catch bugs is a recipe for disaster and the upstream eBPF maintainer
> >> is "strongly against defensive programming", this patch updates the
> >> LSM hook, and all of the implementations to support sockptr_t and
> >> safely handle both user and kernel space buffers.
> >>
> >> Signed-off-by: Paul Moore <paul@paul-moore.com>
> >> ---
> >>  include/linux/lsm_hook_defs.h |    2 +-
> >>  include/linux/lsm_hooks.h     |    4 ++--
> >>  include/linux/security.h      |   11 +++++++----
> >>  net/core/sock.c               |    3 ++-
> >>  security/apparmor/lsm.c       |   29 +++++++++++++----------------
> >>  security/security.c           |    6 +++---
> >>  security/selinux/hooks.c      |   13 ++++++-------
> >>  security/smack/smack_lsm.c    |   19 ++++++++++---------
> >>  8 files changed, 44 insertions(+), 43 deletions(-)
> > Casey and John, could you please look over the Smack and AppArmor bits
> > of this patch when you get a chance?  I did my best on the conversion,
> > but I would appreciate a review by the experts :)
>
> I'm off the grid until 10/20, but will add this to my do-asap stack.

No worries, this is linux-next stuff anyway so no real rush at this
point.  Enjoy the time away.
Jakub Kicinski Oct. 13, 2022, 3:53 p.m. UTC | #4
On Mon, 10 Oct 2022 17:58:29 -0400 Paul Moore wrote:
> Commit 4ff09db1b79b ("bpf: net: Change sk_getsockopt() to take the
> sockptr_t argument") made it possible to call sk_getsockopt()
> with both user and kernel address space buffers through the use of
> the sockptr_t type.  Unfortunately at the time of conversion the
> security_socket_getpeersec_stream() LSM hook was written to only
> accept userspace buffers, and in a desire to avoid having to change
> the LSM hook the commit author simply passed the sockptr_t's
> userspace buffer pointer.  Since the only sk_getsockopt() callers
> at the time of conversion which used kernel sockptr_t buffers did
> not allow SO_PEERSEC, and hence the
> security_socket_getpeersec_stream() hook, this was acceptable but
> also very fragile as future changes presented the possibility of
> silently passing kernel space pointers to the LSM hook.
> 
> There are several ways to protect against this, including careful
> code review of future commits, but since relying on code review to
> catch bugs is a recipe for disaster and the upstream eBPF maintainer
> is "strongly against defensive programming", this patch updates the
> LSM hook, and all of the implementations to support sockptr_t and
> safely handle both user and kernel space buffers.

Code seems sane, FWIW, but the commit message sounds petty,
which is likely why nobody is willing to ack it.
Paul Moore Oct. 13, 2022, 3:59 p.m. UTC | #5
On Thu, Oct 13, 2022 at 11:53 AM Jakub Kicinski <kuba@kernel.org> wrote:
> On Mon, 10 Oct 2022 17:58:29 -0400 Paul Moore wrote:
> > Commit 4ff09db1b79b ("bpf: net: Change sk_getsockopt() to take the
> > sockptr_t argument") made it possible to call sk_getsockopt()
> > with both user and kernel address space buffers through the use of
> > the sockptr_t type.  Unfortunately at the time of conversion the
> > security_socket_getpeersec_stream() LSM hook was written to only
> > accept userspace buffers, and in a desire to avoid having to change
> > the LSM hook the commit author simply passed the sockptr_t's
> > userspace buffer pointer.  Since the only sk_getsockopt() callers
> > at the time of conversion which used kernel sockptr_t buffers did
> > not allow SO_PEERSEC, and hence the
> > security_socket_getpeersec_stream() hook, this was acceptable but
> > also very fragile as future changes presented the possibility of
> > silently passing kernel space pointers to the LSM hook.
> >
> > There are several ways to protect against this, including careful
> > code review of future commits, but since relying on code review to
> > catch bugs is a recipe for disaster and the upstream eBPF maintainer
> > is "strongly against defensive programming", this patch updates the
> > LSM hook, and all of the implementations to support sockptr_t and
> > safely handle both user and kernel space buffers.
>
> Code seems sane, FWIW, but the commit message sounds petty,
> which is likely why nobody is willing to ack it.

Heh, feel free to look at Alexei's comments to my original email; the
commit description seems spot on to me.

FWIW, once Casey and John give a thumbs up on their respective code
areas I do plan to pull this into the lsm/next tree with, or without,
any ACKs from the netdev/bpf folks.  It would be nice if I could get
an ACK from you guys, but since netdev/bpf feels comfortable merging
small security/ code without ACKs from the LSM folks, I see no problem
merging small netdev/bpf code with ACKs from the netdev/bpf folks.
Alexei Starovoitov Oct. 14, 2022, 3:51 p.m. UTC | #6
On Thu, Oct 13, 2022 at 8:59 AM Paul Moore <paul@paul-moore.com> wrote:
>
> On Thu, Oct 13, 2022 at 11:53 AM Jakub Kicinski <kuba@kernel.org> wrote:
> > On Mon, 10 Oct 2022 17:58:29 -0400 Paul Moore wrote:
> > > Commit 4ff09db1b79b ("bpf: net: Change sk_getsockopt() to take the
> > > sockptr_t argument") made it possible to call sk_getsockopt()
> > > with both user and kernel address space buffers through the use of
> > > the sockptr_t type.  Unfortunately at the time of conversion the
> > > security_socket_getpeersec_stream() LSM hook was written to only
> > > accept userspace buffers, and in a desire to avoid having to change
> > > the LSM hook the commit author simply passed the sockptr_t's
> > > userspace buffer pointer.  Since the only sk_getsockopt() callers
> > > at the time of conversion which used kernel sockptr_t buffers did
> > > not allow SO_PEERSEC, and hence the
> > > security_socket_getpeersec_stream() hook, this was acceptable but
> > > also very fragile as future changes presented the possibility of
> > > silently passing kernel space pointers to the LSM hook.
> > >
> > > There are several ways to protect against this, including careful
> > > code review of future commits, but since relying on code review to
> > > catch bugs is a recipe for disaster and the upstream eBPF maintainer
> > > is "strongly against defensive programming", this patch updates the
> > > LSM hook, and all of the implementations to support sockptr_t and
> > > safely handle both user and kernel space buffers.
> >
> > Code seems sane, FWIW, but the commit message sounds petty,
> > which is likely why nobody is willing to ack it.
>
> Heh, feel free to look at Alexei's comments to my original email; the
> commit description seems spot on to me.

Paul,

The commit message:
"
also very fragile as future changes presented the possibility of
silently passing kernel space pointers to the LSM hook.
"
shows that you do not understand how copy_from_user works.

Martin's change didn't introduce any fragility.
Do you realize that user space can pass any 64-bit value
as 'user pointer' via syscall, right?
And that value may just as well be a valid kernel address.
copy_from_user always had a check to prevent reading kernel
memory. It will simply return an error when it sees
kernel address.

Your patch itself is not wrong per-se, but it's doing
not what you think it's doing.
Right now the patch is useless, but
if switch statement in sol_socket_sockopt() would be relaxed
the bpf progs would be able to pass kernel pointers
to security_socket_getpeersec which makes little sense at this point.
So the code you're adding will be a dead code without a test
for the foreseeable future.
Because of that I can only add my Nack.
Paul Moore Oct. 14, 2022, 4:25 p.m. UTC | #7
On Fri, Oct 14, 2022 at 11:51 AM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
> On Thu, Oct 13, 2022 at 8:59 AM Paul Moore <paul@paul-moore.com> wrote:
> >
> > On Thu, Oct 13, 2022 at 11:53 AM Jakub Kicinski <kuba@kernel.org> wrote:
> > > On Mon, 10 Oct 2022 17:58:29 -0400 Paul Moore wrote:
> > > > Commit 4ff09db1b79b ("bpf: net: Change sk_getsockopt() to take the
> > > > sockptr_t argument") made it possible to call sk_getsockopt()
> > > > with both user and kernel address space buffers through the use of
> > > > the sockptr_t type.  Unfortunately at the time of conversion the
> > > > security_socket_getpeersec_stream() LSM hook was written to only
> > > > accept userspace buffers, and in a desire to avoid having to change
> > > > the LSM hook the commit author simply passed the sockptr_t's
> > > > userspace buffer pointer.  Since the only sk_getsockopt() callers
> > > > at the time of conversion which used kernel sockptr_t buffers did
> > > > not allow SO_PEERSEC, and hence the
> > > > security_socket_getpeersec_stream() hook, this was acceptable but
> > > > also very fragile as future changes presented the possibility of
> > > > silently passing kernel space pointers to the LSM hook.
> > > >
> > > > There are several ways to protect against this, including careful
> > > > code review of future commits, but since relying on code review to
> > > > catch bugs is a recipe for disaster and the upstream eBPF maintainer
> > > > is "strongly against defensive programming", this patch updates the
> > > > LSM hook, and all of the implementations to support sockptr_t and
> > > > safely handle both user and kernel space buffers.
> > >
> > > Code seems sane, FWIW, but the commit message sounds petty,
> > > which is likely why nobody is willing to ack it.
> >
> > Heh, feel free to look at Alexei's comments to my original email; the
> > commit description seems spot on to me.
>
> Paul,
>
> The commit message:
> "
> also very fragile as future changes presented the possibility of
> silently passing kernel space pointers to the LSM hook.
> "
> shows that you do not understand how copy_from_user works.
>
> Martin's change didn't introduce any fragility.
> Do you realize that user space can pass any 64-bit value
> as 'user pointer' via syscall, right?
> And that value may just as well be a valid kernel address.
> copy_from_user always had a check to prevent reading kernel
> memory. It will simply return an error when it sees
> kernel address.
>
> Your patch itself is not wrong per-se, but it's doing
> not what you think it's doing.
> Right now the patch is useless, but
> if switch statement in sol_socket_sockopt() would be relaxed
> the bpf progs would be able to pass kernel pointers
> to security_socket_getpeersec which makes little sense at this point.
> So the code you're adding will be a dead code without a test
> for the foreseeable future.
> Because of that I can only add my Nack.

Oh don't worry, I've already registered your NACK because the patch
has a three line diff in net/core/sock.c and it's going in via the LSM
tree.  I'll CC you on the pull request and mention your NACK to Linus,
you can feel free to make whatever argument you believe justifies your
objection at that point in time.

However, just so I'm clear on your new objection, you are basically
saying that copy_to_sockptr() shouldn't exist?  If you honestly
believe that, and you aren't just picking on this patch because of a
grudge, I would encourage you to submit a patch removing
copy_to_sockptr() and friends from the kernel and let's see what
happens; please CC me on the patch(es) as I think the discussion for
that would be very interesting :)
Casey Schaufler Oct. 20, 2022, 1:16 p.m. UTC | #8
On 10/10/2022 2:58 PM, Paul Moore wrote:
> Commit 4ff09db1b79b ("bpf: net: Change sk_getsockopt() to take the
> sockptr_t argument") made it possible to call sk_getsockopt()
> with both user and kernel address space buffers through the use of
> the sockptr_t type.  Unfortunately at the time of conversion the
> security_socket_getpeersec_stream() LSM hook was written to only
> accept userspace buffers, and in a desire to avoid having to change
> the LSM hook the commit author simply passed the sockptr_t's
> userspace buffer pointer.  Since the only sk_getsockopt() callers
> at the time of conversion which used kernel sockptr_t buffers did
> not allow SO_PEERSEC, and hence the
> security_socket_getpeersec_stream() hook, this was acceptable but
> also very fragile as future changes presented the possibility of
> silently passing kernel space pointers to the LSM hook.
>
> There are several ways to protect against this, including careful
> code review of future commits, but since relying on code review to
> catch bugs is a recipe for disaster and the upstream eBPF maintainer
> is "strongly against defensive programming", this patch updates the
> LSM hook, and all of the implementations to support sockptr_t and
> safely handle both user and kernel space buffers.
>
> Signed-off-by: Paul Moore <paul@paul-moore.com>

Smack part looks ok, I haven't had the opportunity to test it.
Will do so as I crunch through the backlog.

Acked-by: Casey Schaufler <casey@schaufler-ca.com>

> ---
>  include/linux/lsm_hook_defs.h |    2 +-
>  include/linux/lsm_hooks.h     |    4 ++--
>  include/linux/security.h      |   11 +++++++----
>  net/core/sock.c               |    3 ++-
>  security/apparmor/lsm.c       |   29 +++++++++++++----------------
>  security/security.c           |    6 +++---
>  security/selinux/hooks.c      |   13 ++++++-------
>  security/smack/smack_lsm.c    |   19 ++++++++++---------
>  8 files changed, 44 insertions(+), 43 deletions(-)
>
> diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
> index ec119da1d89b4..6abde829b6e5e 100644
> --- a/include/linux/lsm_hook_defs.h
> +++ b/include/linux/lsm_hook_defs.h
> @@ -302,7 +302,7 @@ LSM_HOOK(int, 0, socket_setsockopt, struct socket *sock, int level, int optname)
>  LSM_HOOK(int, 0, socket_shutdown, struct socket *sock, int how)
>  LSM_HOOK(int, 0, socket_sock_rcv_skb, struct sock *sk, struct sk_buff *skb)
>  LSM_HOOK(int, 0, socket_getpeersec_stream, struct socket *sock,
> -	 char __user *optval, int __user *optlen, unsigned len)
> +	 sockptr_t optval, sockptr_t optlen, unsigned int len)
>  LSM_HOOK(int, 0, socket_getpeersec_dgram, struct socket *sock,
>  	 struct sk_buff *skb, u32 *secid)
>  LSM_HOOK(int, 0, sk_alloc_security, struct sock *sk, int family, gfp_t priority)
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index 4ec80b96c22e7..883f0f252f062 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -962,8 +962,8 @@
>   *	SO_GETPEERSEC.  For tcp sockets this can be meaningful if the
>   *	socket is associated with an ipsec SA.
>   *	@sock is the local socket.
> - *	@optval userspace memory where the security state is to be copied.
> - *	@optlen userspace int where the module should copy the actual length
> + *	@optval memory where the security state is to be copied.
> + *	@optlen memory where the module should copy the actual length
>   *	of the security state.
>   *	@len as input is the maximum length to copy to userspace provided
>   *	by the caller.
> diff --git a/include/linux/security.h b/include/linux/security.h
> index ca1b7109c0dbb..0e419c595cee5 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -31,6 +31,7 @@
>  #include <linux/err.h>
>  #include <linux/string.h>
>  #include <linux/mm.h>
> +#include <linux/sockptr.h>
>  
>  struct linux_binprm;
>  struct cred;
> @@ -1411,8 +1412,8 @@ int security_socket_getsockopt(struct socket *sock, int level, int optname);
>  int security_socket_setsockopt(struct socket *sock, int level, int optname);
>  int security_socket_shutdown(struct socket *sock, int how);
>  int security_sock_rcv_skb(struct sock *sk, struct sk_buff *skb);
> -int security_socket_getpeersec_stream(struct socket *sock, char __user *optval,
> -				      int __user *optlen, unsigned len);
> +int security_socket_getpeersec_stream(struct socket *sock, sockptr_t optval,
> +				      sockptr_t optlen, unsigned int len);
>  int security_socket_getpeersec_dgram(struct socket *sock, struct sk_buff *skb, u32 *secid);
>  int security_sk_alloc(struct sock *sk, int family, gfp_t priority);
>  void security_sk_free(struct sock *sk);
> @@ -1548,8 +1549,10 @@ static inline int security_sock_rcv_skb(struct sock *sk,
>  	return 0;
>  }
>  
> -static inline int security_socket_getpeersec_stream(struct socket *sock, char __user *optval,
> -						    int __user *optlen, unsigned len)
> +static inline int security_socket_getpeersec_stream(struct socket *sock,
> +						    sockptr_t optval,
> +						    sockptr_t optlen,
> +						    unsigned int len)
>  {
>  	return -ENOPROTOOPT;
>  }
> diff --git a/net/core/sock.c b/net/core/sock.c
> index eeb6cbac6f499..70064415349d6 100644
> --- a/net/core/sock.c
> +++ b/net/core/sock.c
> @@ -1793,7 +1793,8 @@ int sk_getsockopt(struct sock *sk, int level, int optname,
>  		break;
>  
>  	case SO_PEERSEC:
> -		return security_socket_getpeersec_stream(sock, optval.user, optlen.user, len);
> +		return security_socket_getpeersec_stream(sock,
> +							 optval, optlen, len);
>  
>  	case SO_MARK:
>  		v.val = sk->sk_mark;
> diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
> index f56070270c69d..89e84ef54e8eb 100644
> --- a/security/apparmor/lsm.c
> +++ b/security/apparmor/lsm.c
> @@ -1103,11 +1103,10 @@ static struct aa_label *sk_peer_label(struct sock *sk)
>   * Note: for tcp only valid if using ipsec or cipso on lan
>   */
>  static int apparmor_socket_getpeersec_stream(struct socket *sock,
> -					     char __user *optval,
> -					     int __user *optlen,
> +					     sockptr_t optval, sockptr_t optlen,
>  					     unsigned int len)
>  {
> -	char *name;
> +	char *name = NULL;
>  	int slen, error = 0;
>  	struct aa_label *label;
>  	struct aa_label *peer;
> @@ -1124,23 +1123,21 @@ static int apparmor_socket_getpeersec_stream(struct socket *sock,
>  	/* don't include terminating \0 in slen, it breaks some apps */
>  	if (slen < 0) {
>  		error = -ENOMEM;
> -	} else {
> -		if (slen > len) {
> -			error = -ERANGE;
> -		} else if (copy_to_user(optval, name, slen)) {
> -			error = -EFAULT;
> -			goto out;
> -		}
> -		if (put_user(slen, optlen))
> -			error = -EFAULT;
> -out:
> -		kfree(name);
> -
> +		goto done;
> +	}
> +	if (slen > len) {
> +		error = -ERANGE;
> +		goto done_len;
>  	}
>  
> +	if (copy_to_sockptr(optval, name, slen))
> +		error = -EFAULT;
> +done_len:
> +	if (copy_to_sockptr(optlen, &slen, sizeof(slen)))
> +		error = -EFAULT;
>  done:
>  	end_current_label_crit_section(label);
> -
> +	kfree(name);
>  	return error;
>  }
>  
> diff --git a/security/security.c b/security/security.c
> index 79d82cb6e4696..f27c885ee98db 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -2267,11 +2267,11 @@ int security_sock_rcv_skb(struct sock *sk, struct sk_buff *skb)
>  }
>  EXPORT_SYMBOL(security_sock_rcv_skb);
>  
> -int security_socket_getpeersec_stream(struct socket *sock, char __user *optval,
> -				      int __user *optlen, unsigned len)
> +int security_socket_getpeersec_stream(struct socket *sock, sockptr_t optval,
> +				      sockptr_t optlen, unsigned int len)
>  {
>  	return call_int_hook(socket_getpeersec_stream, -ENOPROTOOPT, sock,
> -				optval, optlen, len);
> +			     optval, optlen, len);
>  }
>  
>  int security_socket_getpeersec_dgram(struct socket *sock, struct sk_buff *skb, u32 *secid)
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index f553c370397ee..0bdddeba90a6c 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -5119,11 +5119,12 @@ static int selinux_socket_sock_rcv_skb(struct sock *sk, struct sk_buff *skb)
>  	return err;
>  }
>  
> -static int selinux_socket_getpeersec_stream(struct socket *sock, char __user *optval,
> -					    int __user *optlen, unsigned len)
> +static int selinux_socket_getpeersec_stream(struct socket *sock,
> +					    sockptr_t optval, sockptr_t optlen,
> +					    unsigned int len)
>  {
>  	int err = 0;
> -	char *scontext;
> +	char *scontext = NULL;
>  	u32 scontext_len;
>  	struct sk_security_struct *sksec = sock->sk->sk_security;
>  	u32 peer_sid = SECSID_NULL;
> @@ -5139,17 +5140,15 @@ static int selinux_socket_getpeersec_stream(struct socket *sock, char __user *op
>  				      &scontext_len);
>  	if (err)
>  		return err;
> -
>  	if (scontext_len > len) {
>  		err = -ERANGE;
>  		goto out_len;
>  	}
>  
> -	if (copy_to_user(optval, scontext, scontext_len))
> +	if (copy_to_sockptr(optval, scontext, scontext_len))
>  		err = -EFAULT;
> -
>  out_len:
> -	if (put_user(scontext_len, optlen))
> +	if (copy_to_sockptr(optlen, &scontext_len, sizeof(scontext_len)))
>  		err = -EFAULT;
>  	kfree(scontext);
>  	return err;
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index b6306d71c9088..2bd7fadf7fb4c 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -4006,12 +4006,12 @@ static int smack_socket_sock_rcv_skb(struct sock *sk, struct sk_buff *skb)
>   * returns zero on success, an error code otherwise
>   */
>  static int smack_socket_getpeersec_stream(struct socket *sock,
> -					  char __user *optval,
> -					  int __user *optlen, unsigned len)
> +					  sockptr_t optval, sockptr_t optlen,
> +					  unsigned int len)
>  {
>  	struct socket_smack *ssp;
>  	char *rcp = "";
> -	int slen = 1;
> +	u32 slen = 1;
>  	int rc = 0;
>  
>  	ssp = sock->sk->sk_security;
> @@ -4019,15 +4019,16 @@ static int smack_socket_getpeersec_stream(struct socket *sock,
>  		rcp = ssp->smk_packet->smk_known;
>  		slen = strlen(rcp) + 1;
>  	}
> -
> -	if (slen > len)
> +	if (slen > len) {
>  		rc = -ERANGE;
> -	else if (copy_to_user(optval, rcp, slen) != 0)
> -		rc = -EFAULT;
> +		goto out_len;
> +	}
>  
> -	if (put_user(slen, optlen) != 0)
> +	if (copy_to_sockptr(optval, rcp, slen))
> +		rc = -EFAULT;
> +out_len:
> +	if (copy_to_sockptr(optlen, &slen, sizeof(slen)))
>  		rc = -EFAULT;
> -
>  	return rc;
>  }
>  
>
Paul Moore Oct. 20, 2022, 3:10 p.m. UTC | #9
On Thu, Oct 20, 2022 at 9:16 AM Casey Schaufler <casey@schaufler-ca.com> wrote:
> On 10/10/2022 2:58 PM, Paul Moore wrote:
> > Commit 4ff09db1b79b ("bpf: net: Change sk_getsockopt() to take the
> > sockptr_t argument") made it possible to call sk_getsockopt()
> > with both user and kernel address space buffers through the use of
> > the sockptr_t type.  Unfortunately at the time of conversion the
> > security_socket_getpeersec_stream() LSM hook was written to only
> > accept userspace buffers, and in a desire to avoid having to change
> > the LSM hook the commit author simply passed the sockptr_t's
> > userspace buffer pointer.  Since the only sk_getsockopt() callers
> > at the time of conversion which used kernel sockptr_t buffers did
> > not allow SO_PEERSEC, and hence the
> > security_socket_getpeersec_stream() hook, this was acceptable but
> > also very fragile as future changes presented the possibility of
> > silently passing kernel space pointers to the LSM hook.
> >
> > There are several ways to protect against this, including careful
> > code review of future commits, but since relying on code review to
> > catch bugs is a recipe for disaster and the upstream eBPF maintainer
> > is "strongly against defensive programming", this patch updates the
> > LSM hook, and all of the implementations to support sockptr_t and
> > safely handle both user and kernel space buffers.
> >
> > Signed-off-by: Paul Moore <paul@paul-moore.com>
>
> Smack part looks ok, I haven't had the opportunity to test it.
> Will do so as I crunch through the backlog.
>
> Acked-by: Casey Schaufler <casey@schaufler-ca.com>

Thanks Casey.

John, how do the AppArmor parts look?

> > ---
> >  include/linux/lsm_hook_defs.h |    2 +-
> >  include/linux/lsm_hooks.h     |    4 ++--
> >  include/linux/security.h      |   11 +++++++----
> >  net/core/sock.c               |    3 ++-
> >  security/apparmor/lsm.c       |   29 +++++++++++++----------------
> >  security/security.c           |    6 +++---
> >  security/selinux/hooks.c      |   13 ++++++-------
> >  security/smack/smack_lsm.c    |   19 ++++++++++---------
> >  8 files changed, 44 insertions(+), 43 deletions(-)
Paul Moore Oct. 28, 2022, 10:32 a.m. UTC | #10
On Thu, Oct 20, 2022 at 11:10 AM Paul Moore <paul@paul-moore.com> wrote:
> On Thu, Oct 20, 2022 at 9:16 AM Casey Schaufler <casey@schaufler-ca.com> wrote:
> > On 10/10/2022 2:58 PM, Paul Moore wrote:
> > > Commit 4ff09db1b79b ("bpf: net: Change sk_getsockopt() to take the
> > > sockptr_t argument") made it possible to call sk_getsockopt()
> > > with both user and kernel address space buffers through the use of
> > > the sockptr_t type.  Unfortunately at the time of conversion the
> > > security_socket_getpeersec_stream() LSM hook was written to only
> > > accept userspace buffers, and in a desire to avoid having to change
> > > the LSM hook the commit author simply passed the sockptr_t's
> > > userspace buffer pointer.  Since the only sk_getsockopt() callers
> > > at the time of conversion which used kernel sockptr_t buffers did
> > > not allow SO_PEERSEC, and hence the
> > > security_socket_getpeersec_stream() hook, this was acceptable but
> > > also very fragile as future changes presented the possibility of
> > > silently passing kernel space pointers to the LSM hook.
> > >
> > > There are several ways to protect against this, including careful
> > > code review of future commits, but since relying on code review to
> > > catch bugs is a recipe for disaster and the upstream eBPF maintainer
> > > is "strongly against defensive programming", this patch updates the
> > > LSM hook, and all of the implementations to support sockptr_t and
> > > safely handle both user and kernel space buffers.
> > >
> > > Signed-off-by: Paul Moore <paul@paul-moore.com>
> >
> > Smack part looks ok, I haven't had the opportunity to test it.
> > Will do so as I crunch through the backlog.
> >
> > Acked-by: Casey Schaufler <casey@schaufler-ca.com>
>
> Thanks Casey.
>
> John, how do the AppArmor parts look?

Hey John - can you weigh in on the AppArmor bits?

> > > ---
> > >  include/linux/lsm_hook_defs.h |    2 +-
> > >  include/linux/lsm_hooks.h     |    4 ++--
> > >  include/linux/security.h      |   11 +++++++----
> > >  net/core/sock.c               |    3 ++-
> > >  security/apparmor/lsm.c       |   29 +++++++++++++----------------
> > >  security/security.c           |    6 +++---
> > >  security/selinux/hooks.c      |   13 ++++++-------
> > >  security/smack/smack_lsm.c    |   19 ++++++++++---------
> > >  8 files changed, 44 insertions(+), 43 deletions(-)
John Johansen Oct. 28, 2022, 12:31 p.m. UTC | #11
On 10/10/22 15:00, Paul Moore wrote:
> On Mon, Oct 10, 2022 at 5:58 PM Paul Moore <paul@paul-moore.com> wrote:
>>
>> Commit 4ff09db1b79b ("bpf: net: Change sk_getsockopt() to take the
>> sockptr_t argument") made it possible to call sk_getsockopt()
>> with both user and kernel address space buffers through the use of
>> the sockptr_t type.  Unfortunately at the time of conversion the
>> security_socket_getpeersec_stream() LSM hook was written to only
>> accept userspace buffers, and in a desire to avoid having to change
>> the LSM hook the commit author simply passed the sockptr_t's
>> userspace buffer pointer.  Since the only sk_getsockopt() callers
>> at the time of conversion which used kernel sockptr_t buffers did
>> not allow SO_PEERSEC, and hence the
>> security_socket_getpeersec_stream() hook, this was acceptable but
>> also very fragile as future changes presented the possibility of
>> silently passing kernel space pointers to the LSM hook.
>>
>> There are several ways to protect against this, including careful
>> code review of future commits, but since relying on code review to
>> catch bugs is a recipe for disaster and the upstream eBPF maintainer
>> is "strongly against defensive programming", this patch updates the
>> LSM hook, and all of the implementations to support sockptr_t and
>> safely handle both user and kernel space buffers.
>>
>> Signed-off-by: Paul Moore <paul@paul-moore.com>
>> ---
>>   include/linux/lsm_hook_defs.h |    2 +-
>>   include/linux/lsm_hooks.h     |    4 ++--
>>   include/linux/security.h      |   11 +++++++----
>>   net/core/sock.c               |    3 ++-
>>   security/apparmor/lsm.c       |   29 +++++++++++++----------------
>>   security/security.c           |    6 +++---
>>   security/selinux/hooks.c      |   13 ++++++-------
>>   security/smack/smack_lsm.c    |   19 ++++++++++---------
>>   8 files changed, 44 insertions(+), 43 deletions(-)
> 
> Casey and John, could you please look over the Smack and AppArmor bits
> of this patch when you get a chance?  I did my best on the conversion,
> but I would appreciate a review by the experts :)
> 
yes, I plan to look at it this weekend
John Johansen Oct. 31, 2022, 9:59 a.m. UTC | #12
On 10/10/22 14:58, Paul Moore wrote:
> Commit 4ff09db1b79b ("bpf: net: Change sk_getsockopt() to take the
> sockptr_t argument") made it possible to call sk_getsockopt()
> with both user and kernel address space buffers through the use of
> the sockptr_t type.  Unfortunately at the time of conversion the
> security_socket_getpeersec_stream() LSM hook was written to only
> accept userspace buffers, and in a desire to avoid having to change
> the LSM hook the commit author simply passed the sockptr_t's
> userspace buffer pointer.  Since the only sk_getsockopt() callers
> at the time of conversion which used kernel sockptr_t buffers did
> not allow SO_PEERSEC, and hence the
> security_socket_getpeersec_stream() hook, this was acceptable but
> also very fragile as future changes presented the possibility of
> silently passing kernel space pointers to the LSM hook.
> 
> There are several ways to protect against this, including careful
> code review of future commits, but since relying on code review to
> catch bugs is a recipe for disaster and the upstream eBPF maintainer
> is "strongly against defensive programming", this patch updates the
> LSM hook, and all of the implementations to support sockptr_t and
> safely handle both user and kernel space buffers.
> 
> Signed-off-by: Paul Moore <paul@paul-moore.com>

looks good to me

Acked-by: John Johansen <john.johansen@canonical.com>

> ---
>   include/linux/lsm_hook_defs.h |    2 +-
>   include/linux/lsm_hooks.h     |    4 ++--
>   include/linux/security.h      |   11 +++++++----
>   net/core/sock.c               |    3 ++-
>   security/apparmor/lsm.c       |   29 +++++++++++++----------------
>   security/security.c           |    6 +++---
>   security/selinux/hooks.c      |   13 ++++++-------
>   security/smack/smack_lsm.c    |   19 ++++++++++---------
>   8 files changed, 44 insertions(+), 43 deletions(-)
> 
> diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
> index ec119da1d89b4..6abde829b6e5e 100644
> --- a/include/linux/lsm_hook_defs.h
> +++ b/include/linux/lsm_hook_defs.h
> @@ -302,7 +302,7 @@ LSM_HOOK(int, 0, socket_setsockopt, struct socket *sock, int level, int optname)
>   LSM_HOOK(int, 0, socket_shutdown, struct socket *sock, int how)
>   LSM_HOOK(int, 0, socket_sock_rcv_skb, struct sock *sk, struct sk_buff *skb)
>   LSM_HOOK(int, 0, socket_getpeersec_stream, struct socket *sock,
> -	 char __user *optval, int __user *optlen, unsigned len)
> +	 sockptr_t optval, sockptr_t optlen, unsigned int len)
>   LSM_HOOK(int, 0, socket_getpeersec_dgram, struct socket *sock,
>   	 struct sk_buff *skb, u32 *secid)
>   LSM_HOOK(int, 0, sk_alloc_security, struct sock *sk, int family, gfp_t priority)
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index 4ec80b96c22e7..883f0f252f062 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -962,8 +962,8 @@
>    *	SO_GETPEERSEC.  For tcp sockets this can be meaningful if the
>    *	socket is associated with an ipsec SA.
>    *	@sock is the local socket.
> - *	@optval userspace memory where the security state is to be copied.
> - *	@optlen userspace int where the module should copy the actual length
> + *	@optval memory where the security state is to be copied.
> + *	@optlen memory where the module should copy the actual length
>    *	of the security state.
>    *	@len as input is the maximum length to copy to userspace provided
>    *	by the caller.
> diff --git a/include/linux/security.h b/include/linux/security.h
> index ca1b7109c0dbb..0e419c595cee5 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -31,6 +31,7 @@
>   #include <linux/err.h>
>   #include <linux/string.h>
>   #include <linux/mm.h>
> +#include <linux/sockptr.h>
>   
>   struct linux_binprm;
>   struct cred;
> @@ -1411,8 +1412,8 @@ int security_socket_getsockopt(struct socket *sock, int level, int optname);
>   int security_socket_setsockopt(struct socket *sock, int level, int optname);
>   int security_socket_shutdown(struct socket *sock, int how);
>   int security_sock_rcv_skb(struct sock *sk, struct sk_buff *skb);
> -int security_socket_getpeersec_stream(struct socket *sock, char __user *optval,
> -				      int __user *optlen, unsigned len);
> +int security_socket_getpeersec_stream(struct socket *sock, sockptr_t optval,
> +				      sockptr_t optlen, unsigned int len);
>   int security_socket_getpeersec_dgram(struct socket *sock, struct sk_buff *skb, u32 *secid);
>   int security_sk_alloc(struct sock *sk, int family, gfp_t priority);
>   void security_sk_free(struct sock *sk);
> @@ -1548,8 +1549,10 @@ static inline int security_sock_rcv_skb(struct sock *sk,
>   	return 0;
>   }
>   
> -static inline int security_socket_getpeersec_stream(struct socket *sock, char __user *optval,
> -						    int __user *optlen, unsigned len)
> +static inline int security_socket_getpeersec_stream(struct socket *sock,
> +						    sockptr_t optval,
> +						    sockptr_t optlen,
> +						    unsigned int len)
>   {
>   	return -ENOPROTOOPT;
>   }
> diff --git a/net/core/sock.c b/net/core/sock.c
> index eeb6cbac6f499..70064415349d6 100644
> --- a/net/core/sock.c
> +++ b/net/core/sock.c
> @@ -1793,7 +1793,8 @@ int sk_getsockopt(struct sock *sk, int level, int optname,
>   		break;
>   
>   	case SO_PEERSEC:
> -		return security_socket_getpeersec_stream(sock, optval.user, optlen.user, len);
> +		return security_socket_getpeersec_stream(sock,
> +							 optval, optlen, len);
>   
>   	case SO_MARK:
>   		v.val = sk->sk_mark;
> diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
> index f56070270c69d..89e84ef54e8eb 100644
> --- a/security/apparmor/lsm.c
> +++ b/security/apparmor/lsm.c
> @@ -1103,11 +1103,10 @@ static struct aa_label *sk_peer_label(struct sock *sk)
>    * Note: for tcp only valid if using ipsec or cipso on lan
>    */
>   static int apparmor_socket_getpeersec_stream(struct socket *sock,
> -					     char __user *optval,
> -					     int __user *optlen,
> +					     sockptr_t optval, sockptr_t optlen,
>   					     unsigned int len)
>   {
> -	char *name;
> +	char *name = NULL;
>   	int slen, error = 0;
>   	struct aa_label *label;
>   	struct aa_label *peer;
> @@ -1124,23 +1123,21 @@ static int apparmor_socket_getpeersec_stream(struct socket *sock,
>   	/* don't include terminating \0 in slen, it breaks some apps */
>   	if (slen < 0) {
>   		error = -ENOMEM;
> -	} else {
> -		if (slen > len) {
> -			error = -ERANGE;
> -		} else if (copy_to_user(optval, name, slen)) {
> -			error = -EFAULT;
> -			goto out;
> -		}
> -		if (put_user(slen, optlen))
> -			error = -EFAULT;
> -out:
> -		kfree(name);
> -
> +		goto done;
> +	}
> +	if (slen > len) {
> +		error = -ERANGE;
> +		goto done_len;
>   	}
>   
> +	if (copy_to_sockptr(optval, name, slen))
> +		error = -EFAULT;
> +done_len:
> +	if (copy_to_sockptr(optlen, &slen, sizeof(slen)))
> +		error = -EFAULT;
>   done:
>   	end_current_label_crit_section(label);
> -
> +	kfree(name);
>   	return error;
>   }
>   
> diff --git a/security/security.c b/security/security.c
> index 79d82cb6e4696..f27c885ee98db 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -2267,11 +2267,11 @@ int security_sock_rcv_skb(struct sock *sk, struct sk_buff *skb)
>   }
>   EXPORT_SYMBOL(security_sock_rcv_skb);
>   
> -int security_socket_getpeersec_stream(struct socket *sock, char __user *optval,
> -				      int __user *optlen, unsigned len)
> +int security_socket_getpeersec_stream(struct socket *sock, sockptr_t optval,
> +				      sockptr_t optlen, unsigned int len)
>   {
>   	return call_int_hook(socket_getpeersec_stream, -ENOPROTOOPT, sock,
> -				optval, optlen, len);
> +			     optval, optlen, len);
>   }
>   
>   int security_socket_getpeersec_dgram(struct socket *sock, struct sk_buff *skb, u32 *secid)
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index f553c370397ee..0bdddeba90a6c 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -5119,11 +5119,12 @@ static int selinux_socket_sock_rcv_skb(struct sock *sk, struct sk_buff *skb)
>   	return err;
>   }
>   
> -static int selinux_socket_getpeersec_stream(struct socket *sock, char __user *optval,
> -					    int __user *optlen, unsigned len)
> +static int selinux_socket_getpeersec_stream(struct socket *sock,
> +					    sockptr_t optval, sockptr_t optlen,
> +					    unsigned int len)
>   {
>   	int err = 0;
> -	char *scontext;
> +	char *scontext = NULL;
>   	u32 scontext_len;
>   	struct sk_security_struct *sksec = sock->sk->sk_security;
>   	u32 peer_sid = SECSID_NULL;
> @@ -5139,17 +5140,15 @@ static int selinux_socket_getpeersec_stream(struct socket *sock, char __user *op
>   				      &scontext_len);
>   	if (err)
>   		return err;
> -
>   	if (scontext_len > len) {
>   		err = -ERANGE;
>   		goto out_len;
>   	}
>   
> -	if (copy_to_user(optval, scontext, scontext_len))
> +	if (copy_to_sockptr(optval, scontext, scontext_len))
>   		err = -EFAULT;
> -
>   out_len:
> -	if (put_user(scontext_len, optlen))
> +	if (copy_to_sockptr(optlen, &scontext_len, sizeof(scontext_len)))
>   		err = -EFAULT;
>   	kfree(scontext);
>   	return err;
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index b6306d71c9088..2bd7fadf7fb4c 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -4006,12 +4006,12 @@ static int smack_socket_sock_rcv_skb(struct sock *sk, struct sk_buff *skb)
>    * returns zero on success, an error code otherwise
>    */
>   static int smack_socket_getpeersec_stream(struct socket *sock,
> -					  char __user *optval,
> -					  int __user *optlen, unsigned len)
> +					  sockptr_t optval, sockptr_t optlen,
> +					  unsigned int len)
>   {
>   	struct socket_smack *ssp;
>   	char *rcp = "";
> -	int slen = 1;
> +	u32 slen = 1;
>   	int rc = 0;
>   
>   	ssp = sock->sk->sk_security;
> @@ -4019,15 +4019,16 @@ static int smack_socket_getpeersec_stream(struct socket *sock,
>   		rcp = ssp->smk_packet->smk_known;
>   		slen = strlen(rcp) + 1;
>   	}
> -
> -	if (slen > len)
> +	if (slen > len) {
>   		rc = -ERANGE;
> -	else if (copy_to_user(optval, rcp, slen) != 0)
> -		rc = -EFAULT;
> +		goto out_len;
> +	}
>   
> -	if (put_user(slen, optlen) != 0)
> +	if (copy_to_sockptr(optval, rcp, slen))
> +		rc = -EFAULT;
> +out_len:
> +	if (copy_to_sockptr(optlen, &slen, sizeof(slen)))
>   		rc = -EFAULT;
> -
>   	return rc;
>   }
>   
>
Paul Moore Nov. 5, 2022, 3:32 a.m. UTC | #13
On Mon, Oct 31, 2022 at 5:59 AM John Johansen
<john.johansen@canonical.com> wrote:
> On 10/10/22 14:58, Paul Moore wrote:
> > Commit 4ff09db1b79b ("bpf: net: Change sk_getsockopt() to take the
> > sockptr_t argument") made it possible to call sk_getsockopt()
> > with both user and kernel address space buffers through the use of
> > the sockptr_t type.  Unfortunately at the time of conversion the
> > security_socket_getpeersec_stream() LSM hook was written to only
> > accept userspace buffers, and in a desire to avoid having to change
> > the LSM hook the commit author simply passed the sockptr_t's
> > userspace buffer pointer.  Since the only sk_getsockopt() callers
> > at the time of conversion which used kernel sockptr_t buffers did
> > not allow SO_PEERSEC, and hence the
> > security_socket_getpeersec_stream() hook, this was acceptable but
> > also very fragile as future changes presented the possibility of
> > silently passing kernel space pointers to the LSM hook.
> >
> > There are several ways to protect against this, including careful
> > code review of future commits, but since relying on code review to
> > catch bugs is a recipe for disaster and the upstream eBPF maintainer
> > is "strongly against defensive programming", this patch updates the
> > LSM hook, and all of the implementations to support sockptr_t and
> > safely handle both user and kernel space buffers.
> >
> > Signed-off-by: Paul Moore <paul@paul-moore.com>
>
> looks good to me

Thanks.  I just merged this into lsm/next.

> Acked-by: John Johansen <john.johansen@canonical.com>
diff mbox series

Patch

diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index ec119da1d89b4..6abde829b6e5e 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -302,7 +302,7 @@  LSM_HOOK(int, 0, socket_setsockopt, struct socket *sock, int level, int optname)
 LSM_HOOK(int, 0, socket_shutdown, struct socket *sock, int how)
 LSM_HOOK(int, 0, socket_sock_rcv_skb, struct sock *sk, struct sk_buff *skb)
 LSM_HOOK(int, 0, socket_getpeersec_stream, struct socket *sock,
-	 char __user *optval, int __user *optlen, unsigned len)
+	 sockptr_t optval, sockptr_t optlen, unsigned int len)
 LSM_HOOK(int, 0, socket_getpeersec_dgram, struct socket *sock,
 	 struct sk_buff *skb, u32 *secid)
 LSM_HOOK(int, 0, sk_alloc_security, struct sock *sk, int family, gfp_t priority)
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 4ec80b96c22e7..883f0f252f062 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -962,8 +962,8 @@ 
  *	SO_GETPEERSEC.  For tcp sockets this can be meaningful if the
  *	socket is associated with an ipsec SA.
  *	@sock is the local socket.
- *	@optval userspace memory where the security state is to be copied.
- *	@optlen userspace int where the module should copy the actual length
+ *	@optval memory where the security state is to be copied.
+ *	@optlen memory where the module should copy the actual length
  *	of the security state.
  *	@len as input is the maximum length to copy to userspace provided
  *	by the caller.
diff --git a/include/linux/security.h b/include/linux/security.h
index ca1b7109c0dbb..0e419c595cee5 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -31,6 +31,7 @@ 
 #include <linux/err.h>
 #include <linux/string.h>
 #include <linux/mm.h>
+#include <linux/sockptr.h>
 
 struct linux_binprm;
 struct cred;
@@ -1411,8 +1412,8 @@  int security_socket_getsockopt(struct socket *sock, int level, int optname);
 int security_socket_setsockopt(struct socket *sock, int level, int optname);
 int security_socket_shutdown(struct socket *sock, int how);
 int security_sock_rcv_skb(struct sock *sk, struct sk_buff *skb);
-int security_socket_getpeersec_stream(struct socket *sock, char __user *optval,
-				      int __user *optlen, unsigned len);
+int security_socket_getpeersec_stream(struct socket *sock, sockptr_t optval,
+				      sockptr_t optlen, unsigned int len);
 int security_socket_getpeersec_dgram(struct socket *sock, struct sk_buff *skb, u32 *secid);
 int security_sk_alloc(struct sock *sk, int family, gfp_t priority);
 void security_sk_free(struct sock *sk);
@@ -1548,8 +1549,10 @@  static inline int security_sock_rcv_skb(struct sock *sk,
 	return 0;
 }
 
-static inline int security_socket_getpeersec_stream(struct socket *sock, char __user *optval,
-						    int __user *optlen, unsigned len)
+static inline int security_socket_getpeersec_stream(struct socket *sock,
+						    sockptr_t optval,
+						    sockptr_t optlen,
+						    unsigned int len)
 {
 	return -ENOPROTOOPT;
 }
diff --git a/net/core/sock.c b/net/core/sock.c
index eeb6cbac6f499..70064415349d6 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -1793,7 +1793,8 @@  int sk_getsockopt(struct sock *sk, int level, int optname,
 		break;
 
 	case SO_PEERSEC:
-		return security_socket_getpeersec_stream(sock, optval.user, optlen.user, len);
+		return security_socket_getpeersec_stream(sock,
+							 optval, optlen, len);
 
 	case SO_MARK:
 		v.val = sk->sk_mark;
diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
index f56070270c69d..89e84ef54e8eb 100644
--- a/security/apparmor/lsm.c
+++ b/security/apparmor/lsm.c
@@ -1103,11 +1103,10 @@  static struct aa_label *sk_peer_label(struct sock *sk)
  * Note: for tcp only valid if using ipsec or cipso on lan
  */
 static int apparmor_socket_getpeersec_stream(struct socket *sock,
-					     char __user *optval,
-					     int __user *optlen,
+					     sockptr_t optval, sockptr_t optlen,
 					     unsigned int len)
 {
-	char *name;
+	char *name = NULL;
 	int slen, error = 0;
 	struct aa_label *label;
 	struct aa_label *peer;
@@ -1124,23 +1123,21 @@  static int apparmor_socket_getpeersec_stream(struct socket *sock,
 	/* don't include terminating \0 in slen, it breaks some apps */
 	if (slen < 0) {
 		error = -ENOMEM;
-	} else {
-		if (slen > len) {
-			error = -ERANGE;
-		} else if (copy_to_user(optval, name, slen)) {
-			error = -EFAULT;
-			goto out;
-		}
-		if (put_user(slen, optlen))
-			error = -EFAULT;
-out:
-		kfree(name);
-
+		goto done;
+	}
+	if (slen > len) {
+		error = -ERANGE;
+		goto done_len;
 	}
 
+	if (copy_to_sockptr(optval, name, slen))
+		error = -EFAULT;
+done_len:
+	if (copy_to_sockptr(optlen, &slen, sizeof(slen)))
+		error = -EFAULT;
 done:
 	end_current_label_crit_section(label);
-
+	kfree(name);
 	return error;
 }
 
diff --git a/security/security.c b/security/security.c
index 79d82cb6e4696..f27c885ee98db 100644
--- a/security/security.c
+++ b/security/security.c
@@ -2267,11 +2267,11 @@  int security_sock_rcv_skb(struct sock *sk, struct sk_buff *skb)
 }
 EXPORT_SYMBOL(security_sock_rcv_skb);
 
-int security_socket_getpeersec_stream(struct socket *sock, char __user *optval,
-				      int __user *optlen, unsigned len)
+int security_socket_getpeersec_stream(struct socket *sock, sockptr_t optval,
+				      sockptr_t optlen, unsigned int len)
 {
 	return call_int_hook(socket_getpeersec_stream, -ENOPROTOOPT, sock,
-				optval, optlen, len);
+			     optval, optlen, len);
 }
 
 int security_socket_getpeersec_dgram(struct socket *sock, struct sk_buff *skb, u32 *secid)
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index f553c370397ee..0bdddeba90a6c 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -5119,11 +5119,12 @@  static int selinux_socket_sock_rcv_skb(struct sock *sk, struct sk_buff *skb)
 	return err;
 }
 
-static int selinux_socket_getpeersec_stream(struct socket *sock, char __user *optval,
-					    int __user *optlen, unsigned len)
+static int selinux_socket_getpeersec_stream(struct socket *sock,
+					    sockptr_t optval, sockptr_t optlen,
+					    unsigned int len)
 {
 	int err = 0;
-	char *scontext;
+	char *scontext = NULL;
 	u32 scontext_len;
 	struct sk_security_struct *sksec = sock->sk->sk_security;
 	u32 peer_sid = SECSID_NULL;
@@ -5139,17 +5140,15 @@  static int selinux_socket_getpeersec_stream(struct socket *sock, char __user *op
 				      &scontext_len);
 	if (err)
 		return err;
-
 	if (scontext_len > len) {
 		err = -ERANGE;
 		goto out_len;
 	}
 
-	if (copy_to_user(optval, scontext, scontext_len))
+	if (copy_to_sockptr(optval, scontext, scontext_len))
 		err = -EFAULT;
-
 out_len:
-	if (put_user(scontext_len, optlen))
+	if (copy_to_sockptr(optlen, &scontext_len, sizeof(scontext_len)))
 		err = -EFAULT;
 	kfree(scontext);
 	return err;
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index b6306d71c9088..2bd7fadf7fb4c 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -4006,12 +4006,12 @@  static int smack_socket_sock_rcv_skb(struct sock *sk, struct sk_buff *skb)
  * returns zero on success, an error code otherwise
  */
 static int smack_socket_getpeersec_stream(struct socket *sock,
-					  char __user *optval,
-					  int __user *optlen, unsigned len)
+					  sockptr_t optval, sockptr_t optlen,
+					  unsigned int len)
 {
 	struct socket_smack *ssp;
 	char *rcp = "";
-	int slen = 1;
+	u32 slen = 1;
 	int rc = 0;
 
 	ssp = sock->sk->sk_security;
@@ -4019,15 +4019,16 @@  static int smack_socket_getpeersec_stream(struct socket *sock,
 		rcp = ssp->smk_packet->smk_known;
 		slen = strlen(rcp) + 1;
 	}
-
-	if (slen > len)
+	if (slen > len) {
 		rc = -ERANGE;
-	else if (copy_to_user(optval, rcp, slen) != 0)
-		rc = -EFAULT;
+		goto out_len;
+	}
 
-	if (put_user(slen, optlen) != 0)
+	if (copy_to_sockptr(optval, rcp, slen))
+		rc = -EFAULT;
+out_len:
+	if (copy_to_sockptr(optlen, &slen, sizeof(slen)))
 		rc = -EFAULT;
-
 	return rc;
 }