On 30/11/2021 14:25, Zhou Qingyang wrote:
> In komeda_plane_add(), komeda_get_layer_fourcc_list() is assigned to
> formats and used in drm_universal_plane_init().
> drm_universal_plane_init() passes formats to
> __drm_universal_plane_init(). __drm_universal_plane_init() further
> passes formats to memcpy() as src parameter, which could lead to an
> undefined behavior bug on failure of komeda_get_layer_fourcc_list().
>
> Fix this bug by adding a check of formats.
>
> This bug was found by a static analyzer. The analysis employs
> differential checking to identify inconsistent security operations
> (e.g., checks or kfrees) between two code paths and confirms that the
> inconsistent operations are not recovered in the current function or
> the callers, so they constitute bugs.
>
> Note that, as a bug found by static analysis, it can be a false
> positive or hard to trigger. Multiple researchers have cross-reviewed
> the bug.
>
> Builds with CONFIG_DRM_KOMEDA=m show no new warnings,
> and our static analyzer no longer warns about this code.
>
> Fixes: 61f1c4a8ab75 ("drm/komeda: Attach komeda_dev to DRM-KMS")
> Signed-off-by: Zhou Qingyang <zhou1615@xxxxxxx>
> ---
> drivers/gpu/drm/arm/display/komeda/komeda_plane.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_plane.c b/drivers/gpu/drm/arm/display/komeda/komeda_plane.c
> index d63d83800a8a..dd3f17e970dd 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_plane.c
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_plane.c
> @@ -265,6 +265,10 @@ static int komeda_plane_add(struct komeda_kms_dev *kms,
>
> formats = komeda_get_layer_fourcc_list(&mdev->fmt_tbl,
> layer->layer_type, &n_formats);
> + if (!formats) {
> + err = -ENOMEM;
> + goto cleanup;
> + }
If this executes it will cause undefined behaviour...
The cleanup code calls komeda_plane_destroy() which calls
drm_plane_cleanup() which does (amongst other things) a
list_del(&plane->head). But the plane hasn't been put on a list yet as
that's done in drm_universal_plane_init().
So in this case we simple want to do:
if (!formats) {
kfree(kplane);
return -ENOMEM;
}
Note that without this 'fix' a NULL return from
komeda_get_layer_fourcc_list() would leave n_formats==0, so while the
NULL pointer is passed into memcpy() it is also passed a length of 0.
Which I believe is safe.
However while looking at this function...
>
> err = drm_universal_plane_init(&kms->base, plane,
> get_possible_crtcs(kms, c->pipeline),
>
This call to drm_universal_plane_init() can fail early before
plane->head has been initialised. In which case the following:
> komeda_put_fourcc_list(formats);
>
> if (err)
> goto cleanup;
commits the exact same sin and would cause a similar NULL dereference in
drm_plane_cleanup().
Steve
