drm/vc4: txp: fix writeback dimension checks and normalize rotation

[name2965]

If the rotation value is 0, it can be compared to an accurate bitmask, thereby bypassing size validation; furthermore, frame buffer size validation is so inadequate that it cannot reliably filter out write buffers that are too small.

This results in a DMA OOB vulnerability on Raspberry Pi models prior to the 5 that lack an IOMMU, and I have successfully reproduced this vulnerability.

I tested this immediately by installing the Raspberry Pi OS 64-bit on a Raspberry Pi 4 Model B rev 1.2 using Imager.

This issue occurs on Raspberry Pi without IOMMU, and DMA OOB can be easily reproduced in any kernel version where vulnerable code exists, in addition to the kernel version I reproduced.

repro.mp4

[popcornmix]

@6by9 ?

[6by9]

As per the comments on the original code below, there are two && which should be ||, but that is the full extent of the bug.

drm_rotation_simplify adds nothing to this check in my opinion because it doesn't understand transpose (downstream only thing). If it unpacked eg ROT_90 | REFLECT_X (or whichever combination it is) into ROT_0 | TRANSPOSE then it'd be lovely, but it doesn't and there is no need.

Expanding out the tests into multiple tests (some with no error message logged) makes the diff from mainline bigger, and hence a larger maintenance burden. Minimal changes are good here.

[name2965]

As per the comments on the original code below, there are two && which should be ||, but that is the full extent of the bug.

drm_rotation_simplify adds nothing to this check in my opinion because it doesn't understand transpose (downstream only thing). If it unpacked eg ROT_90 | REFLECT_X (or whichever combination it is) into ROT_0 | TRANSPOSE then it'd be lovely, but it doesn't and there is no need.

Expanding out the tests into multiple tests (some with no error message logged) makes the diff from mainline bigger, and hence a larger maintenance burden. Minimal changes are good here.

I agree that drm_rotation_simplify is unnecessary. However, simply changing the conditional operator and adding parentheses is not enough to resolve this issue.

The cause of the problem occurring in vc4_txp_connector_atomic_check is partly due to the incorrect use of the conditional operator, but also because atomic_check is called while rotation remains initialized to NULL. Consequently, even if the conditional operator is modified, the rotation remains NULL, which bypasses width and height checks and can trigger a DMA OOB vulnerability.

This was definitely not a normal flow, so after debugging in detail to see why rotation was becoming NULL, I realized that because &drm_connector_funcs.reset was not defined separately to initialize rotation and the default function was still being used, rotation remained NULL until it was initialized separately in user space, and this continued up to atomic_check.

Therefore, I will modify the existing patch and write and push an additional patch to resolve the rotation initialization issue.

[name2965]

Oh, and if you need the reproduction code for the review, I will send it to you when I get home later.

[name2965]

repro.zip

This is the reproduction code I wrote for testing. I modified the code reproduced in the video by removing as many hardcoded values ​​as possible and reinforcing the logic to ensure it works well in other environments, but it may not function properly.

[pelwell]

I'm happy with these commits (trusting that the reset method is necessary), but note that 6.12 is essentially frozen except for bug fixes - of which this is one.

[name2965]

I'm happy with these commits (trusting that the reset method is necessary), but note that 6.12 is essentially frozen except for bug fixes - of which this is one.

Of course. I think that adding a custom reset function is the best way to prevent DMA OOB bug caused by this uninitialize rotation var.
However, if there are no further changes in the current 6.12, in which version of the Raspberry Pi kernel is new feature development carried out?

[pelwell]

We've jumped on to the next LTS - 6.18. This is already shipping as our default rpi-update kernel, and the Trixie kernel will be moved onto 6.18 in the very near future (assuming no showstopper bugs are found).

[6by9]

I'll give your test a quick run later today.

[6by9]

Based on the pattern with vc4_vec_connector_reset (in vc4_vec.c), it is safe to set the default after __drm_atomic_helper_connector_reset has been called.

The open coding of drm_atomic_helper_connector_reset can therefore be replaced by a call to it, and then check for a default value of rotation and set it.

[name2965]

Based on the pattern with vc4_vec_connector_reset (in vc4_vec.c), it is safe to set the default after __drm_atomic_helper_connector_reset has been called.

The open coding of drm_atomic_helper_connector_reset can therefore be replaced by a call to it, and then check for a default value of rotation and set it.

In VC4, since the drm_connector_state struct is not subclassed separately, I was debating whether to call drm_atomic_helper_connector_reset() first.

However, I was concerned that initializing the connector->state pointer beforehand might cause concurrency issues, so I safely implemented it to call reset after the state is initialized.

Will there really be no problems if I call drm_atomic_helper_connector_reset() first?

[6by9]

If there were an issue, the mainline devs wouldn't have written:
https://github.com/torvalds/linux/blob/master/drivers/gpu/drm/vc4/vc4_vec.c#L381-L385

static void vc4_vec_connector_reset(struct drm_connector *connector)
{
	drm_atomic_helper_connector_reset(connector);
	drm_atomic_helper_connector_tv_reset(connector);
}

and the same in https://github.com/torvalds/linux/blob/master/drivers/gpu/drm/sun4i/sun4i_tv.c#L404-L408

static void sun4i_tv_connector_reset(struct drm_connector *connector)
{
	drm_atomic_helper_connector_reset(connector);
	drm_atomic_helper_connector_tv_reset(connector);
}

The implementation of drm_atomic_helper_connector_tv_reset is doing exactly the same thing as you, going in to set properties to their default values https://github.com/torvalds/linux/blob/master/drivers/gpu/drm/drm_atomic_state_helper.c#L504-L577

[name2965]

That make sense thanks! I will write the patch right away, test it, and push new commit.

[6by9]

Other than being against 6.12, that looks good. Cherry-picked to 6.18 and it passes your test case.

Ignore checkpatch's complaint on u64 instead of uint64_t in this case.