Skip to content

Commit

Permalink
Fix various coverity issues.
Browse files Browse the repository at this point in the history
Mostly INTEGER_OVERFLOW (CWE-190).
  • Loading branch information
oniko committed May 3, 2024
1 parent 33e26be commit bede116
Show file tree
Hide file tree
Showing 8 changed files with 78 additions and 40 deletions.
2 changes: 1 addition & 1 deletion lib/crypto_backend/crypto_storage.c
Original file line number Diff line number Diff line change
Expand Up @@ -261,7 +261,7 @@ int crypt_storage_init(struct crypt_storage **ctx,
}

s->sector_size = sector_size;
s->iv_shift = large_iv ? int_log2(sector_size) - SECTOR_SHIFT : 0;
s->iv_shift = large_iv ? (unsigned)int_log2(sector_size) - SECTOR_SHIFT : 0;

*ctx = s;
return 0;
Expand Down
1 change: 1 addition & 0 deletions lib/crypto_backend/utf8.c
Original file line number Diff line number Diff line change
Expand Up @@ -230,6 +230,7 @@ static size_t utf16_encode_unichar(char16_t *out, char32_t c)
return 1;

case 0x10000U ... 0x10ffffU:
/* coverity[overflow_const:FALSE] */
c -= 0x10000U;
out[0] = htole16((c >> 10) + 0xd800U);
out[1] = htole16((c & 0x3ffU) + 0xdc00U);
Expand Down
4 changes: 4 additions & 0 deletions lib/luks2/luks2_luks1_convert.c
Original file line number Diff line number Diff line change
Expand Up @@ -621,6 +621,10 @@ int LUKS2_luks1_to_luks2(struct crypt_device *cd, struct luks_phdr *hdr1, struct
if (max_size < required_size)
max_size = required_size;

/* fix coverity false positive integer underflow */
if (max_size < 2 * LUKS2_HDR_16K_LEN)
return -EINVAL;

r = json_luks1_object(hdr1, &jobj, max_size - 2 * LUKS2_HDR_16K_LEN);
if (r < 0)
return r;
Expand Down
12 changes: 9 additions & 3 deletions lib/tcrypt/tcrypt.c
Original file line number Diff line number Diff line change
Expand Up @@ -1070,7 +1070,7 @@ uint64_t TCRYPT_get_iv_offset(struct crypt_device *cd,
struct tcrypt_phdr *hdr,
struct crypt_params_tcrypt *params)
{
uint64_t iv_offset;
uint64_t iv_offset, partition_offset;

if (params->mode && !strncmp(params->mode, "xts", 3))
iv_offset = TCRYPT_get_data_offset(cd, hdr, params);
Expand All @@ -1079,8 +1079,14 @@ uint64_t TCRYPT_get_iv_offset(struct crypt_device *cd,
else
iv_offset = hdr->d.mk_offset / SECTOR_SIZE;

if (params->flags & CRYPT_TCRYPT_SYSTEM_HEADER)
iv_offset += crypt_dev_partition_offset(device_path(crypt_data_device(cd)));
if (params->flags & CRYPT_TCRYPT_SYSTEM_HEADER) {
partition_offset = crypt_dev_partition_offset(device_path(crypt_data_device(cd)));
/* FIXME: we need to deal with overflow sooner */
if (iv_offset > (UINT64_MAX - partition_offset))
iv_offset = UINT64_MAX;
else
iv_offset += partition_offset;
}

return iv_offset;
}
Expand Down
29 changes: 15 additions & 14 deletions lib/utils_io.c
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
*/

#include <errno.h>
#include <limits.h>
#include <string.h>
#include <stdlib.h>
#include <stdint.h>
Expand All @@ -32,23 +33,23 @@
/* coverity[ -taint_source : arg-1 ] */
static ssize_t _read_buffer(int fd, void *buf, size_t length, volatile int *quit)
{
size_t read_size = 0;
ssize_t r;
ssize_t r, read_size = 0;

if (fd < 0 || !buf)
if (fd < 0 || !buf || length > SSIZE_MAX)
return -EINVAL;

do {
r = read(fd, buf, length - read_size);
if (r == -1 && errno != EINTR)
return r;
if (r > 0) {
read_size += (size_t)r;
/* coverity[overflow:FALSE] */
read_size += r;
buf = (uint8_t*)buf + r;
}
if (r == 0 || (quit && *quit))
return (ssize_t)read_size;
} while (read_size != length);
return read_size;
} while ((size_t)read_size != length);

return (ssize_t)length;
}
Expand All @@ -65,25 +66,25 @@ ssize_t read_buffer_intr(int fd, void *buf, size_t length, volatile int *quit)

static ssize_t _write_buffer(int fd, const void *buf, size_t length, volatile int *quit)
{
size_t write_size = 0;
ssize_t w;
ssize_t w, write_size = 0;

if (fd < 0 || !buf || !length)
if (fd < 0 || !buf || !length || length > SSIZE_MAX)
return -EINVAL;

do {
w = write(fd, buf, length - write_size);
w = write(fd, buf, length - (size_t)write_size);
if (w < 0 && errno != EINTR)
return w;
if (w > 0) {
write_size += (size_t) w;
/* coverity[overflow:FALSE] */
write_size += w;
buf = (const uint8_t*)buf + w;
}
if (w == 0 || (quit && *quit))
return (ssize_t)write_size;
} while (write_size != length);
return write_size;
} while ((size_t)write_size != length);

return (ssize_t)write_size;
return write_size;
}

