Skip to content

Commit

Permalink
netlabel: fix RCU annotation for IPv4 options on socket creation
Browse files Browse the repository at this point in the history 
Xiumei reports the following splat when netlabel and TCP socket are used:

 =============================
 WARNING: suspicious RCU usage
 6.9.0-rc2+ torvalds#637 Not tainted
 -----------------------------
 net/ipv4/cipso_ipv4.c:1880 suspicious rcu_dereference_protected() usage!

 other info that might help us debug this:

 rcu_scheduler_active = 2, debug_locks = 1
 1 lock held by ncat/23333:
  #0: ffffffff906030c0 (rcu_read_lock){....}-{1:2}, at: netlbl_sock_setattr+0x25/0x1b0

 stack backtrace:
 CPU: 11 PID: 23333 Comm: ncat Kdump: loaded Not tainted 6.9.0-rc2+ torvalds#637
 Hardware name: Supermicro SYS-6027R-72RF/X9DRH-7TF/7F/iTF/iF, BIOS 3.0  07/26/2013
 Call Trace:
  <TASK>
  dump_stack_lvl+0xa9/0xc0
  lockdep_rcu_suspicious+0x117/0x190
  cipso_v4_sock_setattr+0x1ab/0x1b0
  netlbl_sock_setattr+0x13e/0x1b0
  selinux_netlbl_socket_post_create+0x3f/0x80
  selinux_socket_post_create+0x1a0/0x460
  security_socket_post_create+0x42/0x60
  __sock_create+0x342/0x3a0
  __sys_socket_create.part.22+0x42/0x70
  __sys_socket+0x37/0xb0
  __x64_sys_socket+0x16/0x20
  do_syscall_64+0x96/0x180
  ? do_user_addr_fault+0x68d/0xa30
  ? exc_page_fault+0x171/0x280
  ? asm_exc_page_fault+0x22/0x30
  entry_SYSCALL_64_after_hwframe+0x71/0x79
 RIP: 0033:0x7fbc0ca3fc1b
 Code: 73 01 c3 48 8b 0d 05 f2 1b 00 f7 d8 64 89 01 48 83 c8 ff c3 66 2e 0f 1f 84 00 00 00 00 00 90 f3 0f 1e fa b8 29 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d d5 f1 1b 00 f7 d8 64 89 01 48
 RSP: 002b:00007fff18635208 EFLAGS: 00000246 ORIG_RAX: 0000000000000029
 RAX: ffffffffffffffda RBX: 0000000000000001 RCX: 00007fbc0ca3fc1b
 RDX: 0000000000000006 RSI: 0000000000000001 RDI: 0000000000000002
 RBP: 000055d24f80f8a0 R08: 0000000000000003 R09: 0000000000000001

R10: 0000000000020000 R11: 0000000000000246 R12: 000055d24f80f8a0
 R13: 0000000000000000 R14: 000055d24f80fb88 R15: 0000000000000000
  </TASK>

The current implementation of cipso_v4_sock_setattr() replaces IP options
under the assumption that the caller holds the socket lock; however, such
assumption is not true, nor needed, in selinux_socket_post_create() hook.

Let all callers of cipso_v4_sock_setattr() specify the "socket lock held"
condition, except selinux_socket_post_create() _ where such condition can
safely be set as true even without holding the socket lock.

v5:
 - fix kernel-doc
 - adjust #idfefs around prototype of netlbl_sk_lock_check() (thanks Paul Moore)

v4:
 - fix build when CONFIG_LOCKDEP is unset (thanks kernel test robot)

v3:
 - rename variable to 'sk_locked' (thanks Paul Moore)
 - keep rcu_replace_pointer() open-coded and re-add NULL check of 'old',
   these two changes will be posted in another patch (thanks Paul Moore)

v2:
 - pass lockdep_sock_is_held() through a boolean variable in the stack
   (thanks Eric Dumazet, Paul Moore, Casey Schaufler)
 - use rcu_replace_pointer() instead of rcu_dereference_protected() +
   rcu_assign_pointer()
 - remove NULL check of 'old' before kfree_rcu()

