Skip to contentSkip to conversation

NVMe: Fix 0-length integrity payload

PATCH New Keith Busch 3 participants 4 messages Feb 23, 2016 – Feb 24, 2016 NVMe

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 NVMe: Fix 0-length integrity payload

A cheeky user could send a passthrough IO command with a metadata pointer, but on a namespace without metadata. With metadata length of 0, kmalloc returns ZERO_SIZE_PTR. Since that is not NULL, the driver would have set this as the bio's integrity payload, which causes an access fault on completion.

This patch ignores the users metadata buffer if the namespace format does not support separate metadata. This is preferred over returning an invalid error to work with existing user space applications.

Signed-off-by Keith Busch <keith.busch at intel.com>
Reported-by Stephen Bates <stephen.bates at microsemi.com>
---
 drivers/nvme/host/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/nvme/host/core.c +1 -1
index d8c3a55..5340352 100644
@@ -184,7 +184,7 @@ int __nvme_submit_user_cmd(struct request_queue *q, struct nvme_command *cmd,
 			goto out_unmap;
 		}
 
-		if (meta_buffer) {
+		if (meta_buffer && meta_len) {
 			struct bio_integrity_payload *bip;
 
 			meta = kmalloc(meta_len, GFP_KERNEL);
Signature · 1 line
2.6.2.307.g37023ba
#1 lore raw

PATCH 1/1 NVMe: Fix 0-length integrity payload

Quoted 10 lines

A cheeky user could send a passthrough IO command with a metadata pointer, but on a namespace without metadata. With metadata length of 0, kmalloc returns ZERO_SIZE_PTR. Since that is not NULL, the driver would have set this as the bio's integrity payload, which causes an access fault on completion.

This patch ignores the users metadata buffer if the namespace format does not support separate metadata. This is preferred over returning an invalid error to work with existing user space applications.

So having user-space get it wrong forever is preferable? Although I assume that this is the equivalent of a strip operation which is perfectly valid. The one difference is that strip validates the meta data by definition...

If you want to be extra pedantic, you can verify that the meta-data is still valid, but given that the data didn't travel the pci I don't know if it makes real sense...

So all-in-all this looks fine,

Reviewed-by Sagi Grimberg <sagig at mellanox.com>

CC'ing mkp

#2 lore raw

PATCH 1/1 NVMe: Fix 0-length integrity payload

Quoted 7 lines · Sagi Grimberg
Quoted 7 lines

A cheeky user could send a passthrough IO command with a metadata pointer, but on a namespace without metadata. With metadata length of 0, kmalloc returns ZERO_SIZE_PTR. Since that is not NULL, the driver would have set this as the bio's integrity payload, which causes an access fault on completion.

Quoted 8 lines
Quoted 3 lines

This patch ignores the users metadata buffer if the namespace format does not support separate metadata. This is preferred over returning an invalid error to work with existing user space applications.

So having user-space get it wrong forever is preferable? Although I assume that this is the equivalent of a strip operation which is perfectly valid. The one difference is that strip validates the meta data by definition...

Yeah, some have had it wrong for years, but it worked from their perspective. Now it crashes the machine, so this just reinstates the previously established behavior. That seemed better than forcing users to fix their apps. They don't like "fixing" in reaction to kernel changes. Though I'd argue their command was nonsense in the first place, and probably doesn't do what they think they're doing.