Skip to contentSkip to conversation

poll/select: avoid arithmetic overflow in __estimate_accuracy()

PATCH In review Guillaume Knispel 2 participants 3 messages Aug 16, 2009 – Aug 17, 2009 VFS

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 poll/select: avoid arithmetic overflow in __estimate_accuracy()

__estimate_accuracy() was prone to integer overflow, for example if *tv == {2147, 483648000} on a 32 bit computer (or even for delays as small as {429, 500000000} if the task is niced).

Because the result was already forced between 0 and 100ms, the effect of the overflow was not too problematic, but the use of the hrtimer range feature was not optimal in overflow cases.

This patch ensures that there can not be an integer overflow in this function.

---
 fs/select.c |   14 ++++++++++----
 1 files changed, 10 insertions(+), 4 deletions(-)
fs/select.c +10 -4
index 8084834..a201fc3 100644
@@ -41,22 +41,28 @@
  * better solutions..
  */
 
+#define MAX_SLACK	(100 * NSEC_PER_MSEC)
+
 static long __estimate_accuracy(struct timespec *tv)
 {
 	long slack;
 	int divfactor = 1000;
 
+	if (tv->tv_sec < 0)
+		return 0;
+
 	if (task_nice(current) > 0)
 		divfactor = divfactor / 5;
 
+	if (tv->tv_sec > MAX_SLACK / (NSEC_PER_SEC/divfactor))
+		return MAX_SLACK;
+
 	slack = tv->tv_nsec / divfactor;
 	slack += tv->tv_sec * (NSEC_PER_SEC/divfactor);
 
-	if (slack > 100 * NSEC_PER_MSEC)
-		slack =  100 * NSEC_PER_MSEC;
+	if (slack > MAX_SLACK)
+		return MAX_SLACK;
 
-	if (slack < 0)
-		slack = 0;
 	return slack;
 }
 
#1 lore raw
Quoted 30 lines · Guillaume Knispel

__estimate_accuracy() was prone to integer overflow, for example if *tv == {2147, 483648000} on a 32 bit computer (or even for delays as small as {429, 500000000} if the task is niced).

Because the result was already forced between 0 and 100ms, the effect of the overflow was not too problematic, but the use of the hrtimer range feature was not optimal in overflow cases.

This patch ensures that there can not be an integer overflow in this function.

---
fs/select.c | 14 ++++++++++----

1 files changed, 10 insertions(+), 4 deletions(-)
fs/select.c +2 -0
index 8084834..a201fc3 100644
@@ -41,22 +41,28 @@
 * better solutions..
 */
 
+#define MAX_SLACK	(100 * NSEC_PER_MSEC)
+
 static long __estimate_accuracy(struct timespec *tv)
 {
 	long slack;
 	int divfactor = 1000;

fs/select.c line 59

Quoted 10 lines
fs/select.c +6 -0
 
+	if (tv->tv_sec < 0)
+		return 0;
+
 	if (task_nice(current) > 0)
 		divfactor = divfactor / 5;
 
+	if (tv->tv_sec > MAX_SLACK / (NSEC_PER_SEC/divfactor))
+		return MAX_SLACK;
+

Yes? Isn't MAX_SLACK also too big for 'long' on 32bit machine?

Try:

% printf "%d\n" "100*1000000000 > 0x7fffffff"
1

Am I missing something here?

Thanks.

Quoted 18 lines
fs/select.c +2 -5
 	slack = tv->tv_nsec / divfactor;
 	slack += tv->tv_sec * (NSEC_PER_SEC/divfactor);
 
-	if (slack > 100 * NSEC_PER_MSEC)
-		slack =  100 * NSEC_PER_MSEC;
+	if (slack > MAX_SLACK)
+		return MAX_SLACK;
 
-	if (slack < 0)
-		slack = 0;
 	return slack;
 }
 
--

To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

#2 lore raw
Quoted 45 lines · Guillaume Knispel
Quoted 30 lines

__estimate_accuracy() was prone to integer overflow, for example if *tv == {2147, 483648000} on a 32 bit computer (or even for delays as small as {429, 500000000} if the task is niced).

Because the result was already forced between 0 and 100ms, the effect of the overflow was not too problematic, but the use of the hrtimer range feature was not optimal in overflow cases.

This patch ensures that there can not be an integer overflow in this function.

---
fs/select.c | 14 ++++++++++----

1 files changed, 10 insertions(+), 4 deletions(-)
fs/select.c +2 -0
index 8084834..a201fc3 100644
@@ -41,22 +41,28 @@
 * better solutions..
 */
 
+#define MAX_SLACK	(100 * NSEC_PER_MSEC)
+
 static long __estimate_accuracy(struct timespec *tv)
 {
 	long slack;
 	int divfactor = 1000;

fs/select.c line 59

Quoted 10 lines
fs/select.c +6 -0
 
+	if (tv->tv_sec < 0)
+		return 0;
+
 	if (task_nice(current) > 0)
 		divfactor = divfactor / 5;
 
+	if (tv->tv_sec > MAX_SLACK / (NSEC_PER_SEC/divfactor))
+		return MAX_SLACK;
+

Yes? Isn't MAX_SLACK also too big for 'long' on 32bit machine?

Quoted 6 lines

Try:

% printf "%d\n" "100*1000000000 > 0x7fffffff"
1

Am I missing something here?

100 * NSEC_PER_MSEC => 100 * 1000000

               -

The exact same multiplication was already in the existing code, and this is not the one that can overflow.

Cheers,
Guillaume Knispel