Re: [PATCH 09/11] media: amlogic: vdec: add HEVC decoding on the HEVC core

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 14 potential issue(s) to consider:
- [Critical] media: amlogic: vdec: CAPTURE overflow in h265_config_sao() and h265_copy_out()
- [Critical] media: amlogic: vdec: MMU map truncation in h265_fill_mmu_map() above 2304p
- [Critical] media: amlogic: vdec: unvalidated non-first slice state in h265_slice_header()
- [Critical] media: amlogic: vdec: unsynchronized IRQ teardown in aml_hevc_exit()
- [High] media: amlogic: vdec: V4L2_PIX_FMT_HEVC_SLICE lacks stateless HEVC controls
- [High] media: amlogic: vdec: OOB read and invalid tile sizes in h265_update_tiles()
- [High] media: amlogic: vdec: shift overflow and broken POC math in h265_slice_header()
- [High] media: amlogic: vdec: DPB slot leak on IDR pictures in h265_apply_rps()
- [High] media: amlogic: vdec: swap buffers not programmed in h265_config_workspace()
- [High] media: amlogic: vdec: freed DMA addresses left active on slice header error
- [High] media: amlogic: vdec: timeout recovery and IRQ race in aml_hevc_dec_run()
- [High] media: amlogic: vdec: premature job completion in h265_finish() from IRQ thread
- [High] media: amlogic: vdec: NULL deref in aml_hevc_dec_run() after CAPTURE restart
- [Medium] media: amlogic: vdec: skipped tile init on same POC in h265_slice_header()
--

commit 6edd9c45f2e4b6fa27b412395f4321725272a652
Author: Christian Stefan Kövér-Draxl <ckoever@outlook.de>

media: amlogic: vdec: add HEVC decoding on the HEVC core

This commit adds HEVC Main and Main 10 decoding support up to 4K on the
Amlogic S4 HEVC hardware core. It registers the V4L2_PIX_FMT_HEVC_SLICE
format, manages workspace and DPB picture buffers, and configures the
hardware decoder per slice from the microcode RPM parameter block.

> diff --git a/drivers/media/platform/amlogic/vdec/aml_vdec.c b/drivers/media/platform/amlogic/vdec/aml_vdec.c
> index f1696aa2fac53..ee020a62836fc 100644
> --- a/drivers/media/platform/amlogic/vdec/aml_vdec.c
> +++ b/drivers/media/platform/amlogic/vdec/aml_vdec.c

[ ... ]

> @@ -112,7 +113,19 @@ static const struct aml_vdec_v4l2_ctrl controls[] = {
>  		.cfg = {
>  			.id = V4L2_CID_STATELESS_MPEG2_QUANTISATION,
>  		},
> -	},};
> +	}, {
> +		.codec_type = CODEC_TYPE_HEVC,
> +		.cfg = {
> +			.id = V4L2_CID_AML_HEVC_REFS,
> +			.name = "HEVC Reference Lists",
> +			.type = V4L2_CTRL_TYPE_INTEGER,
> +			.min = S32_MIN,
> +			.max = S32_MAX,
> +			.step = 1,
> +			.dims = { AML_HEVC_REFS_LEN },
> +		},
> +	}
> +};