Fixes: f6d8bd0 ("inet: add RCU protection to inet->opt")
Reported-by: Xiumei Mu <[email protected]>
Acked-by: Paul Moore <[email protected]>
Signed-off-by: Davide Caratti <[email protected]>
Acked-by: Casey Schaufler <[email protected]>
Acked-by: Paul Moore <[email protected]>
Signed-off-by: NipaLocal <nipa@local>
  • Loading branch information
@dcaratti
dcaratti authored and NipaLocal committed May 13, 2024
1 parent 15ec7b5 commit f40a86b
Show file tree
Hide file tree
Showing 6 changed files with 52 additions and 12 deletions.
6 changes: 4 additions & 2 deletions include/net/cipso_ipv4.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,8 @@ int cipso_v4_getattr(const unsigned char *cipso,
struct netlbl_lsm_secattr *secattr);
int cipso_v4_sock_setattr(struct sock *sk,
const struct cipso_v4_doi *doi_def,
const struct netlbl_lsm_secattr *secattr);
const struct netlbl_lsm_secattr *secattr,
bool sk_locked);
void cipso_v4_sock_delattr(struct sock *sk);
int cipso_v4_sock_getattr(struct sock *sk, struct netlbl_lsm_secattr *secattr);
int cipso_v4_req_setattr(struct request_sock *req,
Expand Down Expand Up @@ -214,7 +215,8 @@ static inline int cipso_v4_getattr(const unsigned char *cipso,

static inline int cipso_v4_sock_setattr(struct sock *sk,
const struct cipso_v4_doi *doi_def,
const struct netlbl_lsm_secattr *secattr)
const struct netlbl_lsm_secattr *secattr,
bool sk_locked)
{
return -ENOSYS;
}
Expand Down
12 changes: 10 additions & 2 deletions include/net/netlabel.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -470,7 +470,8 @@ void netlbl_bitmap_setbit(unsigned char *bitmap, u32 bit, u8 state);
int netlbl_enabled(void);
int netlbl_sock_setattr(struct sock *sk,
u16 family,
const struct netlbl_lsm_secattr *secattr);
const struct netlbl_lsm_secattr *secattr,
bool sk_locked);
void netlbl_sock_delattr(struct sock *sk);
int netlbl_sock_getattr(struct sock *sk,
struct netlbl_lsm_secattr *secattr);
Expand All @@ -487,6 +488,7 @@ int netlbl_skbuff_getattr(const struct sk_buff *skb,
u16 family,
struct netlbl_lsm_secattr *secattr);
void netlbl_skbuff_err(struct sk_buff *skb, u16 family, int error, int gateway);
bool netlbl_sk_lock_check(struct sock *sk);

/*
* LSM label mapping cache operations
Expand Down Expand Up @@ -614,7 +616,8 @@ static inline int netlbl_enabled(void)
}
static inline int netlbl_sock_setattr(struct sock *sk,
u16 family,
const struct netlbl_lsm_secattr *secattr)
const struct netlbl_lsm_secattr *secattr,
bool sk_locked)
{
return -ENOSYS;
}
Expand Down Expand Up @@ -673,6 +676,11 @@ static inline struct audit_buffer *netlbl_audit_start(int type,
{
return NULL;
}

static inline bool netlbl_sk_lock_check(struct sock *sk)
{
return true;
}
#endif /* CONFIG_NETLABEL */

