| Message ID | 166543910984.474337.2779830480340611497.stgit@olly (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | lsm: make security_socket_getpeersec_stream() sockptr_t safe | expand |
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 :)
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.
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.
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.
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.
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.
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 :)
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; > } > >
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(-)
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(-)
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
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; > } > >
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 --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; }
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(-)