[Severity: High]
In drivers/media/platform/amlogic/vdec/aml_vdec_platform.c,
aml_s4_video_formats[] advertises V4L2_PIX_FMT_HEVC_SLICE:

	{
		.name = "HEVC",
		.fourcc = V4L2_PIX_FMT_HEVC_SLICE,
		.type = AML_FMT_DEC,
		...
		.codec_type = CODEC_TYPE_HEVC,

However, controls[] in drivers/media/platform/amlogic/vdec/aml_vdec.c only
registers the private V4L2_CID_AML_HEVC_REFS control (defined in
drivers/media/platform/amlogic/vdec/hevc.h as V4L2_CID_USER_BASE + 0x1f01)
and omits the standard V4L2 stateless HEVC controls
(V4L2_CID_STATELESS_HEVC_SPS, V4L2_CID_STATELESS_HEVC_PPS,
V4L2_CID_STATELESS_HEVC_DECODE_PARAMS, V4L2_CID_STATELESS_HEVC_SLICE_PARAMS,
V4L2_CID_STATELESS_HEVC_SCALING_MATRIX, V4L2_CID_STATELESS_HEVC_DECODE_MODE,
and V4L2_CID_STATELESS_HEVC_START_CODE).

In addition, although the commit description states that the access unit is
rebuilt in Annex-B form with VPS, SPS, and PPS from the stateless controls,
aml_hevc_dec_run() passes the raw OUTPUT buffer directly to h265_feed(), and
h265_irq_thread() under H265_NAL_SEARCH_DONE discards coded slices with
H265_DISCARD_NAL until in-band H265_NAL_VPS, H265_NAL_SPS, and H265_NAL_PPS
NAL units have been seen in the bitstream:

	case H265_NAL_SEARCH_DONE: {
		int nal = read_dos_reg(hw, H265_CUR_NAL_UNIT_TYPE);
		u32 parse = H265_DISCARD_NAL;
		...
		} else if (h->have_vps && h->have_sps && h->have_pps && nal <= H265_NAL_CRA) {

How can standard V4L2 stateless HEVC clients drive V4L2_PIX_FMT_HEVC_SLICE
without the stateless HEVC controls and Annex-B header reconstruction, or
was this intended to use the stateless controls rather than in-band parameter
sets and a driver-private control?

> diff --git a/drivers/media/platform/amlogic/vdec/hevc.c b/drivers/media/platform/amlogic/vdec/hevc.c
> new file mode 100644
> index 0000000000000..6d9f457436cfd
> --- /dev/null
> +++ b/drivers/media/platform/amlogic/vdec/hevc.c
> @@ -0,0 +1,1728 @@

[ ... ]

> +static int h265_alloc_pics(struct aml_hevc_ctx *h)
> +{
> +	struct device *dev = h->hw->dev;
> +	u32 w = h->pic_w, ht = h->pic_h;
> +	size_t body = ALIGN(h265_comp_body_size(w, ht, h->ten_bit), PAGE_SIZE);
> +	/* Size the MV buffers for the smallest LCU, so they fit any. */
> +	size_t mv = h265_mv_buf_size(w, ht, 16);
> +	size_t hdr = h265_hdr_alloc_size(w, ht);
> +	size_t dw = h->dw_target_y ? 0 : ALIGN(w * ht * 3 / 2, SZ_64K);
> +	int i;
> +
> +	if (h->alloc_w == w && h->alloc_h == ht && h->dw_size == dw &&
> +	    h->alloc_ten_bit == h->ten_bit && h->alloc_nslots == h->nslots)
> +		return 0;
> +
> +	h265_free_pics(h);
> +	for (i = 0; i < h->nslots; i++) {
> +		struct h265_pic *pic = &h->pic[i];
> +
> +		pic->body_v = dma_alloc_coherent(dev, body, &pic->body_p, GFP_KERNEL);
> +		pic->hdr_v = dma_alloc_coherent(dev, hdr, &pic->hdr_p, GFP_KERNEL);
> +		pic->mv_v = dma_alloc_coherent(dev, mv, &pic->mv_p, GFP_KERNEL);
> +		if (dw)
> +			pic->dw_v = dma_alloc_noncoherent(dev, dw, &pic->dw_p,
> +							  DMA_FROM_DEVICE, GFP_KERNEL);
> +		if (!pic->body_v || !pic->hdr_v || !pic->mv_v || (dw && !pic->dw_v)) {
> +			h->body_size = body;
> +			h->hdr_size = hdr;
> +			h->mv_size = mv;
> +			h->dw_size = dw;
> +			h265_free_pics(h);
> +			return -ENOMEM;
> +		}

[Severity: High]
When h265_alloc_pics() reallocates picture buffers on a resolution, bit-depth,
or DPB size change, it first frees the existing buffers with h265_free_pics(h).
If one of the subsequent allocations fails and returns -ENOMEM,
HEVCD_MPP_ANC2AXI_TBL_DATA is not cleared, and if a smaller body_size is
allocated later, h265_fill_mmu_map() only overwrites the first pages entries
of h->mmu_map, leaving stale PFNs from the freed buffers in the remaining
entries.

Furthermore, when h265_slice_header() returns a negative error in
h265_irq_thread():

		ret = h265_slice_header(h);
		if (ret < 0) {
			dev_err(hw->dev, "hevc: slice setup failed (%d)\n", ret);
			h265_finish(h, VB2_BUF_STATE_ERROR);
			return IRQ_HANDLED;
		}

neither HEVC_MPSR nor HEVC_CPSR is cleared to halt the AMRISC, and h->cur is
not reset to -1 even if a slot was already claimed and marked valid and
referenced in h265_slice_header().

Could this leave the hardware running with stale physical addresses in
HEVCD_MPP_ANC2AXI_TBL_DATA and h->mmu_map, or leak the claimed slot on slice
setup errors?

[ ... ]

> +/* hevc_config_work_space_hw() */
> +static void h265_config_workspace(struct aml_hevc_ctx *h)
> +{
> +	struct aml_vdec_hw *hw = h->hw;
> +
> +	wr(hw, HEVCD_IPP_LINEBUFF_BASE, h->ipp);
> +	wr(hw, HEVC_RPM_BUFFER, h->rpm);
> +	wr(hw, HEVC_SHORT_TERM_RPS, h->short_term_rps);
> +	wr(hw, H265_VPS_BUFFER, h->vps);
> +	wr(hw, H265_SPS_BUFFER, h->sps);
> +	wr(hw, HEVC_PPS_BUFFER, h->pps);
> +	wr(hw, H265_SAO_UP, h->sao_up);
> +	wr(hw, HEVC_ASSIST_MMU_MAP_ADDR, h->mmu_map);
> +	wr(hw, H265_SCALELUT, h->scalelut);
> +	wr(hw, HEVC_DBLK_CFG3, 0x4010);	/* <= 4096x2304 */
> +	wr(hw, HEVC_DBLK_CFG4, h->dblk_para);
> +	wr(hw, HEVC_DBLK_CFG5, h->dblk_data);
> +	wr(hw, HEVC_DBLK_CFGE, h->dblk_data2);
> +	wr(hw, H265_LMEM_DUMP_ADR, h->lmem);
> +}

[Severity: High]
In h265_alloc_workspace(), h->swap_buf and h->swap_buf2 are carved out of the
workspace allocation:

	h->swap_buf = h265_carve(&p, H265_SWAP_BUF_SIZE);
	h->swap_buf2 = h265_carve(&p, H265_SWAP_BUF2_SIZE);

However, h265_config_workspace() never writes h->swap_buf or h->swap_buf2 to
HEVC_STREAM_SWAP_BUFFER (HEVC_ASSIST_SCRATCH_7) and HEVC_STREAM_SWAP_BUFFER2
(HEVC_ASSIST_SCRATCH_8), and those scratch registers are not cleared before
starting the microcode.

If the microcode accesses the stream swap buffers, will it DMA to address 0
or to stale physical addresses left in the scratch registers by a previous
VP9 or AV1 session?

[ ... ]

> +/* apply_ref_pic_set(): everything the current RPS does not name stops being
> + * a reference.
> + */
> +static void h265_apply_rps(struct aml_hevc_ctx *h)
> +{
> +	int i, j;
> +
> +	for (i = 0; i < H265_SLOTS; i++) {
> +		struct h265_pic *pic = &h->pic[i];
> +		bool found = false;
> +
> +		pic->long_term = false;
> +		if (!pic->valid || !pic->referenced || pic->poc == h->curr_poc)
> +			continue;
> +		for (j = 0; j < 16; j++) {
> +			int poc;
> +			bool used, lt;
> +
> +			if (!h265_rps_poc(h, j, &poc, &used, &lt))
> +				break;
> +			if (poc == pic->poc) {
> +				pic->long_term = lt;
> +				found = true;
> +				break;
> +			}
> +		}
> +		if (!found)
> +			pic->referenced = false;
> +	}
> +}

[Severity: High]
In h265_slice_header(), h265_apply_rps(h) is called on a new picture before
a slot is chosen and before h->cur is updated. When a new IDR picture arrives,
h->curr_poc is set to 0 first:

	if (h->nal_type == H265_NAL_IDR_W_RADL || h->nal_type == H265_NAL_IDR_N_LP) {
		h->curr_poc = 0;

Because h265_apply_rps() skips any existing slot where pic->poc == h->curr_poc,
any picture from an earlier GOP that still has pic->poc == 0 and
pic->referenced == true is skipped instead of having pic->referenced cleared.
When subsequent frames in the new GOP reference POC 0 in their RPS, both the
old POC 0 slot and the new IDR slot match poc == pic->poc and remain marked
referenced.

Across multiple IDR frames, doesn't this leak one DPB slot per IDR until all
h->nslots slots remain referenced and h265_slice_header() fails with -ENOBUFS?

[ ... ]

> +static void h265_update_tiles(struct aml_hevc_ctx *h, int w_cu, int h_cu,
> +			      int sao_mem_unit)
> +{
> +	int sao_vb_size = (sao_mem_unit + (2 << 4)) * h_cu;
> +	int sao_abv_size = sao_mem_unit * w_cu;
> +	int i, j, x, y;
> +	bool uniform;
> +
> +	h->tile_enabled = h->p[P_TILES_ENABLED_FLAG] & 1;
> +	if (!h->tile_enabled) {
> +		h->num_tile_col = 1;
> +		h->num_tile_row = 1;
> +		h->tile[0][0] = (struct h265_tile){ 0, 0, w_cu, h_cu,
> +						    h->sao_vb, h->sao_abv };
> +		return;
> +	}
> +	h->num_tile_col = h->p[P_NUM_TILE_COLUMNS_MINUS1] + 1;
> +	h->num_tile_row = h->p[P_NUM_TILE_ROWS_MINUS1] + 1;
> +	if (h->num_tile_col > H265_MAX_TILE_COLS || h->num_tile_col <= 0)
> +		h->num_tile_col = 1;
> +	if (h->num_tile_row > H265_MAX_TILE_ROWS || h->num_tile_row <= 0)
> +		h->num_tile_row = 1;
> +	uniform = h->p[P_TILES_ENABLED_FLAG] & 2;
> +
> +	y = 0;
> +	for (i = 0; i < h->num_tile_row; i++) {
> +		x = 0;
> +		for (j = 0; j < h->num_tile_col; j++) {
> +			struct h265_tile *t = &h->tile[i][j];
> +
> +			if (j == h->num_tile_col - 1)
> +				t->width = w_cu - x;
> +			else
> +				t->width = uniform ? w_cu / h->num_tile_col :
> +						     h->p[P_TILE_WIDTH + j];
> +			if (i == h->num_tile_row - 1)
> +				t->height = h_cu - y;
> +			else
> +				t->height = uniform ? h_cu / h->num_tile_row :
> +						      h->p[P_TILE_HEIGHT + i];
> +			t->start_cu_x = x;
> +			t->start_cu_y = y;
> +			t->sao_vb = h->sao_vb + j * sao_vb_size;
> +			t->sao_abv = h->sao_abv + i * sao_abv_size;
> +			x += t->width;
> +		}
> +		y += h->tile[i][0].height;
> +	}
> +}

[Severity: High]
In the RPM parameter layout enum, P_TILE_HEIGHT has 8 entries before
P_MISC_FLAG0:

	P_TILE_WIDTH,			/* 12 entries */
	P_TILE_HEIGHT = P_TILE_WIDTH + 12,	/* 8 entries */
	P_MISC_FLAG0 = P_TILE_HEIGHT + 8,

However, H265_MAX_TILE_ROWS is 20, so when uniform is false and
h->num_tile_row > 9, indexing h->p[P_TILE_HEIGHT + i] for i >= 8 reads past
P_TILE_HEIGHT into P_MISC_FLAG0 and subsequent RPM fields.

Also, if non-uniform tile widths or heights in h->p[] sum to more than w_cu or
h_cu, w_cu - x or h_cu - y becomes negative, which passes the zero-only check
in h265_slice_header():

	if (!h->tile_width_lcu || !h->tile_height_lcu)
		return -EINVAL;

and writes negative dimensions into the MPRED registers. Should
h265_update_tiles() bound P_TILE_HEIGHT accesses to the RPM array size and
reject non-positive tile widths and heights?

[ ... ]

> +static void h265_fill_mmu_map(struct aml_hevc_ctx *h)
> +{
> +	struct h265_pic *pic = &h->pic[h->cur];
> +	u32 *map = h->side_v + (h->mmu_map - h->side_p);
> +	u32 pages = h->body_size / PAGE_SIZE, first = pic->body_p >> PAGE_SHIFT, i;
> +
> +	for (i = 0; i < pages && i < H265_MMU_MAP_SIZE / 4; i++)
> +		map[i] = first + i;
> +}

[Severity: Critical]
H265_MMU_MAP_SIZE is 0x1200 * 4 bytes (4,608 page entries), which matches the
page count of a 4096x2304 10-bit compressed body. However, h265_slice_header()
accepts picture heights up to H265_MAX_DIM (4096):

	if (p[P_PIC_WIDTH] < H265_MIN_DIM || p[P_PIC_HEIGHT] < H265_MIN_DIM ||
	    p[P_PIC_WIDTH] > H265_MAX_DIM || p[P_PIC_HEIGHT] > H265_MAX_DIM) {

For a 4096x4096 picture, h265_comp_body_size() is up to 8,192 pages, and
h265_config_comp() programs that full length into HEVC_CM_BODY_LENGTH and
HEVC_CM_HEADER_OFFSET while h265_fill_mmu_map() silently stops filling map[]
at 4,608 entries.

When the hardware writes beyond page 4,607, won't the MMU DMA engine read past
h->mmu_map into h->aux and DMA compressed frame data over unrelated physical
memory (including page 0)?

[ ... ]

> +static int h265_slice_header(struct aml_hevc_ctx *h)
> +{
> +	struct aml_vdec_hw *hw = h->hw;
> +	u16 *p = h->p;
> +	int i, ret, col_ref;
> +
> +	h->temporal_id = p[P_TEMPORAL_ID];
> +	h->nal_type = p[P_NAL_UNIT_TYPE];
> +	h->misc_flag0 = p[P_MISC_FLAG0];
> +
> +	if (!p[P_FIRST_SLICE_SEGMENT_IN_PIC_FLAG]) {
> +		h->slice_segment_addr = p[P_SLICE_SEGMENT_ADDRESS];
> +		if (!p[P_DEPENDENT_SLICE_SEGMENT_FLAG])
> +			h->slice_addr = h->slice_segment_addr;
> +	} else {
> +		h->slice_segment_addr = 0;
> +		h->slice_addr = 0;
> +	}
> +	h->iprev_poc = h->curr_poc;
> +	h->slice_type = p[P_SLICE_TYPE];
> +	h->tmvp = p[P_SLICE_TEMPORAL_MVP_ENABLE_FLAG] & 1;
> +	h->is_next_slice_segment = !!p[P_DEPENDENT_SLICE_SEGMENT_FLAG];

[Severity: Critical]
When p[P_FIRST_SLICE_SEGMENT_IN_PIC_FLAG] is set, h->slice_segment_addr is
set to 0 above, but the rest of h265_slice_header() never reads
h->slice_segment_addr and instead checks raw p[P_SLICE_SEGMENT_ADDRESS] and
p[P_DEPENDENT_SLICE_SEGMENT_FLAG].

On a non-first slice (p[P_SLICE_SEGMENT_ADDRESS] != 0 with h->cur >= 0),
h265_slice_header() also updates h->pic_w, h->pic_h, h->ten_bit, and
h->lcu_size without reallocating picture buffers or verifying that they match
h->alloc_w, h->alloc_h, and h->alloc_ten_bit, and it does not check that
h->slice_addr < h->lcu_total before h265_config_mpred() computes:

	dma_addr_t rd_ptr = rd_start + h->slice_addr * unit;
	dma_addr_t wr_ptr = cur->mv_p + h->slice_addr * unit;

Can a continuation slice with a larger SPS/PPS or an out-of-range
slice_segment_address cause the hardware to DMA past the allocated MV, SAO,
and picture buffers?

[ ... ]

> +	h->lcu_size = 1 << (p[P_LOG2_MIN_CB_SIZE_MINUS3] + 3 +
> +			    p[P_LOG2_DIFF_MAX_MIN_CB_SIZE]);
> +	if (h->lcu_size < 16 || h->lcu_size > 64)
> +		return -EINVAL;
> +	h->lcu_size_log2 = ilog2(h->lcu_size);
> +	h->lcu_x_num = DIV_ROUND_UP(h->pic_w, h->lcu_size);
> +	h->lcu_y_num = DIV_ROUND_UP(h->pic_h, h->lcu_size);
> +	h->lcu_total = h->lcu_x_num * h->lcu_y_num;
> +
> +	/* POC, 8.3.1 */
> +	if (h->nal_type == H265_NAL_IDR_W_RADL || h->nal_type == H265_NAL_IDR_N_LP) {
> +		h->curr_poc = 0;
> +		if (h->temporal_id - 1 == 0)
> +			h->prev_tid0_poc = h->curr_poc;
> +	} else {
> +		int max_lsb = 1 << (p[P_LOG2_MAX_POC_LSB_MINUS4] + 4);
> +		int prev_lsb = h->prev_tid0_poc % max_lsb;
> +		int prev_msb = h->prev_tid0_poc - prev_lsb;
> +		int lsb = p[P_POC_LSB];
> +		int msb;
> +
> +		if (lsb < prev_lsb && prev_lsb - lsb >= max_lsb / 2)
> +			msb = prev_msb + max_lsb;
> +		else if (lsb > prev_lsb && lsb - prev_lsb > max_lsb / 2)
> +			msb = prev_msb - max_lsb;
> +		else
> +			msb = prev_msb;
> +		if (h->nal_type == H265_NAL_BLA_W_LP ||
> +		    h->nal_type == H265_NAL_BLA_W_RADL ||
> +		    h->nal_type == H265_NAL_BLA_N_LP)
> +			msb = 0;
> +		h->curr_poc = msb + lsb;
> +		if (h->temporal_id - 1 == 0)
> +			h->prev_tid0_poc = h->curr_poc;
> +	}

[Severity: High]
There appear to be three issues in the LCU and POC calculations here in
h265_slice_header():

1. Both 1 << (p[P_LOG2_MIN_CB_SIZE_MINUS3] + 3 + p[P_LOG2_DIFF_MAX_MIN_CB_SIZE])
   and max_lsb = 1 << (p[P_LOG2_MAX_POC_LSB_MINUS4] + 4) shift a signed 32-bit
   int by unvalidated u16 values from the RPM buffer. If
   p[P_LOG2_MAX_POC_LSB_MINUS4] is corrupted (for example >= 27), max_lsb can
   overflow to INT_MIN or 0 and trigger a divide error in
   h->prev_tid0_poc % max_lsb.

2. When h->prev_tid0_poc is negative, h->prev_tid0_poc % max_lsb produces a
   negative prev_lsb in C rather than h->prev_tid0_poc & (max_lsb - 1),
   miscomputing prev_msb and h->curr_poc.

3. Per ITU-T H.265 section 8.3.1, prevTid0Pic should only be updated when the
   picture is not a RASL, RADL, or sub-layer non-reference picture, whereas
   h->prev_tid0_poc is updated here for all pictures with temporal_id == 1
   (even RASL pictures skipped immediately below).

Should the shift exponents be bounds-checked first, prev_lsb masked with
max_lsb - 1, and h->prev_tid0_poc restricted to non-RASL/RADL reference
pictures?

[ ... ]

> +	if (h->iprev_poc != h->curr_poc) {
> +		h->new_tile = true;
> +		h->tile_x = 0;
> +		h->tile_y = 0;
> +	} else if (h->tile_enabled) {
> +		int t = h265_tile_index(h, p[P_SLICE_SEGMENT_ADDRESS], h->lcu_x_num);
> +
> +		if (t != -1 && t != (h->tile_x | (h->tile_y << 8))) {
> +			h->new_tile = true;
> +			h->tile_x = t & 0xff;
> +			h->tile_y = (t >> 8) & 0xff;
> +		} else {
> +			h->new_tile = false;
> +		}
> +	} else {
> +		h->new_tile = false;
> +	}
> +	if (h->tile_x >= H265_MAX_TILE_COLS || h->tile_y >= H265_MAX_TILE_ROWS)
> +		h->new_tile = false;
> +	if (h->new_tile) {
> +		struct h265_tile *t = &h->tile[h->tile_y][h->tile_x];
> +
> +		h->tile_start_lcu_x = t->start_cu_x;
> +		h->tile_start_lcu_y = t->start_cu_y;
> +		h->tile_width_lcu = t->width;
> +		h->tile_height_lcu = t->height;
> +	}

[Severity: Medium]
If two consecutive pictures have the same POC (such as back-to-back IDR
pictures with curr_poc == 0, or an IDR picture following a skipped RASL
picture whose curr_poc was 0), h->iprev_poc != h->curr_poc is false on the
first slice of the new picture.

When that happens, h->new_tile is set to false and h->tile_width_lcu /
h->tile_height_lcu are either left at 0 (failing the check below with -EINVAL)
or retain stale dimensions from the previous picture. Should this test
h->new_pic instead of h->iprev_poc != h->curr_poc?

[ ... ]

> +static void h265_copy_out(struct aml_hevc_ctx *h)
> +{
> +	struct aml_vdec_ctx *ctx = h->v4l2_ctx;
> +	struct v4l2_pix_format_mplane *pf = &ctx->pix_fmt[AML_FMT_DST];
> +	struct vb2_v4l2_buffer *dst = v4l2_m2m_next_dst_buf(ctx->m2m_ctx);
> +	struct h265_pic *pic;
> +	struct vb2_buffer *vb;
> +	u32 stride, dst_h, row, w, ht;
> +	u8 *y, *c;
> +	const u8 *src;
> +
> +	if (!dst || h->cur < 0)
> +		return;
> +	vb = &dst->vb2_buf;
> +	pic = &h->pic[h->cur];
> +	stride = pf->plane_fmt[0].bytesperline ? : pic->width;
> +	dst_h = pf->height ? : pic->height;
> +
> +	if (!h->dw_target_y) {
> +		src = pic->dw_v;
> +		if (!src)
> +			return;
> +		dma_sync_single_for_cpu(h->hw->dev, pic->dw_p, h->dw_size, DMA_FROM_DEVICE);
> +		w = pic->width;
> +		ht = pic->height;
> +		y = vb2_plane_vaddr(vb, 0);
> +		if (!y)
> +			return;
> +		c = vb->num_planes >= 2 ? vb2_plane_vaddr(vb, 1) : y + (size_t)stride * dst_h;
> +		if (!c)
> +			return;
> +		for (row = 0; row < ht; row++)
> +			memcpy(y + (size_t)row * stride, src + (size_t)row * w, w);
> +		for (row = 0; row < ht / 2; row++)
> +			memcpy(c + (size_t)row * stride, src + (size_t)(ht + row) * w, w);
> +		aml_vdec_dst_cpu_written(h->hw->dev, vb);
> +	}

[Severity: Critical]
Can a bitstream with dimensions larger than the configured CAPTURE format
overflow the CAPTURE buffer in both h265_config_sao() and h265_copy_out()?

In aml_hevc_dec_run(), the CAPTURE buffer size check runs before the hardware
parses the current access unit's SPS and slice header:

	h265_set_dw_target(h);
	if (h->dw_target_y && (size_t)h->alloc_w * h->alloc_h * 3 / 2 > h->dw_target_size &&
	    h->alloc_w) {
		h->dw_target_y = 0;	/* stream outgrew the capture buffer */
	}

On the first frame (where h->alloc_w is 0) or on a mid-stream resolution
increase, h->dw_target_y stays non-zero even if the new h->pic_w and h->pic_h
exceed pf->width, pf->height, and h->dw_target_size. When h265_config_sao()
then programs HEVC_SAO_Y_START_ADDR and HEVC_SAO_C_START_ADDR with
h->dw_target_y and h->dw_target_c without checking h->pic_w and h->pic_h
against pf->width and h->dw_target_size, the hardware double-write unit DMAs
past the end of the CAPTURE buffer.

Then, on the second frame, h->alloc_w is non-zero, so aml_hevc_dec_run() clears
h->dw_target_y to 0. That changes dw in h265_alloc_pics() from 0 to non-zero,
which calls h265_free_pics(h) and wipes all decoded reference pictures in the
DPB. Finally, in the fallback path in h265_copy_out() above, memcpy() copies
ht rows of w bytes into y and c without verifying that w <= stride,
ht <= dst_h, or that the destination planes are large enough.

> +	if (vb->num_planes >= 2) {
> +		vb2_set_plane_payload(vb, 0, stride * dst_h);
> +		vb2_set_plane_payload(vb, 1, stride * dst_h / 2);
> +	} else {
> +		vb2_set_plane_payload(vb, 0, stride * dst_h * 3 / 2);
> +	}
> +	h->have_picture = true;
> +}
> +
> +static void h265_finish(struct aml_hevc_ctx *h, enum vb2_buffer_state state)
> +{
> +	struct aml_vdec_ctx *ctx = h->v4l2_ctx;
> +	struct vb2_v4l2_buffer *buf;
> +	unsigned int i;
> +
> +	if (h->finished)
> +		return;
> +	h->finished = true;
> +
> +	if (!h->have_picture) {
> +		buf = v4l2_m2m_next_dst_buf(ctx->m2m_ctx);
> +		if (buf)
> +			for (i = 0; i < buf->vb2_buf.num_planes; i++)
> +				vb2_set_plane_payload(&buf->vb2_buf, i, 0);
> +	}
> +	buf = v4l2_m2m_next_src_buf(ctx->m2m_ctx);
> +	if (buf && buf->vb2_buf.req_obj.req)
> +		v4l2_ctrl_request_complete(buf->vb2_buf.req_obj.req, &ctx->ctrl_handler);
> +
> +	ctx->int_cond = 1;
> +	ctx->job_finished = true;
> +	v4l2_m2m_buf_done_and_job_finish(ctx->dev->m2m_dev_dec, ctx->m2m_ctx, state);
> +	wake_up(&ctx->queue);
> +}

[Severity: High]
h265_finish() runs in h265_irq_thread() and calls
v4l2_m2m_buf_done_and_job_finish() before waking ctx->queue and before
aml_hevc_dec_run() returns to m2mops_vdec_device_run() in
drivers/media/platform/amlogic/vdec/aml_vdec.c:

	ctx->job_finished = false;
	ret = ctx->codec_ops->run(ctx);

	v4l2_m2m_buf_copy_metadata(src_buf, dst_buf);

Because v4l2_m2m_buf_done_and_job_finish() removes src_buf and dst_buf from
their queues, returns them to userspace, and schedules the next M2M job
immediately, doesn't this return the buffers to userspace before
v4l2_m2m_buf_copy_metadata() has copied the timestamp and flags, and allow the
next M2M job (or a concurrent streamoff/close after v4l2_m2m_cancel_job()) to
race with the tail of h265_finish() and aml_hevc_dec_run()?

[ ... ]

> +void aml_hevc_exit(void *priv)
> +{
> +	struct aml_vdec_ctx *ctx = priv;
> +	struct aml_hevc_ctx *h = ctx->codec_priv;
> +
> +	if (!h)
> +		return;
> +	h->hw->hevc_irq_thread_fn = NULL;
> +	h->hw->hevc_priv = NULL;
> +	if (h->started)
> +		regmap_write(h->hw->map[DOS_BUS], HEVC_MPSR, 0);
> +	h265_free(h);
> +	kfree(h);
> +	ctx->codec_priv = NULL;
> +}

[Severity: Critical]
In aml_hevc_exit(), h->hw->hevc_irq_thread_fn and h->hw->hevc_priv are cleared
before stopping HEVC_MPSR, HEVC_ASSIST_MBOX0_MASK is not cleared, and
synchronize_irq(h->hw->hevc_irq) is never called before h265_free(h) and
kfree(h).

In drivers/media/platform/amlogic/vdec/aml_vdec_hw.c, hevc_irq_thread()
invokes the callback without locking:

static irqreturn_t hevc_irq_thread(int irq, void *priv)
{
	struct aml_vdec_hw *hw = priv;

	if (hw->hevc_irq_thread_fn)
		return hw->hevc_irq_thread_fn(irq, hw->hevc_priv);

	return IRQ_HANDLED;
}

If hevc_irq_thread() is running or about to run when aml_hevc_exit() executes,
can h265_irq_thread() be called with priv == NULL or continue accessing h and
its DMA buffers after h265_free(h) and kfree(h) have freed them?

> +
> +int aml_hevc_dec_run(void *priv)
> +{
> +	struct aml_vdec_ctx *ctx = priv;
> +	struct aml_hevc_ctx *h = ctx->codec_priv;
> +	struct aml_vdec_hw *hw = h->hw;

[Severity: High]
In drivers/media/platform/amlogic/vdec/aml_vdec.c, vb2ops_vdec_stop_streaming()
calls aml_vdec_release_instance(ctx) -> aml_hevc_exit(ctx) (setting
ctx->codec_priv = NULL) when either the OUTPUT or CAPTURE queue stops, whereas
vb2ops_vdec_start_streaming() only calls aml_vdec_init_dec_inst(ctx) when the
OUTPUT queue starts.

If userspace stops and restarts the CAPTURE queue while OUTPUT remains
streaming, ctx->codec_priv is NULL when aml_hevc_dec_run() is called. Won't
dereferencing h->hw here cause a NULL pointer dereference?

[ ... ]

> +	/* Uninterruptible on purpose - see aml_vp9_dec_run(). */
> +	ret = wait_event_timeout(ctx->queue, ctx->int_cond,
> +				 msecs_to_jiffies(H265_TIMEOUT_MS));
> +	ctx->int_cond = 0;
> +	if (!ret) {
> +		dev_err(hw->dev, "hevc: decode timeout, status %02x pc %08x shift %08x\n",
> +			hw->hevc_irq_status, read_dos_reg(hw, HEVC_MPC_E),
> +			read_dos_reg(hw, HEVC_SHIFT_BYTE_COUNT));
> +		wr(hw, HEVC_MPSR, 0);
> +		h->cur = -1;
> +		hw->hevc_dirty = true;
> +		return -ETIMEDOUT;
> +	}
> +	return 0;
> +}

[Severity: High]
When wait_event_timeout() times out here in aml_hevc_dec_run(), there appear
to be two recovery issues:

1. The timeout path sets h->cur = -1 and returns -ETIMEDOUT without masking
   HEVC_ASSIST_MBOX0_MASK, setting h->finished = true, or calling
   synchronize_irq(hw->hevc_irq). If h265_irq_thread() is running concurrently
   (for example sleeping in h265_alloc_pics() inside h265_slice_header()),
   setting h->cur = -1 races with h265_fill_mmu_map(), h265_config_mpred(),
   and h265_config_sao() indexing h->pic[h->cur] (reading h->pic[-1] out of
   bounds), and h265_finish() can race with err_cancel_job in
   m2mops_vdec_device_run() to complete the request and M2M job twice.

2. Although hw->hevc_dirty is set to true on timeout, aml_hevc_dec_run() only
   calls hvdec_switch_codec() and hevc_load_fw() when !h->ucode_loaded:

	if (!h->ucode_loaded) {
		hvdec_switch_codec(hw, CODEC_TYPE_HEVC);
		ret = hevc_load_fw(hw, CODEC_TYPE_HEVC, H265_FW_FORMAT_MMU,
				   H265_FW_CORE_HEVC, "hevc");

   Because h->ucode_loaded is not reset to false on timeout (and hw->hevc_dirty
   is not set when decoding 10-bit frames), won't subsequent frames in the
   same context skip cycling the core memories and reloading the firmware?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261025192507.210058-1-christian.koever-draxl@student.uibk.ac.at?part=9