const struct netlbl_calipso_ops *
Expand Down
7 changes: 4 additions & 3 deletions net/ipv4/cipso_ipv4.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1815,6 +1815,7 @@ static int cipso_v4_genopt(unsigned char *buf, u32 buf_len,
* @sk: the socket
* @doi_def: the CIPSO DOI to use
* @secattr: the specific security attributes of the socket
* @sk_locked: true if caller holds the socket lock
*
* Description:
* Set the CIPSO option on the given socket using the DOI definition and
Expand All @@ -1826,7 +1827,8 @@ static int cipso_v4_genopt(unsigned char *buf, u32 buf_len,
*/
int cipso_v4_sock_setattr(struct sock *sk,
const struct cipso_v4_doi *doi_def,
const struct netlbl_lsm_secattr *secattr)
const struct netlbl_lsm_secattr *secattr,
bool sk_locked)
{
int ret_val = -EPERM;
unsigned char *buf = NULL;
Expand Down Expand Up @@ -1876,8 +1878,7 @@ int cipso_v4_sock_setattr(struct sock *sk,

sk_inet = inet_sk(sk);

old = rcu_dereference_protected(sk_inet->inet_opt,
lockdep_sock_is_held(sk));
old = rcu_dereference_protected(sk_inet->inet_opt, sk_locked);
if (inet_test_bit(IS_ICSK, sk)) {
sk_conn = inet_csk(sk);
if (old)
Expand Down
31 changes: 28 additions & 3 deletions net/netlabel/netlabel_kapi.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -965,6 +965,7 @@ int netlbl_enabled(void)
* @sk: the socket to label
* @family: protocol family
* @secattr: the security attributes
* @sk_locked: true if caller holds the socket lock
*
* Description:
* Attach the correct label to the given socket using the security attributes
Expand All @@ -977,7 +978,8 @@ int netlbl_enabled(void)
*/
int netlbl_sock_setattr(struct sock *sk,
u16 family,
const struct netlbl_lsm_secattr *secattr)
const struct netlbl_lsm_secattr *secattr,
bool sk_locked)
{
int ret_val;
struct netlbl_dom_map *dom_entry;
Expand All @@ -997,7 +999,7 @@ int netlbl_sock_setattr(struct sock *sk,
case NETLBL_NLTYPE_CIPSOV4:
ret_val = cipso_v4_sock_setattr(sk,
dom_entry->def.cipso,
secattr);
secattr, sk_locked);
break;
case NETLBL_NLTYPE_UNLABELED:
ret_val = 0;
Expand Down Expand Up @@ -1090,6 +1092,28 @@ int netlbl_sock_getattr(struct sock *sk,
return ret_val;
}

/**
* netlbl_sk_lock_check - Check if the socket lock has been acquired.
* @sk: the socket to be checked
*
* Return: true if socket @sk is locked or if lock debugging is disabled at
* runtime or compile-time; false otherwise
*
*/
#ifdef CONFIG_LOCKDEP
bool netlbl_sk_lock_check(struct sock *sk)
{
if (debug_locks)
return lockdep_sock_is_held(sk);
return true;
}
#else
bool netlbl_sk_lock_check(struct sock *sk)
{
return true;
}
#endif

/**
* netlbl_conn_setattr - Label a connected socket using the correct protocol
* @sk: the socket to label
Expand Down Expand Up @@ -1126,7 +1150,8 @@ int netlbl_conn_setattr(struct sock *sk,
switch (entry->type) {
case NETLBL_NLTYPE_CIPSOV4:
ret_val = cipso_v4_sock_setattr(sk,
entry->cipso, secattr);
entry->cipso, secattr,
netlbl_sk_lock_check(sk));
break;
case NETLBL_NLTYPE_UNLABELED:
/* just delete the protocols we support for right now
Expand Down
5 changes: 4 additions & 1 deletion security/selinux/netlabel.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -402,7 +402,10 @@ int selinux_netlbl_socket_post_create(struct sock *sk, u16 family)
secattr = selinux_netlbl_sock_genattr(sk);
if (secattr == NULL)
return -ENOMEM;
rc = netlbl_sock_setattr(sk, family, secattr);
/* On socket creation, replacement of IP options is safe even if
* the caller does not hold the socket lock.
*/
rc = netlbl_sock_setattr(sk, family, secattr, true);
switch (rc) {
case 0:
sksec->nlbl_state = NLBL_LABELED;
Expand Down
3 changes: 2 additions & 1 deletion security/smack/smack_lsm.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2565,7 +2565,8 @@ static int smack_netlbl_add(struct sock *sk)
local_bh_disable();
bh_lock_sock_nested(sk);

rc = netlbl_sock_setattr(sk, sk->sk_family, &skp->smk_netlabel);
rc = netlbl_sock_setattr(sk, sk->sk_family, &skp->smk_netlabel,
netlbl_sk_lock_check(sk));
switch (rc) {
case 0:
ssp->smk_state = SMK_NETLBL_LABELED;
Expand Down

0 comments on commit f40a86b

Please sign in to comment.