misc: mic/scif: fix wrap around tests

PATCH In review Dan Carpenter 2 participants 5 messages Oct 9, 2015 – Oct 14, 2015

Subsystems (2)

Kernel janitors Char and misc drivers

lore

Share & tools

lore

Dan Carpenter

author

2015-10-09 06:40 UTC #0 lore raw

PATCH 2/2 misc: mic/scif: fix wrap around tests

Signed integer overflow is undefined. Also I added a check for "(offset < 0)" in scif_unregister() because that makes it match the other conditions and because I didn't want to subtract a negative.

Fixes ba612aa8b487 ('misc: mic: SCIF memory registration and unregistration')

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

---

Imagine you are on 64 bit and len is larger than INT_MAX << 12, it means
that we truncate it because scif_get_window_offset() takes an integer
argument. I don't know if this is an issue. Maybe I should use
INT_MAX instead of LONG_MAX? I am working on a static checker warning
for these types of issues:
drivers/misc/mic/scif/scif_rma.c:1631 scif_register() warn: truncating user data 'len >> 12' '0-4503599627370495' drivers/misc/mic/scif/scif_rma.c:1643 scif_register() warn: truncating user data 'len >> 12' '0-4503599627370495'

The other static warnings here are:

drivers/misc/mic/scif/scif_rma.c:745 scif_unregister_window() warn: inconsistent returns 'mutex:&ep->rma_info.rma_lock'.
Locked on: line 745
Unlocked on: line 687
drivers/misc/mic/scif/scif_rma.c:1463 scif_unpin_pages() warn: passing __func__ while the format string already contains the name of the function 'scif_unpin_pages'

drivers/misc/mic/scif/scif_rma.c +4 -3

index bc2dccb..fea7d2c 100644
@@ -1510,7 +1510,7 @@ off_t scif_register_pinned_pages(scif_epd_t epd,
 	if ((map_flags & SCIF_MAP_FIXED) &&
 	    ((ALIGN(offset, PAGE_SIZE) != offset) ||
 	    (offset < 0) ||
-	    (offset + (off_t)len < offset)))
+	    (len > LONG_MAX - offset)))
 		return -EINVAL;
 
 	might_sleep();

@@ -1613,7 +1613,7 @@ off_t scif_register(scif_epd_t epd, void *addr, size_t len, off_t offset,
 	if ((map_flags & SCIF_MAP_FIXED) &&
 	    ((ALIGN(offset, PAGE_SIZE) != offset) ||
 	    (offset < 0) ||
-	    (offset + (off_t)len < offset)))
+	    (len < LONG_MAX - offset)))
 		return -EINVAL;
 
 	/* Unsupported protection requested */

@@ -1731,7 +1731,8 @@ scif_unregister(scif_epd_t epd, off_t offset, size_t len)
 
 	/* Offset is not page aligned or offset+len wraps around */
 	if ((ALIGN(offset, PAGE_SIZE) != offset) ||
-	    (offset + (off_t)len < offset))
+	    (offset < 0) ||
+	    (len > LONG_MAX - offset))
 		return -EINVAL;
 
 	err = scif_verify_epd(ep);

Sudeep Dutt

2015-10-11 09:14 UTC #1 lore raw

Quoted 11 lines · Dan Carpenter

Signed integer overflow is undefined. Also I added a check for "(offset < 0)" in scif_unregister() because that makes it match the other conditions and because I didn't want to subtract a negative.
Fixes ba612aa8b487 ('misc: mic: SCIF memory registration and unregistration')
Signed-off-by Dan Carpenter <[email protected]>
---
Imagine you are on 64 bit and len is larger than INT_MAX << 12, it means that we truncate it because scif_get_window_offset() takes an integer argument. I don't know if this is an issue.

scif_get_window_offset(..) takes an integer argument for the number of pages. We believe that an int for number of 4K pages is sufficient for current systems. I don't think there is an issue here.

Quoted 11 lines

Maybe I should use
INT_MAX instead of LONG_MAX? I am working on a static checker warning
for these types of issues:
drivers/misc/mic/scif/scif_rma.c:1631 scif_register() warn: truncating user data 'len >> 12' '0-4503599627370495' drivers/misc/mic/scif/scif_rma.c:1643 scif_register() warn: truncating user data 'len >> 12' '0-4503599627370495'
The other static warnings here are:
drivers/misc/mic/scif/scif_rma.c:745 scif_unregister_window() warn: inconsistent returns 'mutex:&ep->rma_info.rma_lock'.
Locked on: line 745
Unlocked on: line 687

