From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wm1-f43.google.com (mail-wm1-f43.google.com [209.85.128.43]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 1FD1E21500E for ; Tue, 12 Nov 2024 11:17:48 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.128.43 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1731410270; cv=none; b=U+ARErToi5tI9DO6wVIqj5/YMVepQ05Nl4UBmNyzt+JnueYAIcmyYwd7TDewJGsB97M2W22V3XU0HvfhvHH2pAHUIutQjRFvcD9kgpLKm1i9CdxyDNo0h0B8hmAd79zA39r167ePPmjBS3zfwRHbiv09UXXOX1/RLourZS+3nhg= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1731410270; c=relaxed/simple; bh=pX3ZsBEZbDSOWWHgNQFnA+7zErsVd56w2urC4gYof0k=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=hxBOiNx+7AUvUAQ1ljNzhHIh549kcMWGHLENBoIWSEwrRWB7fiRvOBoKVUJVKhSygYf+A4sB1bjrHxgVrJ21V6xqcqgb9dO8GfRYCWfyh1g/kItVD1OHFzNXZCQEaYM7cKhGfpmbfvzCMyAm/z7035zIUXkQaXQHnk20/PQqYiY= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linaro.org; spf=pass smtp.mailfrom=linaro.org; dkim=pass (2048-bit key) header.d=linaro.org header.i=@linaro.org header.b=X+XgE4BB; arc=none smtp.client-ip=209.85.128.43 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=linaro.org Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=linaro.org header.i=@linaro.org header.b="X+XgE4BB" Received: by mail-wm1-f43.google.com with SMTP id 5b1f17b1804b1-43155abaf0bso48924635e9.0 for ; Tue, 12 Nov 2024 03:17:48 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1731410267; x=1732015067; darn=vger.kernel.org; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=dWJ4Wcr1DDF3OrOWnRqKL1IZGwMD1FboXeJ/D4aEFDY=; b=X+XgE4BBdIkeP6q12SpSAfL6MbJnpQPSh4EuqoJ2vQvOywfLYISd0SoCTYr3V9XwIQ 7DvlWWt7txbdZGfMS8nu6eVQmLXrVTaoBeH/qvwizvcLNE/fswmBCXoggaqDwjRdZmON Hg42O2SxeDm66YNO1rpO72PqxVIUahMk1oMoc8QN423sblreUEzasMuWyJOvo/9gQUij +hGaKPuoNBBUOfDjcfLmyonOyPkqwhdYr3LgmgPBjZfHH1/W5eXgU6GN9htb1OyuCI/O W0iRjgHbNvRyi7ua6zT0uL3s6BkT/j9S1/TI7GrGvYcVjgd5eR75oMYyEsKnbbpNTls/ Z8lw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1731410267; x=1732015067; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=dWJ4Wcr1DDF3OrOWnRqKL1IZGwMD1FboXeJ/D4aEFDY=; b=NqG5NJdQb0ZnekzwWCyq4eSdyMPy4AsT74vz2y9rwXHVqxv74oP1ozpO9+mP4nF4Lq 3nU+UxQYT0pRzDPOPlmI0o+8YhEXojRasA6DmnvR35Zd6ptrdJg8GOxRUQyMRQl65Yoc NhEaVRuGDQnXxLGA7psT9HXDItpRxsRIHNvtwod96lpji4AM41JlJwyjcCmPEUhz8yDc 1og+VrAG/1OWsrrt10GEQuymahqlYeJ7+KNu/RGrG8eiTl5rw0/BKfg5e8tfbGJv36es k891Tm7vlQphxAo1ZlIrfmS6r/jXMn+ANAf+UbtJv7J+GHeyRGA9nIKuLfoUVL/wVm3I uf9A== X-Forwarded-Encrypted: i=1; AJvYcCWef6jEZoBppWOuPY1AtyiBAoO7YIK4r9Y8LWI0GsPIWom34LkzfXmMZmoaaOsZWIdIfcmJG6Omwv+CuOk=@vger.kernel.org X-Gm-Message-State: AOJu0Yzt8rYxuOa7PAHX5AWbXk6TKOu03GNgqhK9thqco2GjMNQAFW7Z kQpBSJJFIzVKwztmGkwb75nfOQboEJLslRw7p/9EdGxIjRsxTcCgJTxdYsd+MzQ= X-Google-Smtp-Source: AGHT+IHdyVKh8yiDtEYPmTAcLgcs8M/v+k6vo/2n4BcU3ilUc3c9bFTnx7DhiZaKwNyqsYQKeh+Q7A== X-Received: by 2002:a05:600c:35d5:b0:431:5f8c:ccbd with SMTP id 5b1f17b1804b1-432b74fda62mr141755165e9.4.1731410267558; Tue, 12 Nov 2024 03:17:47 -0800 (PST) Received: from [192.168.0.40] ([176.61.106.227]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-432b053ff08sm209961455e9.10.2024.11.12.03.17.46 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 12 Nov 2024 03:17:47 -0800 (PST) Message-ID: Date: Tue, 12 Nov 2024 11:17:46 +0000 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH 2/4] media: venus: hfi_parser: avoid OOB access beyond payload word count To: Vikash Garodia , Stanimir Varbanov , Mauro Carvalho Chehab Cc: linux-media@vger.kernel.org, linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org, stable@vger.kernel.org References: <20241105-venus_oob-v1-0-8d4feedfe2bb@quicinc.com> <20241105-venus_oob-v1-2-8d4feedfe2bb@quicinc.com> <474d3c62-5747-45b9-b5c3-253607b0c17a@linaro.org> <9098b8ef-76e0-f976-2f4e-1c6370caf59e@quicinc.com> Content-Language: en-US From: Bryan O'Donoghue In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit On 12/11/2024 08:05, Vikash Garodia wrote: > You did not printed the last iteration without the proposed fix. In the last > iteration (Myword 1), it would access the data beyond allocated size of somebuf. > So we can see how the fix protects from OOB situation. Right but the loop _can't_ be correct. What's the point in fixing an OOB in a loop that doesn't work ? This is the loop: #define BUF_SIZE 0x20 // BUF_SIZE doesn't really matter char somebuf[BUF_SIZE]; u32 *word = somebuf[0]; u32 words = ARRAY_SIZE(somebuf); while (words > 1) { data = word + 1; // this word++; // and this words--; } On the first loop word = somebuf[0]; data = somebuf[3]; On the second loop word = somebuf[3]; // the same value as *data in the previous loop and that's just broken because on the second loop *word == *data in the first loop ! That's what my program showed you word 4 == 0x03020100 data=0x07060504 // word == data from previous loop word 3 == 0x07060504 data=0x0b0a0908 // word == data from previous loop word 2 == 0x0b0a0908 data=0x0f0e0d0c The step size, the number of bytes this loop increments is fundamentally wrong because a) Its a fixed size [1] b) *word in loop(n+1) == *data in loop(n) Which cannot ever parse more than one data item - in effect never loop - in one go. > For the functionality part, packet from firmware would come as > followed by i.e > *word = HFI_PROPERTY_PARAM_CODEC_SUPPORTED > *data = payload --> hence here data is pointed to next u32 to point and parse > payload for HFI_PROPERTY_PARAM_CODEC_SUPPORTED. > likewise for other properties in the same packet [1] But we've established that word increments by one word. We wouldn't fix this loop by just making it into while (words > 1) { data = word + 1; word = data + 1; words -= 2; } Because the consumers of the data have different step sizes, different number of bytes they consume for the structs they cast. => case HFI_PROPERTY_PARAM_CODEC_SUPPORTED: parse_codecs(core, data); // consumes sizeof(struct hfi_codec_supported) struct hfi_codec_supported { u32 dec_codecs; u32 enc_codecs; }; case HFI_PROPERTY_PARAM_MAX_SESSIONS_SUPPORTED: parse_max_sessions(core, data); // consumes sizeof(struct hfi_max_sessions_supported) struct hfi_max_sessions_supported { u32 max_sessions; }; case HFI_PROPERTY_PARAM_CODEC_MASK_SUPPORTED: parse_codecs_mask(&codecs, &domain, data); // consumes sizeof(struct hfi_codec_mask_supported) struct hfi_codec_mask_supported { u32 codecs; u32 video_domains; }; case HFI_PROPERTY_PARAM_UNCOMPRESSED_FORMAT_SUPPORTED: parse_raw_formats(core, codecs, domain, data); // consumes sizeof(struct hfi_uncompressed_format_supported) struct hfi_uncompressed_format_supported { u32 buffer_type; u32 format_entries; struct hfi_uncompressed_plane_info plane_info; }; case HFI_PROPERTY_PARAM_CAPABILITY_SUPPORTED: parse_caps(core, codecs, domain, data); struct hfi_capabilities { u32 num_capabilities; struct hfi_capability data[]; }; where hfi_platform.h:#define MAX_CAP_ENTRIES 32 I'll stop there. This routine needs a rewrite. --- bod