>
> RedMemSlotInfo keeps an array of groups, and each group contains an
> array of slots. Unfortunately, these checks are off by 1, they check
> that the index is greater or equal to the number of elements in the
> array, while these arrays are 0 based. The check should only check for
> strictly greater than the number of elements.
>
> For the group array, this is not a big issue, as these memslot groups
> are created by spice-server users (eg QEMU), and the group ids used to
> index that array are also generated by the spice-server user, so it
> should not be possible for the guest to set them to arbitrary values.
>
> The slot id is more problematic, as it's calculated from a QXLPHYSICAL
> address, and such addresses are usually set by the guest QXL driver, so
> the guest can set these to arbitrary values, including malicious values,
> which are probably easy to build from the guest PCI configuration.
>
> This patch fixes the arrays bound check, and adds a test case for this.
> This fixes CVE-2019-3813.
>
> Signed-off-by: Christophe Fergeau <cfergeau@xxxxxxxxxx>
Acked-by: Frediano Ziglio <fziglio@xxxxxxxxxx>
as far as I know I already acked it for the private build.
> ---
> Forgot to send this for upstream when the CVE went public :-/
>
> server/memslot.c | 4 ++--
> server/tests/test-qxl-parsing.c | 30 ++++++++++++++++++++++++++++++
> 2 files changed, 32 insertions(+), 2 deletions(-)
>
> diff --git a/server/memslot.c b/server/memslot.c
> index c29313214..97311b2e5 100644
> --- a/server/memslot.c
> +++ b/server/memslot.c
> @@ -97,13 +97,13 @@ void *memslot_get_virt(RedMemSlotInfo *info, QXLPHYSICAL
> addr, uint32_t add_size
>
> MemSlot *slot;
>
> - if (group_id > info->num_memslots_groups) {
> + if (group_id >= info->num_memslots_groups) {
> spice_critical("group_id too big");
> return NULL;
> }
>
> slot_id = memslot_get_id(info, addr);
> - if (slot_id > info->num_memslots) {
> + if (slot_id >= info->num_memslots) {
> print_memslots(info);
> spice_critical("slot_id %d too big, addr=%" PRIx64, slot_id, addr);
> return NULL;
> diff --git a/server/tests/test-qxl-parsing.c
> b/server/tests/test-qxl-parsing.c
> index 324f7fdc9..edccfee47 100644
> --- a/server/tests/test-qxl-parsing.c
> +++ b/server/tests/test-qxl-parsing.c
> @@ -85,6 +85,31 @@ static void deinit_qxl_surface(QXLSurfaceCmd *qxl)
> g_free(from_physical(qxl->u.surface_create.data));
> }
>
> +static void test_memslot_invalid_group_id(void)
> +{
> + RedMemSlotInfo mem_info;
> + init_meminfo(&mem_info);
> +
> + memslot_get_virt(&mem_info, 0, 16, 1);
> +}
> +
> +static void test_memslot_invalid_slot_id(void)
> +{
> + RedMemSlotInfo mem_info;
> + init_meminfo(&mem_info);
> +
> + memslot_get_virt(&mem_info, 1 << mem_info.memslot_id_shift, 16, 0);
> +}
> +
> +static void test_memslot_invalid_addresses(void)
> +{
> +
> g_test_trap_subprocess("/server/memslot-invalid-addresses/subprocess/group_id",
> 0, 0);
> + g_test_trap_assert_stderr("*group_id too big*");
> +
> +
> g_test_trap_subprocess("/server/memslot-invalid-addresses/subprocess/slot_id",
> 0, 0);
> + g_test_trap_assert_stderr("*slot_id 1 too big*");
> +}
> +
> static void test_no_issues(void)
> {
> RedMemSlotInfo mem_info;
> @@ -269,6 +294,11 @@ int main(int argc, char *argv[])
> {
> g_test_init(&argc, &argv, NULL);
>
> + /* try to use invalid memslot group/slot */
> + g_test_add_func("/server/memslot-invalid-addresses",
> test_memslot_invalid_addresses);
> + g_test_add_func("/server/memslot-invalid-addresses/subprocess/group_id",
> test_memslot_invalid_group_id);
> + g_test_add_func("/server/memslot-invalid-addresses/subprocess/slot_id",
> test_memslot_invalid_slot_id);
> +
> /* try to create a surface with no issues, should succeed */
> g_test_add_func("/server/qxl-parsing-no-issues", test_no_issues);
>
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel