linux-kernel - Re: [PATCH 02/11] nvmet-fcloop: add ref counting to lport

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <0efdf3e7-e49e-4e2f-846b-b072169de318@flourine.local>
Date: Thu, 6 Mar 2025 10:26:36 +0100
From: Daniel Wagner <dwagner@...e.de>
To: Christoph Hellwig <hch@....de>
Cc: James Smart <james.smart@...adcom.com>, 
	Sagi Grimberg <sagi@...mberg.me>, Chaitanya Kulkarni <kch@...dia.com>, 
	Hannes Reinecke <hare@...e.de>, Keith Busch <kbusch@...nel.org>, linux-nvme@...ts.infradead.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 02/11] nvmet-fcloop: add ref counting to lport

On Wed, Mar 05, 2025 at 03:17:40PM +0100, Christoph Hellwig wrote:
> On Wed, Feb 26, 2025 at 07:45:54PM +0100, Daniel Wagner wrote:
> > +static void
> > +fcloop_lport_free(struct kref *ref)
> > +{
> > +	struct fcloop_lport *lport =
> > +		container_of(ref, struct fcloop_lport, ref);
> > +	unsigned long flags;
> > +
> > +	spin_lock_irqsave(&fcloop_lock, flags);
> > +	list_del(&lport->lport_list);
> > +	spin_unlock_irqrestore(&fcloop_lock, flags);
> > +
> > +	kfree(lport);
> 
> Maybe it's just me, but I find the kref a really horrible pattern over
> usig a simple refcount_t.

There was already some kref usage in the fc code, that's why I started
to use it. But I agree there is not much to be gained from the kref
wrappers. I'll replace them.

> Otherwise adding proper refcounting looks fine.

BTW, I found a bunch more places which need to do proper ref counting. I
have now a version which works pretty good. Though there is one
case which gives me an UAF:

  setup target
  setup host
  connect
  loop
    remove target
    wait for host connecting state
    add target
    wait for host live state

When fcloop has no in flight commands and the target is removed, fcloop
will unregister the localport now. But the nvme-fc driver just assumes
that the port is always there and just sends down new commands
independend of the port status:

nvme_fc_start_fcp_op()
{
[...]

	ret = ctrl->lport->ops->fcp_io(&ctrl->lport->localport,
					&ctrl->rport->remoteport,
					queue->lldd_handle, &op->fcp_req);
[...]
}

There is nothing which updates the ctrl in nvme_fc_unregister_localport.
Not really sure what to do here. fcloop obviously is now behaving
differently to the hw drivers. But still, this looks very fragile that
there is no sort of synchronization between port unregistration and ctrl
state.