The function expects the lock to be held by the caller so there is no issue here.

Quoted 2 lines

drivers/misc/mic/scif/scif_rma.c:1463 scif_unpin_pages() warn: passing __func__ while the format string already contains the name of the function 'scif_unpin_pages'

It might be useful to enhance checkpatch to catch such issues.

Quoted 14 lines

drivers/misc/mic/scif/scif_rma.c +1 -1

index bc2dccb..fea7d2c 100644
@@ -1510,7 +1510,7 @@ off_t scif_register_pinned_pages(scif_epd_t epd,
 	if ((map_flags & SCIF_MAP_FIXED) &&
 	    ((ALIGN(offset, PAGE_SIZE) != offset) ||
 	    (offset < 0) ||
-	    (offset + (off_t)len < offset)))
+	    (len > LONG_MAX - offset)))
 		return -EINVAL;
 
 	might_sleep();

drivers/misc/mic/scif/scif_rma.c off_t scif_register(scif_epd_t epd, void *addr, size_t len, off_t offset, · line 1616

Quoted 5 lines

drivers/misc/mic/scif/scif_rma.c +1 -1

@@ -1613,7 +1613,7 @@ off_t scif_register(scif_epd_t epd, void *addr, size_t len, off_t offset,
 	if ((map_flags & SCIF_MAP_FIXED) &&
 	    ((ALIGN(offset, PAGE_SIZE) != offset) ||
 	    (offset < 0) ||
-	    (offset + (off_t)len < offset)))
+	    (len < LONG_MAX - offset)))

Why is this change required? The earlier code was being used to detect wraparound and I think it works fine.

Thanks,
Sudeep Dutt

Dan Carpenter

author

2015-10-13 12:51 UTC #2 lore raw

Quoted 9 lines · Sudeep Dutt

Quoted 8 lines · Dan Carpenter
Signed integer overflow is undefined. Also I added a check for "(offset < 0)" in scif_unregister() because that makes it match the other conditions and because I didn't want to subtract a negative.
Fixes ba612aa8b487 ('misc: mic: SCIF memory registration and unregistration')
Signed-off-by Dan Carpenter <[email protected]>
---

Quoted 7 lines

Quoted 3 lines
Imagine you are on 64 bit and len is larger than INT_MAX << 12, it means that we truncate it because scif_get_window_offset() takes an integer argument. I don't know if this is an issue.
scif_get_window_offset(..) takes an integer argument for the number of pages. We believe that an int for number of 4K pages is sufficient for current systems. I don't think there is an issue here.

The issue isn't that we need more pages, it's that we can overflow INT_MAX so scif_get_window_offset() succeeds when it should fail.

Quoted 7 lines

Quoted 7 lines
Maybe I should use
INT_MAX instead of LONG_MAX? I am working on a static checker warning
for these types of issues:
drivers/misc/mic/scif/scif_rma.c:1631 scif_register() warn: truncating user data 'len >> 12' '0-4503599627370495' drivers/misc/mic/scif/scif_rma.c:1643 scif_register() warn: truncating user data 'len >> 12' '0-4503599627370495'

Quoted 9 lines

Quoted 5 lines
The other static warnings here are:
drivers/misc/mic/scif/scif_rma.c:745 scif_unregister_window() warn: inconsistent returns 'mutex:&ep->rma_info.rma_lock'.
Locked on: line 745
Unlocked on: line 687
The function expects the lock to be held by the caller so there is no issue here.

You're missing the point. Never mind, I will send a patch for this.

Quoted 9 lines

drivers/misc/mic/scif/scif_rma.c off_t scif_register(scif_epd_t epd, void *addr, size_t len, off_t offset, · line 1616
Quoted 6 lines
drivers/misc/mic/scif/scif_rma.c +1 -1
@@ -1613,7 +1613,7 @@ off_t scif_register(scif_epd_t epd, void *addr, size_t len, off_t offset,
 	if ((map_flags & SCIF_MAP_FIXED) &&
 	    ((ALIGN(offset, PAGE_SIZE) != offset) ||
 	    (offset < 0) ||
-	    (offset + (off_t)len < offset)))
+	    (len < LONG_MAX - offset)))
Why is this change required? The earlier code was being used to detect wraparound and I think it works fine.

