Skip to contentSkip to conversation

Verify memory sizes on 32 bit systems.

PATCH In review Tobias Stoeckmann 2 participants 5 messages Jul 30, 2018 – Aug 3, 2018

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 Verify memory sizes on 32 bit systems.

Large file system support is activated by default, which means that on 32 bit systems, off_t is 64 bit in size.

Using st.st_size or any other 64 bit variable with mmap can lead to integer truncation and therefore insufficient memory mapping.

---
 libkmod/libkmod-file.c | 2 ++
 1 file changed, 2 insertions(+)
libkmod/libkmod-file.c +2 -0
index 5eeba6a..86f34c6 100644
@@ -255,6 +255,8 @@ static int load_reg(struct kmod_file *file)
 		return -errno;
 
 	file->size = st.st_size;
+	if ((uintmax_t)st.st_size > (uintmax_t)SIZE_MAX)
+		return -EFBIG;
 	file->memory = mmap(NULL, file->size, PROT_READ, MAP_PRIVATE,
 			    file->fd, 0);
 	if (file->memory == MAP_FAILED)
Signature · 1 line
2.13.2
#1 lore raw
Quoted 17 lines · Tobias Stoeckmann

Large file system support is activated by default, which means that on 32 bit systems, off_t is 64 bit in size.

Using st.st_size or any other 64 bit variable with mmap can lead to integer truncation and therefore insufficient memory mapping.

---
 libkmod/libkmod-file.c | 2 ++
 1 file changed, 2 insertions(+)
libkmod/libkmod-file.c +0 -0
index 5eeba6a..86f34c6 100644

libkmod/libkmod-file.c static int load_reg(struct kmod_file *file) · line 258

Quoted 4 lines
libkmod/libkmod-file.c +1 -0
@@ -255,6 +255,8 @@ static int load_reg(struct kmod_file *file)
                return -errno;
 
        file->size = st.st_size;
+       if ((uintmax_t)st.st_size > (uintmax_t)SIZE_MAX)

why casting?

Lucas De Marchi

Quoted 11 lines
libkmod/libkmod-file.c +1 -1
+               return -EFBIG;
        file->memory = mmap(NULL, file->size, PROT_READ, MAP_PRIVATE,
                            file->fd, 0);
        if (file->memory == MAP_FAILED)
--

2.13.2

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

Signature · 1 line
Lucas De Marchi
#2 lore raw
Quoted 3 lines · Lucas De Marchi
Quoted 1 line

+ if ((uintmax_t)st.st_size > (uintmax_t)SIZE_MAX)

why casting?

Without casting, a warning could be triggered on 64 bit systems, because on these systems a signed 64 bit int (off_t) is compared with an unsigned 64 bit int (size_t) which could never be larger.

Casting it to uintmax_t, which is at least 64 bits in size, silences this error.

No technical reason beyond that.

Tobias

#3 lore raw
Quoted 4 lines · Tobias Stoeckmann
Quoted 2 lines · Lucas De Marchi
Quoted 1 line

+ if ((uintmax_t)st.st_size > (uintmax_t)SIZE_MAX)

Quoted 8 lines
Quoted 1 line

why casting?

Without casting, a warning could be triggered on 64 bit systems, because on these systems a signed 64 bit int (off_t) is compared with an unsigned 64 bit int (size_t) which could never be larger.

Casting it to uintmax_t, which is at least 64 bits in size, silences this error.

so you are casting the *sign* away, ok. But casting only st.st_size should suffice.

Lucas De Marchi

Quoted 5 lines

No technical reason beyond that.

Tobias

Signature · 1 line
Lucas De Marchi
#4 lore raw
Quoted 2 lines · Lucas De Marchi

so you are casting the *sign* away, ok. But casting only st.st_size should suffice.

I've just tried with some examples and I couldn't trigger the warning which I have seen in other situations like this, i.e. "always false due to limited range of data type".

If I cast the off_t to size_t, the compiler knows that it is not possible that it could ever be larger than SIZE_MAX, optimizing the block away. So I took uintmax_t.

But as long as there's no annoying warning, I am fine with just casting st_size.

Tobias