[lxc-devel] [lxc/master] storage/loop.c: integer overflow

[ljagiello on Github]

A non-text attachment was scrubbed...
Name: not available
Type: text/x-mailbox
Size: 1710 bytes
Desc: not available
URL: <http://lists.linuxcontainers.org/pipermail/lxc-devel/attachments/20180818/c5790344/attachment.bin>
-------------- next part --------------
From 087aab8fa9dd353c9018e977e8e79d1736996653 Mon Sep 17 00:00:00 2001
From: Lukasz Jagiello <lukasz at wikia-inc.com>
Date: Fri, 17 Aug 2018 21:11:02 -0700
Subject: [PATCH] storage/loop.c: integer overflow

The issue was introduced in PR (https://github.com/lxc/lxc/pull/1705):

Previous code:
```
  if (lseek(fd, size, SEEK_SET) < 0) {
    SYSERROR("Error seeking to set new loop file size");
    close(fd);
    return -1;
  }
```
New code:
```
  int fd, ret;

  [...]

  ret = lseek(fd, size, SEEK_SET);
  if (ret < 0) {
    SYSERROR("Failed to seek to set new loop file size for loop "
       "file \"%s\"", path);
    close(fd);
    return -1;
  }
```

Based on http://man7.org/linux/man-pages/man2/lseek.2.html:
> Upon successful completion, lseek() returns the resulting offset
> location as measured in bytes from the beginning of the file.

Because `size` is `uint64_t`, it's very easy to overflow `res` and as a
result the code will pass randomly (if overflow is >0 it will pass it
it's <0 it will fail).

Example from strace:
```
[...]
[pid  5561] lseek(4, 53687091200, SEEK_SET) = 53687091200
[pid  5561] close(4)                    = 0
[pid  5561] exit_group(1)               = ?
[pid  5561] +++ exited with 1 +++
[...]
```

There are many ways how this issue can be fixed. `ret` can be change
from `int` to `uint64_t`, another solution will be dedicated variable
for lseek return.

IMHO reverting syntax to the previous version seems to be the cleanest
solution. Let me know what works for you.

This PR fix issues (https://github.com/lxc/lxc/issues/1872).
---
 src/lxc/storage/loop.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/src/lxc/storage/loop.c b/src/lxc/storage/loop.c
index c4d393452..04101d9a8 100644
--- a/src/lxc/storage/loop.c
+++ b/src/lxc/storage/loop.c
@@ -307,8 +307,7 @@ static int do_loop_create(const char *path, uint64_t size, const char *fstype)
 		return -1;
 	}
 
-	ret = lseek(fd, size, SEEK_SET);
-	if (ret < 0) {
+	if (lseek(fd, size, SEEK_SET) < 0) {
 		SYSERROR("Failed to seek to set new loop file size for loop "
 			 "file \"%s\"", path);
 		close(fd);