gguf : add input validation, prevent integer overflows

[ggerganov]

Input validation when reading GGUF files

Without these checks it is possible for crafted GGUF files to cause integer overflows, followed by heap overflows.

Thanks to Databricks team for responsibly reporting these

[slaren]

Not directly related to this, but shouldn't these cases be default: instead to detect invalid types above GGUF_TYPE_COUNT?

ggml/src/ggml.c

Lines 19360 to 19363 in 866cc68

	case GGUF_TYPE_COUNT: GGML_ASSERT(false && "invalid type"); break;
	}
	} break;
	case GGUF_TYPE_COUNT: GGML_ASSERT(false && "invalid type");

[slaren]

I think this may also cause a buffer overflow if n_dims > 4:

ggml/src/ggml.c

Lines 19391 to 19394 in 866cc68

	ok = ok && gguf_fread_el (file, &info->n_dims, sizeof(info->n_dims), &offset);
	for (uint32_t j = 0; j < info->n_dims; ++j) {
	ok = ok && gguf_fread_el(file, &info->ne[j], sizeof(info->ne[j]), &offset);
	}

[slaren]

This type is not checked and then later is used to call ggml functions such as ggml_type_name that may cause an overflow:

ggml/src/ggml.c

Line 19395 in 866cc68

ok = ok && gguf_fread_el (file, &info->type, sizeof(info->type), &offset);

[slaren]

There is no guarantee that the underlying type of the enum is unsigned, so I think it would be good to also check for type >= 0.

[neilarchibald]

Fixes look pretty good, I would also suggest checking the return of calloc/malloc for error, currently it will return a null pointer and crash.

[neilarchibald]

I just noticed you're using GGUF_TYPE_SIZE indexing using an arbitrary value read from the file. This could be used to index to a large value elsewhere and cause a wrap again, a check needs to be added when reading in the type value.

[slaren]

ggml/src/ggml.c

Lines 19439 to 19453 in fb345bd

	const int64_t ne =
	(int64_t) info->ne[0] *
	(int64_t) info->ne[1] *
	(int64_t) info->ne[2] *
	(int64_t) info->ne[3];
	if (ne % ggml_blck_size(info->type) != 0) {
	fprintf(stderr, "%s: tensor '%s' of type %d (%s) number of elements (%" PRId64 ") is not a multiple of block size (%d)\n",
	__func__, info->name.data, (int)info->type, ggml_type_name(info->type), ne, ggml_blck_size(info->type));
	fclose(file);
	gguf_free(ctx);
	return NULL;
	}
	const size_t size_cur = ggml_row_size(info->type, ne);

There are a few potential integer overflows here when computing the size, and this size may be later used to allocate a buffer, which then may result in a buffer overflow when reading the data.

[slaren]

ggml/src/ggml.c

Line 19400 in fb345bd

ok = ok && gguf_fread_el (file, &info->offset, sizeof(info->offset), &offset);

We should probably check that the offset and the end of the tensor is within the size of the file.

[slaren]

ggml/src/ggml.c

Line 19399 in fb345bd

ok = ok && gguf_fread_el (file, &info->type, sizeof(info->type), &offset);

We should check that the type is valid.

[slaren]

ggml/src/ggml.c

Lines 19466 to 19469 in fb345bd

	const size_t mem_size =
	params.no_alloc ?
	(ctx->header.n_tensors )*ggml_tensor_overhead() :
	(ctx->header.n_tensors + 1)*ggml_tensor_overhead() + ctx->size;

It seem unlikely to happen in practice, even with no_alloc a large enough n_tensors could cause an integer overflow. It would probably be good to wrap all the integer operations in functions that check for overflows.

[slaren]

ggml/src/ggml.c

Line 19288 in fb345bd

ctx->kv = malloc(ctx->header.n_kv * sizeof(struct gguf_kv));

This is another potential integer overflow that may cause a the buffer to be too small.

[ggerganov]

Think I covered all of the listed concerns. Will merge this and if you spot something else, directly open a PR. Thanks