ssize_t write_buffer(int fd, const void *buf, size_t length)
Expand Down
5 changes: 3 additions & 2 deletions lib/utils_keyring.c
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,7 @@ static key_serial_t find_key_by_type_and_desc(const char *type, const char *desc
char *newline;
size_t buffer_len = 0;

int n;
ssize_t n;

do {
id = request_key(type, desc, NULL, 0);
Expand All @@ -171,7 +171,8 @@ static key_serial_t find_key_by_type_and_desc(const char *type, const char *desc
return 0;

while ((n = read(f, buf + buffer_len, sizeof(buf) - buffer_len - 1)) > 0) {
buffer_len += n;
/* coverity[overflow:FALSE] */
buffer_len += (size_t)n;
buf[buffer_len] = '\0';
newline = strchr(buf, '\n');
while (newline != NULL && buffer_len != 0) {
Expand Down
13 changes: 8 additions & 5 deletions src/utils_password.c
Original file line number Diff line number Diff line change
Expand Up @@ -109,8 +109,10 @@ static int tools_check_password(const char *password)
static ssize_t read_tty_eol(int fd, char *pass, size_t maxlen)
{
bool eol = false;
size_t read_size = 0;
ssize_t r;
ssize_t r, read_size = 0;

if (maxlen > SSIZE_MAX)
return -EINVAL;

do {
r = read(fd, pass, maxlen - read_size);
Expand All @@ -119,12 +121,13 @@ static ssize_t read_tty_eol(int fd, char *pass, size_t maxlen)
if (r >= 0) {
if (!r || pass[r-1] == '\n')
eol = true;
read_size += (size_t)r;
/* coverity[overflow:FALSE] */
read_size += r;
pass = pass + r;
}
} while (!eol && read_size != maxlen);
} while (!eol && (size_t)read_size != maxlen);

return (ssize_t)read_size;
return read_size;
}

/* The pass buffer is zeroed and has trailing \0 already " */
Expand Down
52 changes: 37 additions & 15 deletions src/utils_reencrypt_luks1.c
Original file line number Diff line number Diff line change
Expand Up @@ -690,25 +690,31 @@ static int restore_luks_header(struct reenc_ctx *rc)
return r;
}

/* FIXME this function will be replaced with equivalent from lib/utils_io.c */
static ssize_t read_buf(int fd, void *buf, size_t count)
{
size_t read_size = 0;
ssize_t s;
ssize_t s, read_size = 0;

if (count > SSIZE_MAX)
return -EINVAL;

do {
/* This expects that partial read is aligned in buffer */
s = read(fd, buf, count - read_size);
if (s == -1 && errno != EINTR)
return s;
if (s == 0)
return (ssize_t)read_size;
return read_size;
if (s > 0) {
if (s != (ssize_t)count)
log_dbg("Partial read %zd / %zu.", s, count);
read_size += (size_t)s;
/* FIXME: temporarily silence coverity INTEGER_OVERFLOW (CWE-190) */
if (read_size > SSIZE_MAX - s)
return -1;
read_size += s;
buf = (uint8_t*)buf + s;
}
} while (read_size != count);
} while ((size_t)read_size != count);

return (ssize_t)count;
}
Expand All @@ -727,6 +733,10 @@ static int copy_data_forward(struct reenc_ctx *rc, int fd_old, int fd_new,
.device = tools_get_device_name(rc->device, &backing_file)
};

assert(rc);
assert(bytes);
assert(buf);

log_dbg("Reencrypting in forward direction.");

if (lseek(fd_old, rc->device_offset, SEEK_SET) < 0 ||
Expand All @@ -746,19 +756,18 @@ static int copy_data_forward(struct reenc_ctx *rc, int fd_old, int fd_new,
if ((rc->device_size - rc->device_offset) < (uint64_t)block_size)
block_size = rc->device_size - rc->device_offset;
s1 = read_buf(fd_old, buf, block_size);
if (s1 < 0 || ((size_t)s1 != block_size &&
(rc->device_offset + s1) != rc->device_size)) {
if (s1 < 0 || ((size_t)s1 != block_size)) {
log_dbg("Read error, expecting %zu, got %zd.",
block_size, s1);
goto out;
}

/* If device_size is forced, never write more than limit */
if ((s1 + rc->device_offset) > rc->device_size)
s1 = rc->device_size - rc->device_offset;

/*
* FIXME: this may fail with shorter write.
* Replace ir with write_buffer from lib/utils_io.c
*/
s2 = write(fd_new, buf, s1);
if (s2 < 0) {
if (s2 < 0 || s2 != s1) {
log_dbg("Write error, expecting %zu, got %zd.",
block_size, s2);
goto out;
Expand Down Expand Up @@ -840,6 +849,10 @@ static int copy_data_backward(struct reenc_ctx *rc, int fd_old, int fd_new,
goto out;
}

/*
* FIXME: this may fail with shorter write.
* Replace ir with write_buffer from lib/utils_io.c
*/
s2 = write(fd_new, buf, working_block);
if (s2 < 0) {
log_dbg("Write error, expecting %zu, got %zd.",
Expand Down Expand Up @@ -872,6 +885,11 @@ static void zero_rest_of_device(int fd, size_t block_size, void *buf,
{
ssize_t s1, s2;

assert(bytes);

if (*bytes > SSIZE_MAX || !block_size || block_size > SSIZE_MAX)
return;

log_dbg("Zeroing rest of device.");

if (lseek(fd, offset, SEEK_SET) < 0) {
Expand All @@ -884,10 +902,14 @@ static void zero_rest_of_device(int fd, size_t block_size, void *buf,

while (!quit && *bytes) {
if (*bytes < (uint64_t)s1)
s1 = *bytes;
s1 = (ssize_t)*bytes;

/*
* FIXME: this may fail with shorter write.
* Replace ir with write_buffer from lib/utils_io.c
*/
s2 = write(fd, buf, s1);
if (s2 != s1) {
if (s2 < 0 || s2 != s1) {
log_dbg("Write error, expecting %zd, got %zd.",
s1, s2);
return;
Expand All @@ -898,7 +920,7 @@ static void zero_rest_of_device(int fd, size_t block_size, void *buf,
return;
}

*bytes -= s2;
*bytes -= (uint64_t)s2;
}
}

Expand Down

0 comments on commit bede116

Please sign in to comment.