Re: [PATCH v2] nvme-tcp: Check if request has started before processing it

On Wed, 2021-03-31 at 09:11 +0200, Hannes Reinecke wrote:

On 3/31/21 1:28 AM, Keith Busch wrote:

quoted

On Tue, Mar 30, 2021 at 10:34:25AM -0700, Sagi Grimberg wrote:

quoted

quoted

quoted

It is, but in this situation, the controller is sending a
second
completion that results in a use-after-free, which makes the
transport irrelevant. Unless there is some other flow (which
is
unclear
to me) that causes this which is a bug that needs to be fixed
rather
than hidden with a safeguard.

The kernel should not crash regardless of any network traffic
that is
sent to the system.  It should not be possible to either
intentionally
of mistakenly contruct packets that will deny service in this
way.

This is not specific to nvme-tcp. I can build an rdma or pci
controller
that can trigger the same crash... I saw a similar patch from
Hannes
implemented in the scsi level, and not the individual scsi
transports..

If scsi wants this too, this could be made generic at the blk-mq
level.
We just need to make something like blk_mq_tag_to_rq(), but return
NULL
if the request isn't started.
 

quoted

I would also mention, that a crash is not even the scariest issue
that
we can see here, because if the request happened to be reused we
are
in the silent data corruption realm...

If this does happen, I think we have to come up with some way to
mitigate it. We're not utilizing the full 16 bits of the
command_id, so
maybe we can append something like a generation sequence number
that can
be checked for validity.

... which will be near impossible.
We can protect against crashing on invalid frames.
We can _not_ protect against maliciously crafted packets referencing
any
random _existing_ tag; that's what TLS is for.

What we can do, though, is checking the 'state' field in the tcp
request, and only allow completions for commands which are in a state
allowing for completions.

Let's see if I can whip up a patch.

That would be great.  BTW in the crash dump I am looking at now, it
looks like pdu->command_id was zero in nvme_tcp_recv_data(), and
blk_mq_tag_to_rq() returned a request struct that had not been used.
So I think we do need to check that the tag was actually allocated.

-Ewan

Cheers,

Hannes

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme