nvme/lightnvm: Prevent small buffer overflow in nvme_nvm_identify
PATCH 1/1 nvme/lightnvm: Prevent small buffer overflow in nvme_nvm_identify
There are two closely named structs in lightnvm:
struct nvme_nvm_addr_format and
struct nvme_addr_format.
The first struct has 4 reserved bytes at the end, the second does not. (gdb) p sizeof(struct nvme_nvm_addr_format)
$1 = 16
(gdb) p sizeof(struct nvm_addr_format)
$2 = 12
In the nvme_nvm_identify function we memcpy from the larger struct to the smaller struct. We incorrectly pass the length of the larger struct and overflow by 4 bytes, lets not do that.
--- drivers/nvme/host/lightnvm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/nvme/host/lightnvm.c +1 -1
@@ -324,7 +324,7 @@ static int nvme_nvm_identity(struct nvm_dev *nvmdev, struct nvm_id *nvm_id) nvm_id->cap =le32_to_cpu (nvme_nvm_id->cap); nvm_id->dom =le32_to_cpu (nvme_nvm_id->dom);memcpy (&nvm_id->ppaf, &nvme_nvm_id->ppaf, -sizeof (struct nvme_nvm_addr_format )); +sizeof (struct nvm_addr_format )); ret =init_grps (nvm_id, nvme_nvm_id); out:
Signature · 1 line
2.7.4
PATCH 1/1 nvme/lightnvm: Prevent small buffer overflow in nvme_nvm_identify
+Matias
Quoted 9 lines · Scott Bauer
There are two closely named structs in lightnvm:
structnvme_nvm_addr_formatand
structnvme_addr_format.The first struct has 4 reserved bytes at the end, the second does not. (gdb) p
sizeof(struct nvme_nvm_addr_format)
$1 = 16
(gdb) psizeof(struct nvm_addr_format)
$2 = 12
We need Matias to resolve what the size of these structs are supposed to be. There is a compile time assert:
sizeof(struct nvme_nvm_addr_format) == 128
that should clearly fail, but the compiler removes _nvme_nvm_check_size since it's not called from anywhere. lightnvm has a couple badly assumed struct sizes, but I don't know if the struct is incorrectly defined or if the assert is wrong.
Quoted 19 lines
In the
nvme_nvm_identifyfunction we memcpy from the larger struct to the smaller struct. We incorrectly pass the length of the larger struct and overflow by 4 bytes, lets not do that.Signed-off-by Scott Bauer <scott.bauer at intel.com>--- drivers/nvme/host/lightnvm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/nvme/host/lightnvm.c +1
-1@@ -324,7 +324,7 @@ static int nvme_nvm_identity(struct nvm_dev *nvmdev, struct nvm_id *nvm_id) nvm_id->cap =le32_to_cpu (nvme_nvm_id->cap); nvm_id->dom =le32_to_cpu (nvme_nvm_id->dom);memcpy (&nvm_id->ppaf, &nvme_nvm_id->ppaf, -sizeof (struct nvme_nvm_addr_format )); +sizeof (struct nvm_addr_format ));
PATCH 1/1 nvme/lightnvm: Prevent small buffer overflow in nvme_nvm_identify
Quoted 16 lines · Keith Busch
+Matias
Quoted 9 lines · Scott Bauer
There are two closely named structs in lightnvm:
structnvme_nvm_addr_formatand
structnvme_addr_format.The first struct has 4 reserved bytes at the end, the second does not. (gdb) p
sizeof(struct nvme_nvm_addr_format)
$1 = 16
(gdb) psizeof(struct nvm_addr_format)
$2 = 12We need Matias to resolve what the size of these structs are supposed to be. There is a compile time assert:
Quoted 6 lines
sizeof(struct nvme_nvm_addr_format) == 128that should clearly fail, but the compiler removes
_nvme_nvm_check_sizesince it's not called from anywhere. lightnvm has a couple badly assumed struct sizes, but I don't know if the struct is incorrectly defined or if the assert is wrong.
Hi Scott,
You're right. I thought that was fixed a while ago.
The sizes are correct for both nvme_nvm_addr_format (the internal nvme structure that is described from the device), and the nvm_addr_format that is the host representation.
Luckily the memory is later overwritten by the group setup, and does not write into memory outside of lightnvm.
I'll post a patch that fixes up the asserts and pick up your patch for 4.12.
Thanks,
Matias
Quoted 20 lines
Quoted 20 lines
In the
nvme_nvm_identifyfunction we memcpy from the larger struct to the smaller struct. We incorrectly pass the length of the larger struct and overflow by 4 bytes, lets not do that.Signed-off-by Scott Bauer <scott.bauer at intel.com>--- drivers/nvme/host/lightnvm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/nvme/host/lightnvm.c +1
-1@@ -324,7 +324,7 @@ static int nvme_nvm_identity(struct nvm_dev *nvmdev, struct nvm_id *nvm_id) nvm_id->cap =le32_to_cpu (nvme_nvm_id->cap); nvm_id->dom =le32_to_cpu (nvme_nvm_id->dom);memcpy (&nvm_id->ppaf, &nvme_nvm_id->ppaf, -sizeof (struct nvme_nvm_addr_format )); +sizeof (struct nvm_addr_format ));