Messages in this thread | | | Date | Sat, 02 Dec 2023 19:00:36 +0530 | From | Siddh Raman Pant <> | Subject | Re: [PATCH 2/4] nfc: Protect access to nfc_dev in an llcp_sock with a rwlock |
| |
On Mon, 27 Nov 2023 16:08:16 +0530, Krzysztof Kozlowski wrote: > On 25/11/2023 21:26, Siddh Raman Pant wrote: > > llcp_sock_sendmsg() calls nfc_llcp_send_ui_frame(), which accesses the > > nfc_dev from the llcp_sock for getting the headroom and tailroom needed > > for skb allocation. > > This path should have reference to nfc device: nfc_get_device(). Why is > this not sufficient?
The index needed for nfc_get_device() is inside nfc_dev itself.
Though now that I think about it, I should have modified the get and put functions of llcp_local itself to hold the ref.
As you said, it looks like a band-aid with the extra lock. I agree. Sorry about that.
> > Parallely, the nfc_dev can be freed via the nfc_unregister_device() > > codepath (nfc_release() being called due to the class unregister in > > nfc_exit()), leading to the UAF reported by Syzkaller. > > > > We have the following call tree before freeing: > > > > nfc_unregister_device() > > -> nfc_llcp_unregister_device() > > -> local_cleanup() > > -> nfc_llcp_socket_release() > > > > nfc_llcp_socket_release() sets the state of sockets to LLCP_CLOSED, > > and this is encountered necessarily before any freeing of nfc_dev. > > Sorry, I don't understand. What is encountered before freeing?
nfc_llcp_socket_release() setting of socket state to closed.
> > Thus, add a rwlock in struct llcp_sock to synchronize access to > > nfc_dev. nfc_dev in an llcp_sock will be NULLed in a write critical > > section when socket state has been set to closed. Thus, we can avoid > > the UAF by bailing out from a read critical section upon seeing NULL. > > > > [...] > > > > @@ -102,6 +102,7 @@ struct nfc_llcp_local { > > struct nfc_llcp_sock { > > struct sock sk; > > struct nfc_dev *dev; > > + rwlock_t rw_dev_lock; > > I dislike the idea of introducing the third (!!!) lock here. It looks > like a bandaid for this one particular problem.
Yes, I see it now. Sorry about that.
> > + pr_err("NFC device does not exit\n"); > > exist?
Ouch, yes.
I'll send a v2 improving the things.
Thanks, Siddh
| |