linux-kernel - Re: [PATCH v2 11/34] media: iris: introduce packetization layer for creating HFI packets

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Mon, 18 Dec 2023 22:50:57 +0100
From: Konrad Dybcio <konrad.dybcio@...aro.org>
To: Dikshita Agarwal <quic_dikshita@...cinc.com>,
 linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
 stanimir.k.varbanov@...il.com, quic_vgarodia@...cinc.com, agross@...nel.org,
 andersson@...nel.org, mchehab@...nel.org, bryan.odonoghue@...aro.org
Cc: linux-arm-msm@...r.kernel.org, quic_abhinavk@...cinc.com
Subject: Re: [PATCH v2 11/34] media: iris: introduce packetization layer for
 creating HFI packets



On 12/18/23 12:32, Dikshita Agarwal wrote:
> Host firmware interface (HFI) is well defined set of interfaces
> for communication between host driver and firmware.
> The command and responses are exchanged in form of packets.
> One or multiple packets are grouped under packet header.
> Each packet has packet type which describes the specific HFI
> and payload which holds the corresponding value for that HFI.
> 
> Sys_init is the first packets sent to firmware, which initializes
> the firmware. Sys_image_version packet is to get the firmware
> version string.
> 
> Signed-off-by: Dikshita Agarwal <quic_dikshita@...cinc.com>
> ---
[...]

>   struct iris_core {
> @@ -65,6 +70,11 @@ struct iris_core {
>   	struct mem_desc				sfr;
>   	struct mutex				lock; /* lock for core structure */
>   	unsigned int				use_tz;
> +	u8					*packet;
> +	u32					packet_size;
I'm not sure it's necessary to always keep a reference to the last
packet in the core struct, especially since it needs to be allocated
first anyway

> +	u32					sys_init_id;
This looks like a hyper-defensive measure against some firmware
overtaking attacks.. Or a way to spot random/unwanted resets of
the firmware core..

Is it actually necessary, or does this just serve as a debug
feature?

> +	u32					header_id;
Similar to above..

> +	u32					packet_id;
And here.

I performed some quick CTRL-F-agge around the series and this is
never reset.. Can the firmware cope with this? What if I watch a
veeeery long youtube video that ends up creating more than
(1<<32)-1 HFI packets while playing?

> +
> +enum hfi_packet_host_flags {
> +	HFI_HOST_FLAGS_NONE			= 0x00000000,
> +	HFI_HOST_FLAGS_INTR_REQUIRED		= 0x00000001,
> +	HFI_HOST_FLAGS_RESPONSE_REQUIRED	= 0x00000002,
> +	HFI_HOST_FLAGS_NON_DISCARDABLE		= 0x00000004,
> +	HFI_HOST_FLAGS_GET_PROPERTY		= 0x00000008,
BIT(n)?

> +};
> +
> +enum hfi_packet_firmware_flags {
> +	HFI_FW_FLAGS_NONE		= 0x00000000,
> +	HFI_FW_FLAGS_SUCCESS		= 0x00000001,
> +	HFI_FW_FLAGS_INFORMATION	= 0x00000002,
> +	HFI_FW_FLAGS_SESSION_ERROR	= 0x00000004,
> +	HFI_FW_FLAGS_SYSTEM_ERROR	= 0x00000008,
BIT(n)?

Konrad