On Thu, Mar 07, 2024 at 12:27:43PM +0200, Sagi Grimberg wrote:Curiously enough, I had been digging into better error reporting for nvme-fabrics. And the one thing I came up with is to make the controller
Why do we need a refcount for an object that has the same exact lifetime
as the ctrl itself? It just feels like unneeded complication.
My claim the UAF is also possible with the current code is not correct.
Or at least not easy to reproduce. I've re-tested a lot and I couldn't
reproduce it.
Though, the UAF is very simple to reproduce with the sync connect patch
applied (nvme-fc: wait for initial connect attempt to finish) together
with Hannes' patch (nvme: authentication error are always
non-retryable):
In this case, the initial connect fails and the resources are removed,
while we are waiting in
+ if (!opts->connect_async) {
+ enum nvme_ctrl_state state;
+
+ wait_for_completion(&ctrl->connect_completion);
+ state = nvme_ctrl_state(&ctrl->ctrl);
+ nvme_fc_ctrl_put(ctrl);
+
+ if (state != NVME_CTRL_LIVE) {
+ /* Cleanup is handled by the connect state machine */
+ return ERR_PTR(-EIO);
+ }
+ }
This opens up the race window. While we are waiting here for the
completion, the ctrl entry in sysfs is still reachable. Unfortunately,
we also fire an uevent which starts another instance of nvme-cli. And
the new instance of nvme-cli iterates over sysfs and reads the already
freed options object.