Skip to contentSkip to conversation

NVMe: check for integer overflow in nvme_map_user_pages()

PATCH In review Dan Carpenter 3 participants 6 messages May 11, 2013 – May 17, 2013

Subsystems (3)
Block layer Kernel janitors NVMe
lore
Share & tools
lore

Dan Carpenter

author
#0 lore raw

PATCH 1/1 NVMe: check for integer overflow in nvme_map_user_pages()

You need to have CAP_SYS_ADMIN to trigger this overflow but it makes the static checkers complain so we should fix it. The worry is that "length" comes from copy_from_user() so we need to check that "length + offset" can't overflow.

I also changed the min_t() cast to be unsigned instead of signed. Now that we cap "length" to INT_MAX it doesn't make a difference, but it's a little easier for reviewers to know that large values aren't cast to negative.

Signed-off-by Dan Carpenter <[email protected]>
drivers/block/nvme-core.c +3 -2
index 8efdfaa..4376375 100644
@@ -1206,7 +1206,7 @@ struct nvme_iod *nvme_map_user_pages(struct nvme_dev *dev, int write,
 
 	if (addr & 3)
 		return ERR_PTR(-EINVAL);
-	if (!length)
+	if (!length || length > INT_MAX - PAGE_SIZE)
 		return ERR_PTR(-EINVAL);
 
 	offset = offset_in_page(addr);

@@ -1227,7 +1227,8 @@ struct nvme_iod *nvme_map_user_pages(struct nvme_dev *dev, int write,
 	sg_init_table(sg, count);
 	for (i = 0; i < count; i++) {
 		sg_set_page(&sg[i], pages[i],
-				min_t(int, length, PAGE_SIZE - offset), offset);
+			    min_t(unsigned, length, PAGE_SIZE - offset),
+			    offset);
 		length -= (PAGE_SIZE - offset);
 		offset = 0;
 	}
#1 lore raw

Why didn't you send this patch to the linux-nvme mailing list or the address listed in MAINTAINERS? Now Exchange & Outlook have hold of it, and it's mangled.

NVM EXPRESS DRIVER

M:      Matthew Wilcox <[email protected]>
L:      [email protected]

________________________________________
From: Dan Carpenter [[email protected]]
Sent: May 11, 2013 9:31 AM
To: Wilcox, Matthew R
Cc: Busch, Keith; Verma, Vishal L; [email protected]; [email protected]
Subject: [patch] NVMe: check for integer overflow in nvme_map_user_pages()

You need to have CAP_SYS_ADMIN to trigger this overflow but it makes the static checkers complain so we should fix it. The worry is that "length" comes from copy_from_user() so we need to check that "length + offset" can't overflow.

I also changed the min_t() cast to be unsigned instead of signed. Now that we cap "length" to INT_MAX it doesn't make a difference, but it's a little easier for reviewers to know that large values aren't cast to negative.

Signed-off-by Dan Carpenter <[email protected]>
drivers/block/nvme-core.c +3 -2
index 8efdfaa..4376375 100644
@@ -1206,7 +1206,7 @@ struct nvme_iod *nvme_map_user_pages(struct nvme_dev *dev, int write,
 
        if (addr & 3)
                return ERR_PTR(-EINVAL);
-       if (!length)
+       if (!length || length > INT_MAX - PAGE_SIZE)
                return ERR_PTR(-EINVAL);
 
        offset = offset_in_page(addr);

@@ -1227,7 +1227,8 @@ struct nvme_iod *nvme_map_user_pages(struct nvme_dev *dev, int write,
        sg_init_table(sg, count);
        for (i = 0; i < count; i++) {
                sg_set_page(&sg[i], pages[i],
-                               min_t(int, length, PAGE_SIZE - offset), offset);
+                           min_t(unsigned, length, PAGE_SIZE - offset),
+                           offset);
                length -= (PAGE_SIZE - offset);
                offset = 0;
        }
author
#2 lore raw

PATCH 1/1 NVMe: check for integer overflow in nvme_map_user_pages()

You need to have CAP_SYS_ADMIN to trigger this overflow but it makes the static checkers complain so we should fix it. The worry is that "length" comes from copy_from_user() so we need to check that "length + offset" can't overflow.

I also changed the min_t() cast to be unsigned instead of signed. Now that we cap "length" to INT_MAX it doesn't make a difference, but it's a little easier for reviewers to know that large values aren't cast to negative.

Signed-off-by Dan Carpenter <[email protected]>

---
Resending to the [email protected] email address.

drivers/block/nvme-core.c +3 -2
index 8efdfaa..4376375 100644
@@ -1206,7 +1206,7 @@ struct nvme_iod *nvme_map_user_pages(struct nvme_dev *dev, int write,
 
 	if (addr & 3)
 		return ERR_PTR(-EINVAL);
-	if (!length)
+	if (!length || length > INT_MAX - PAGE_SIZE)
 		return ERR_PTR(-EINVAL);
 
 	offset = offset_in_page(addr);

@@ -1227,7 +1227,8 @@ struct nvme_iod *nvme_map_user_pages(struct nvme_dev *dev, int write,
 	sg_init_table(sg, count);
 	for (i = 0; i < count; i++) {
 		sg_set_page(&sg[i], pages[i],
-				min_t(int, length, PAGE_SIZE - offset), offset);
+			    min_t(unsigned, length, PAGE_SIZE - offset),
+			    offset);
 		length -= (PAGE_SIZE - offset);
 		offset = 0;
 	}
Applied
#3 lore raw
Quoted 11 lines · Dan Carpenter

You need to have CAP_SYS_ADMIN to trigger this overflow but it makes the static checkers complain so we should fix it. The worry is that "length" comes from copy_from_user() so we need to check that "length + offset" can't overflow.

I also changed the min_t() cast to be unsigned instead of signed. Now that we cap "length" to INT_MAX it doesn't make a difference, but it's a little easier for reviewers to know that large values aren't cast to negative.

Signed-off-by Dan Carpenter <[email protected]>

Applied

author
#4 lore raw

PATCH 1/1 MAINTAINERS: update NVM EXPRESS DRIVER file list

In reply to Wilcox, Matthew R

There isn't an nvme.c file any more. It has been split into multiple files.

Signed-off-by Dan Carpenter <[email protected]>
MAINTAINERS +1 -1
index 8f190f5..97ec343 100644
@@ -5747,7 +5747,7 @@ M:	Matthew Wilcox <[email protected]>
 L:	[email protected]
 T:	git git://git.infradead.org/users/willy/linux-nvme.git
 S:	Supported
-F:	drivers/block/nvme.c
+F:	drivers/block/nvme*
 F:	include/linux/nvme.h
 
 OMAP SUPPORT