It doesn't work. off_t is a signed type so it's undefined. The compiler will often just remove the condition.

regards,
dan carpenter

Sudeep Dutt

2015-10-14 03:21 UTC #3 lore raw

In reply to Dan Carpenter

Quoted 37 lines · Dan Carpenter

Signed integer overflow is undefined. Also I added a check for "(offset < 0)" in scif_unregister() because that makes it match the other conditions and because I didn't want to subtract a negative.
Fixes ba612aa8b487 ('misc: mic: SCIF memory registration and unregistration')
Signed-off-by Dan Carpenter <[email protected]>
---
Imagine you are on 64 bit and len is larger than INT_MAX << 12, it means
that we truncate it because scif_get_window_offset() takes an integer
argument. I don't know if this is an issue. Maybe I should use
INT_MAX instead of LONG_MAX? I am working on a static checker warning
for these types of issues:
drivers/misc/mic/scif/scif_rma.c:1631 scif_register() warn: truncating user data 'len >> 12' '0-4503599627370495' drivers/misc/mic/scif/scif_rma.c:1643 scif_register() warn: truncating user data 'len >> 12' '0-4503599627370495'
The other static warnings here are:
drivers/misc/mic/scif/scif_rma.c:745 scif_unregister_window() warn: inconsistent returns 'mutex:&ep->rma_info.rma_lock'.
Locked on: line 745
Unlocked on: line 687
drivers/misc/mic/scif/scif_rma.c:1463 scif_unpin_pages() warn: passing __func__ while the format string already contains the name of the function 'scif_unpin_pages'
drivers/misc/mic/scif/scif_rma.c +1 -1
index bc2dccb..fea7d2c 100644
@@ -1510,7 +1510,7 @@ off_t scif_register_pinned_pages(scif_epd_t epd,
 	if ((map_flags & SCIF_MAP_FIXED) &&
 	    ((ALIGN(offset, PAGE_SIZE) != offset) ||
 	    (offset < 0) ||
-	    (offset + (off_t)len < offset)))
+	    (len > LONG_MAX - offset)))
 		return -EINVAL;
 
 	might_sleep();

drivers/misc/mic/scif/scif_rma.c off_t scif_register(scif_epd_t epd, void *addr, size_t len, off_t offset, · line 1616

Quoted 5 lines

drivers/misc/mic/scif/scif_rma.c +1 -1

@@ -1613,7 +1613,7 @@ off_t scif_register(scif_epd_t epd, void *addr, size_t len, off_t offset,
 	if ((map_flags & SCIF_MAP_FIXED) &&
 	    ((ALIGN(offset, PAGE_SIZE) != offset) ||
 	    (offset < 0) ||
-	    (offset + (off_t)len < offset)))
+	    (len < LONG_MAX - offset)))

Hi Dan,
Should this be > instead of < like the others?
Thanks,
Sudeep Dutt

Quoted 13 lines

drivers/misc/mic/scif/scif_rma.c +2 -1

 		return -EINVAL;
 
 	/* Unsupported protection requested */

@@ -1731,7 +1731,8 @@ scif_unregister(scif_epd_t epd, off_t offset, size_t len)
 
 	/* Offset is not page aligned or offset+len wraps around */
 	if ((ALIGN(offset, PAGE_SIZE) != offset) ||
-	    (offset + (off_t)len < offset))
+	    (offset < 0) ||
+	    (len > LONG_MAX - offset))
 		return -EINVAL;
 
 	err = scif_verify_epd(ep);

Dan Carpenter

author

2015-10-14 18:05 UTC #4 lore raw

Quoted 9 lines · Sudeep Dutt

drivers/misc/mic/scif/scif_rma.c off_t scif_register(scif_epd_t epd, void *addr, size_t len, off_t offset, · line 1616
Quoted 6 lines
drivers/misc/mic/scif/scif_rma.c +1 -1
@@ -1613,7 +1613,7 @@ off_t scif_register(scif_epd_t epd, void *addr, size_t len, off_t offset,
 	if ((map_flags & SCIF_MAP_FIXED) &&
 	    ((ALIGN(offset, PAGE_SIZE) != offset) ||
 	    (offset < 0) ||
-	    (offset + (off_t)len < offset)))
+	    (len < LONG_MAX - offset)))
Hi Dan,
Should this be > instead of < like the others?

Gar. You're right, obviously. Thanks. I will resend tomorrow.

regards,
dan carpenter