Re ...: [PATCH 5.4 39/78] Bluetooth: use correct lock to prevent UAF of hdev object

[LinMa]

By checking the source code, I found that there are following positions that will access the hci_sk_list.lock

1. void hci_send_to_sock(struct hci_dev *hdev, struct sk_buff *skb)
2. void hci_send_to_channel(unsigned short channel, struct sk_buff *skb,
			 int flag, struct sock *skip_sk)
3. void hci_send_monitor_ctrl_event(struct hci_dev *hdev, u16 event,
				 void *data, u16 data_len, ktime_t tstamp,
				 int flag, struct sock *skip_sk)
4. static void send_monitor_control_replay(struct sock *mon_sk)
And this discussed one
5. void hci_sock_dev_event(struct hci_dev *hdev, int event)

> This comment is right, when I submit this patch I mentioned that the replacement of this lock can hang the detaching routine because it needs to wait the release of the lock_sock().
> 
> But this does no harm in my testing. In fact, the relevant code can only be executed when removing the controller. I think it can wait for the lock. Moreover, this patch can fix the potential UAF indeed.

Assuming the hci_sk_list.lock is held by the cleanup routine. I don't think other possible functions will necessarily busy waiting this lock.

>> >  		/* Detach sockets from device */
>> >  		read_lock(&hci_sk_list.lock);
>> >  		sk_for_each(sk, &hci_sk_list.head) {
>> > -			bh_lock_sock_nested(sk);
>> > +			lock_sock(sk);
>> >  			if (hci_pi(sk)->hdev == hdev) {
>> >  				hci_pi(sk)->hdev = NULL;
>> >  				sk->sk_err = EPIPE;
>> > @@ -764,7 +764,7 @@ void hci_sock_dev_event(struct hci_dev *
>> >  
>> >  				hci_dev_put(hdev);
>> >  			}
>> > -			bh_unlock_sock(sk);
>> > +			release_sock(sk);
>> >  		}
>> >  		read_unlock(&hci_sk_list.lock);
>> >  	}
>> > 
>> >

In another word, these lock requiring events won't be normal. For example, the hci_send_to_sock() function is not assumed to be awakened when the controller is going to be removed. The attacker may intend to do this, however, it seems that he can only hang the kernel by keeping the userfaultfd page. Because he cannot trigger the UAF for now, he won't gain any benefit for hanging the hci_sock_dev_event() function. After the attacker release the userfaultfd page and the hci_send_to_sock() moves on, the hci_sk_list.lock will be hence released as expected.

In summary, I think that: even the hci_sk_list.lock is held and the hci_send_to_sock() functions sleep, it should not have any bad effect as this appearance can only take place in the controller removal routine. Welcome for the further suggestions.

Best Regards

Lin Ma