| 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
| ||
|
Message-ID: <0c656c92-f029-bc02-6026-23649836d080@xs4all.nl>
Date: Tue, 14 Jun 2022 16:09:24 +0200
From: Hans Verkuil <hverkuil@...all.nl>
To: Benjamin Gaignard <benjamin.gaignard@...labora.com>,
mchehab@...nel.org, ezequiel@...guardiasur.com.ar,
p.zabel@...gutronix.de, gregkh@...uxfoundation.org,
mripard@...nel.org, paul.kocialkowski@...tlin.com, wens@...e.org,
jernej.skrabec@...il.com, samuel@...lland.org,
nicolas.dufresne@...labora.com, andrzej.p@...labora.com
Cc: linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-rockchip@...ts.infradead.org, linux-staging@...ts.linux.dev,
linux-arm-kernel@...ts.infradead.org, linux-sunxi@...ts.linux.dev,
kernel@...labora.com
Subject: Re: [PATCH v8 15/17] media: uapi: HEVC: fix padding in v4l2 control
structures
On 6/14/22 10:36, Benjamin Gaignard wrote:
> Fix padding where needed to remove holes and stay align on cache boundaries
align -> aligned
>
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@...labora.com>
> ---
> .../media/v4l/ext-ctrls-codec.rst | 6 +++---
> drivers/media/v4l2-core/v4l2-ctrls-core.c | 9 ---------
> include/media/hevc-ctrls.h | 19 ++++++++++++-------
> 3 files changed, 15 insertions(+), 19 deletions(-)
>
> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> index 05228e280f66..48a8825a001b 100644
> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> @@ -3509,9 +3509,6 @@ enum v4l2_mpeg_video_hevc_size_of_length_field -
> * - __u8
> - ``num_active_dpb_entries``
> - The number of entries in ``dpb``.
> - * - struct :c:type:`v4l2_hevc_dpb_entry`
> - - ``dpb[V4L2_HEVC_DPB_ENTRIES_NUM_MAX]``
> - - The decoded picture buffer, for meta-data about reference frames.
> * - __u8
> - ``num_poc_st_curr_before``
> - The number of reference pictures in the short-term set that come before
> @@ -3535,6 +3532,9 @@ enum v4l2_mpeg_video_hevc_size_of_length_field -
> - ``poc_lt_curr[V4L2_HEVC_DPB_ENTRIES_NUM_MAX]``
> - PocLtCurr as described in section 8.3.2 "Decoding process for reference
> picture set": provides the index of the long term references in DPB array.
> + * - struct :c:type:`v4l2_hevc_dpb_entry`
> + - ``dpb[V4L2_HEVC_DPB_ENTRIES_NUM_MAX]``
> + - The decoded picture buffer, for meta-data about reference frames.
> * - __u64
> - ``flags``
> - See :ref:`Decode Parameters Flags <hevc_decode_params_flags>`
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-core.c b/drivers/media/v4l2-core/v4l2-ctrls-core.c
> index c5c5407584ff..fb68786c498b 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls-core.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls-core.c
> @@ -824,20 +824,11 @@ static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx,
> if (p_hevc_decode_params->num_active_dpb_entries >
> V4L2_HEVC_DPB_ENTRIES_NUM_MAX)
> return -EINVAL;
> -
> - for (i = 0; i < p_hevc_decode_params->num_active_dpb_entries;
> - i++) {
> - struct v4l2_hevc_dpb_entry *dpb_entry =
> - &p_hevc_decode_params->dpb[i];
> -
> - zero_padding(*dpb_entry);
> - }
> break;
>
> case V4L2_CTRL_TYPE_HEVC_SLICE_PARAMS:
> p_hevc_slice_params = p;
>
> - zero_padding(p_hevc_slice_params->pred_weight_table);
> zero_padding(*p_hevc_slice_params);
> break;
>
> diff --git a/include/media/hevc-ctrls.h b/include/media/hevc-ctrls.h
> index efc0412ac41e..9abca1a75bd4 100644
> --- a/include/media/hevc-ctrls.h
> +++ b/include/media/hevc-ctrls.h
> @@ -133,7 +133,9 @@ struct v4l2_ctrl_hevc_sps {
> __u8 chroma_format_idc;
> __u8 sps_max_sub_layers_minus1;
>
> + __u8 reserved[6];
> __u64 flags;
> + __u8 padding[24];
Why are there 24 padding bytes at the end? For future use? If so, what is
the rationale for '24'? Is it likely that new fields will be added in future
HEVC revisions? Or is it in case we forget something?
It's missing kerneldoc comments as well: it should state that the application
must zero this.
Why mix 'reserved' with 'padding'? It's odd to see both names in a single
struct.
In any case, this patch goes beyond 'fixing padding', it is doing more.
If you really want to add space for future use at the end of structs,
then do that in a separate patch together with a rationale for it.
Regards,
Hans
> };
>
> #define V4L2_HEVC_PPS_FLAG_DEPENDENT_SLICE_SEGMENT_ENABLED (1ULL << 0)
> @@ -210,9 +212,10 @@ struct v4l2_ctrl_hevc_pps {
> __s8 pps_beta_offset_div2;
> __s8 pps_tc_offset_div2;
> __u8 log2_parallel_merge_level_minus2;
> + __u8 reserved[9];
>
> - __u8 padding[4];
> __u64 flags;
> + __u8 padding[56];
> };
>
> #define V4L2_HEVC_DPB_ENTRY_LONG_TERM_REFERENCE 0x01
> @@ -245,8 +248,8 @@ struct v4l2_hevc_dpb_entry {
> __u64 timestamp;
> __u8 flags;
> __u8 field_pic;
> + __u16 reserved;
> __s32 pic_order_cnt_val;
> - __u8 padding[2];
> };
>
> /**
> @@ -285,8 +288,6 @@ struct v4l2_hevc_pred_weight_table {
> __s8 delta_chroma_weight_l1[V4L2_HEVC_DPB_ENTRIES_NUM_MAX][2];
> __s8 chroma_offset_l1[V4L2_HEVC_DPB_ENTRIES_NUM_MAX][2];
>
> - __u8 padding[6];
> -
> __u8 luma_log2_weight_denom;
> __s8 delta_chroma_log2_weight_denom;
> };
> @@ -381,18 +382,20 @@ struct v4l2_ctrl_hevc_slice_params {
> /* ISO/IEC 23008-2, ITU-T Rec. H.265: Picture timing SEI message */
> __u8 pic_struct;
>
> + __u8 reserved0[3];
> /* ISO/IEC 23008-2, ITU-T Rec. H.265: General slice segment header */
> __u32 slice_segment_addr;
> __u8 ref_idx_l0[V4L2_HEVC_DPB_ENTRIES_NUM_MAX];
> __u8 ref_idx_l1[V4L2_HEVC_DPB_ENTRIES_NUM_MAX];
> __u16 short_term_ref_pic_set_size;
> __u16 long_term_ref_pic_set_size;
> - __u8 padding;
>
> /* ISO/IEC 23008-2, ITU-T Rec. H.265: Weighted prediction parameter */
> struct v4l2_hevc_pred_weight_table pred_weight_table;
>
> + __u8 reserved1[2];
> __u64 flags;
> + __u8 padding[40];
> };
>
> #define V4L2_HEVC_DECODE_PARAM_FLAG_IRAP_PIC 0x1
> @@ -408,7 +411,6 @@ struct v4l2_ctrl_hevc_slice_params {
> * @long_term_ref_pic_set_size: specifies the size of long-term reference
> * pictures set include in the SPS of the first slice
> * @num_active_dpb_entries: the number of entries in dpb
> - * @dpb: the decoded picture buffer, for meta-data about reference frames
> * @num_poc_st_curr_before: the number of reference pictures in the short-term
> * set that come before the current frame
> * @num_poc_st_curr_after: the number of reference pictures in the short-term
> @@ -419,6 +421,7 @@ struct v4l2_ctrl_hevc_slice_params {
> * @poc_st_curr_after: provides the index of the short term after references
> * in DPB array
> * @poc_lt_curr: provides the index of the long term references in DPB array
> + * @dpb: the decoded picture buffer, for meta-data about reference frames
> * @flags: see V4L2_HEVC_DECODE_PARAM_FLAG_{}
> */
> struct v4l2_ctrl_hevc_decode_params {
> @@ -426,14 +429,16 @@ struct v4l2_ctrl_hevc_decode_params {
> __u16 short_term_ref_pic_set_size;
> __u16 long_term_ref_pic_set_size;
> __u8 num_active_dpb_entries;
> - struct v4l2_hevc_dpb_entry dpb[V4L2_HEVC_DPB_ENTRIES_NUM_MAX];
> __u8 num_poc_st_curr_before;
> __u8 num_poc_st_curr_after;
> __u8 num_poc_lt_curr;
> __u8 poc_st_curr_before[V4L2_HEVC_DPB_ENTRIES_NUM_MAX];
> __u8 poc_st_curr_after[V4L2_HEVC_DPB_ENTRIES_NUM_MAX];
> __u8 poc_lt_curr[V4L2_HEVC_DPB_ENTRIES_NUM_MAX];
> + __u8 reserved[4];
> + struct v4l2_hevc_dpb_entry dpb[V4L2_HEVC_DPB_ENTRIES_NUM_MAX];
> __u64 flags;
> + __u8 padding[56];
> };
>
> /**
Powered by blists - more mailing lists