[linux-morello] Re: [PATCH 3/3] drm: fix up purecap handling of all iotcls in libdrm test tools

On 05/04/2024 12:20, carsten.haitzler@foss.arm.com wrote:

[...]

@@ -2919,16 +2962,9 @@ int drm_mode_getconnector(struct drm_device *dev, void *data, copied++; } }

• out_resp->count_encoders = encoders_count;
• out_resp->connector_id = connector->base.id;
• out_resp->connector_type = connector->connector_type;
• out_resp->connector_type_id = connector->connector_type_id;
• is_current_master = drm_is_current_master(file_priv);

Nit: probably want to keep the empty lines around that line.

mutex_lock(&dev->mode_config.mutex);

• if (out_resp->count_modes == 0) {

• if (count_modes == 0) { if (is_current_master) connector->funcs->fill_modes(connector, dev->mode_config.max_width,

@@ -2938,11 +2974,6 @@ int drm_mode_getconnector(struct drm_device *dev, void *data, connector->base.id, connector->name); }

• out_resp->mm_width = connector->display_info.width_mm;
• out_resp->mm_height = connector->display_info.height_mm;
• out_resp->subpixel = connector->display_info.subpixel_order;
• out_resp->connection = connector->status;
• /* delayed so we get modes regardless of pre-fill_modes state */ list_for_each_entry(mode, &connector->modes, head) { WARN_ON(mode->expose_to_userspace);

@@ -2958,9 +2989,8 @@ int drm_mode_getconnector(struct drm_device *dev, void *data, * This ioctl is called twice, once to determine how much space is * needed, and the 2nd time to fill it. */

• if ((out_resp->count_modes >= mode_count) && mode_count) {

• if ((count_modes >= mode_count) && mode_count) { copied = 0;

• mode_ptr = uaddr_to_user_ptr(out_resp->modes_ptr);
  

  list_for_each_entry(mode, &connector->modes, head) { if (!mode->expose_to_userspace) continue;

@@ -2998,25 +3028,50 @@ int drm_mode_getconnector(struct drm_device *dev, void *data, mode->expose_to_userspace = false; }

• out_resp->count_modes = mode_count; mutex_unlock(&dev->mode_config.mutex);

drm_modeset_lock(&dev->mode_config.connection_mutex, NULL); encoder = drm_connector_get_encoder(connector);

• if (encoder)

• out_resp->encoder_id = encoder->base.id;
  

• else

• out_resp->encoder_id = 0;
  

/* Only grab properties after probing, to make sure EDID and other * properties reflect the latest status. */ ret = drm_mode_object_get_properties(&connector->base, file_priv->atomic,

• 	uaddr_to_user_ptr(out_resp->props_ptr),
  

• 	uaddr_to_user_ptr(out_resp->prop_values_ptr),
  

• 	&out_resp->count_props);
  

• 	prop_ptr, prop_values,
  

• 	&count_props);
  

I think this will result in UB, because drm_mode_object_get_properties() reads from arg_count_props (last argument) *before* writing to it, and count_props is uninitialised at this point. Maybe we should initialise it like the other properties at the beginning of this function.

drm_modeset_unlock(&dev->mode_config.connection_mutex);

• if (drm_test_compat64()) {

• out_resp32->count_encoders = encoders_count;
  

• out_resp32->count_modes = mode_count;
  

• out_resp32->count_props = count_props;
  

• out_resp32->connector_id = connector->base.id;
  

• out_resp32->connector_type = connector->connector_type;
  

• out_resp32->connector_type_id = connector->connector_type_id;
  

• if (encoder)
  

• 	out_resp32->encoder_id = encoder->base.id;
  

• else
  

• 	out_resp32->encoder_id = 0;
  

• out_resp32->mm_width = connector->display_info.width_mm;
  

• out_resp32->mm_height = connector->display_info.height_mm;
  

• out_resp32->subpixel = connector->display_info.subpixel_order;
  

• out_resp32->connection = connector->status;
  

• } else {

• out_resp->count_encoders = encoders_count;
  

• out_resp->count_modes = mode_count;
  

• out_resp->count_props = count_props;
  

• out_resp->connector_id = connector->base.id;
  

• out_resp->connector_type = connector->connector_type;
  

• out_resp->connector_type_id = connector->connector_type_id;
  

• if (encoder)
  

• 	out_resp->encoder_id = encoder->base.id;
  

• else
  

• 	out_resp->encoder_id = 0;
  

• out_resp->mm_width = connector->display_info.width_mm;
  

• out_resp->mm_height = connector->display_info.height_mm;
  

• out_resp->subpixel = connector->display_info.subpixel_order;
  

• out_resp->connection = connector->status;
  

• }

out: drm_connector_put(connector); diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c index 8525ef851540..eab94e74b56a 100644 --- a/drivers/gpu/drm/drm_mode_config.c +++ b/drivers/gpu/drm/drm_mode_config.c @@ -92,11 +92,30 @@ int drm_mode_getresources(struct drm_device *dev, void *data, struct drm_file *file_priv) { struct drm_mode_card_res *card_res = data;

• struct drm_mode_card_res32 {

• __u64 fb_id_ptr;
  

• __u64 crtc_id_ptr;
  

• __u64 connector_id_ptr;
  

• __u64 encoder_id_ptr;
  

• __u32 count_fbs;
  

• __u32 count_crtcs;
  

• __u32 count_connectors;
  

• __u32 count_encoders;
  

• __u32 min_width;
  

• __u32 max_width;
  

• __u32 min_height;
  

• __u32 max_height;
  

• };
• struct drm_mode_card_res32 *card_res32 = data; struct drm_framebuffer *fb; struct drm_connector *connector; struct drm_crtc *crtc; struct drm_encoder *encoder;

• int count, ret = 0;

• int count_fb = 0, count_fbs;
• int count_crtc = 0, count_crtcs;
• int count_encoder = 0, count_encoders;
• int count_connector = 0, count_connectors;
• int ret = 0; uint32_t __user *fb_id; uint32_t __user *crtc_id; uint32_t __user *connector_id;

@@ -107,49 +126,54 @@ int drm_mode_getresources(struct drm_device *dev, void *data, return -EOPNOTSUPP; mutex_lock(&file_priv->fbs_lock);

• count = 0;
• fb_id = u64_to_user_ptr(card_res->fb_id_ptr);

• if (drm_test_compat64()) {

• fb_id        = compat_ptr(card_res32->fb_id_ptr);
  

• crtc_id      = compat_ptr(card_res32->crtc_id_ptr);
  

• encoder_id   = compat_ptr(card_res32->encoder_id_ptr);
  

• connector_id = compat_ptr(card_res32->connector_id_ptr);
  

• count_fbs        = card_res32->count_fbs;
  

• count_crtcs      = card_res32->count_crtcs;
  

• count_encoders   = card_res32->count_encoders;
  

• count_connectors = card_res32->count_connectors;
  

• } else {

• fb_id        = (uint32_t __user *)(card_res->fb_id_ptr);
  

• crtc_id      = (uint32_t __user *)(card_res->crtc_id_ptr);
  

• encoder_id   = (uint32_t __user *)(card_res->encoder_id_ptr);
  

• connector_id = (uint32_t __user *)(card_res->connector_id_ptr);
  

• count_fbs        = card_res->count_fbs;
  

• count_crtcs      = card_res->count_crtcs;
  

• count_encoders   = card_res->count_encoders;
  

• count_connectors = card_res->count_connectors;
  

• } list_for_each_entry(fb, &file_priv->fbs, filp_head) {

• if (count < card_res->count_fbs &&
  

•     put_user(fb->base.id, fb_id + count)) {
  

• 	mutex_unlock(&file_priv->fbs_lock);
  

• 	return -EFAULT;
  

• if (count_fb < count_fbs &&
  

•     put_user(fb->base.id, fb_id + count_fb)) {
  

• 	ret = -EFAULT;
  

• 	goto error;
  

  }

• count++;
  

• count_fb++;
  

  }

• card_res->count_fbs = count;
• mutex_unlock(&file_priv->fbs_lock);

I can't quite convince myself that postponing the mutex_unlock() until the end of the function is a good idea or even correct. Either way it seems completely unrelated to the rest of the patch.

[...]

int drm_mode_getplane(struct drm_device *dev, void *data, struct drm_file *file_priv) {

• struct drm_mode_get_plane32 {

• __u32 plane_id;
  

• __u32 crtc_id;
  

• __u32 fb_id;
  

• __u32 possible_crtcs;
  

• __u32 gamma_size;
  

• __u32 count_format_types;
  

• __u64 format_type_ptr;
  

• };
• struct drm_mode_get_plane32 *plane_resp32 = data; struct drm_mode_get_plane *plane_resp = data; struct drm_plane *plane; uint32_t __user *format_ptr;
• __u32 plane_id, count_format_types, crtc_id, fb_id, possible_crtcs, gamma_size;

if (!drm_core_check_feature(dev, DRIVER_MODESET)) return -EOPNOTSUPP;

• plane = drm_plane_find(dev, file_priv, plane_resp->plane_id);

• if (drm_test_compat64()) {

• format_ptr = uaddr_to_user_ptr(plane_resp32->format_type_ptr);
  

• plane_id = plane_resp32->plane_id;
  

• count_format_types = plane_resp32->count_format_types;
  

• } else {

• format_ptr = uaddr_to_user_ptr(plane_resp->format_type_ptr);
  

The pointer conversions need to be updated here (compat_ptr / simple cast).

• plane_id = plane_resp->plane_id;
  

• count_format_types = plane_resp->count_format_types;
  

• }
• plane = drm_plane_find(dev, file_priv, plane_id); if (!plane) return -ENOENT;

drm_modeset_lock(&plane->mutex, NULL); if (plane->state && plane->state->crtc && drm_lease_held(file_priv, plane->state->crtc->base.id))

• plane_resp->crtc_id = plane->state->crtc->base.id;
  

• crtc_id = plane->state->crtc->base.id;
  

  else if (!plane->state && plane->crtc && drm_lease_held(file_priv, plane->crtc->base.id))

• plane_resp->crtc_id = plane->crtc->base.id;
  

• crtc_id = plane->crtc->base.id;
  

  else

• plane_resp->crtc_id = 0;
  

• crtc_id = 0;
  

if (plane->state && plane->state->fb)

• plane_resp->fb_id = plane->state->fb->base.id;
  

• fb_id = plane->state->fb->base.id;
  

  else if (!plane->state && plane->fb)

• plane_resp->fb_id = plane->fb->base.id;
  

• fb_id = plane->fb->base.id;
  

  else

• plane_resp->fb_id = 0;
  

• fb_id = 0;
  

  drm_modeset_unlock(&plane->mutex);

• plane_resp->plane_id = plane->base.id;
• plane_resp->possible_crtcs = drm_lease_filter_crtcs(file_priv,

• 					    plane->possible_crtcs);
  

• plane_id = plane->base.id;
• possible_crtcs = drm_lease_filter_crtcs(file_priv,

• 				plane->possible_crtcs);
  

• plane_resp->gamma_size = 0;

• gamma_size = 0;

/* * This ioctl is called twice, once to determine how much space is * needed, and the 2nd time to fill it. */ if (plane->format_count &&

•    (plane_resp->count_format_types >= plane->format_count)) {
  

• format_ptr = uaddr_to_user_ptr(plane_resp->format_type_ptr);
  

•    (count_format_types >= plane->format_count)) {
  

  if (copy_to_user(format_ptr, plane->format_types, sizeof(uint32_t) * plane->format_count)) {

@@ -741,6 +781,22 @@ int drm_mode_getplane(struct drm_device *dev, void *data, } plane_resp->count_format_types = plane->format_count;

Should be deleted (it's now in the if/else below).

• if (drm_test_compat64()) {

• plane_resp32->crtc_id = crtc_id;
  

• plane_resp32->fb_id = fb_id;
  

• plane_resp32->plane_id = plane_id;
  

• plane_resp32->possible_crtcs = possible_crtcs;
  

• plane_resp32->gamma_size = gamma_size;
  

• plane_resp32->count_format_types = count_format_types;
  

• } else {

• plane_resp->crtc_id = crtc_id;
  

• plane_resp->fb_id = fb_id;
  

• plane_resp->plane_id = plane_id;
  

• plane_resp->possible_crtcs = possible_crtcs;
  

• plane_resp->gamma_size = gamma_size;
  

• plane_resp->count_format_types = count_format_types;
  

• }
• return 0;

} diff --git a/drivers/gpu/drm/drm_property.c b/drivers/gpu/drm/drm_property.c index dfec479830e4..6ae86d4510a8 100644 --- a/drivers/gpu/drm/drm_property.c +++ b/drivers/gpu/drm/drm_property.c @@ -457,6 +457,16 @@ EXPORT_SYMBOL(drm_property_destroy); int drm_mode_getproperty_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv) {

• struct drm_mode_get_property32 {

• __u64 values_ptr;
  

• __u64 enum_blob_ptr;
  

• __u32 prop_id;
  

• __u32 flags;
  

• char name[DRM_PROP_NAME_LEN];
  

• __u32 count_values;
  

• __u32 count_enum_blobs;
  

• };
• struct drm_mode_get_property32 *out_resp32 = data; struct drm_mode_get_property *out_resp = data; struct drm_property *property; int enum_count = 0;

@@ -465,22 +475,39 @@ int drm_mode_getproperty_ioctl(struct drm_device *dev, struct drm_property_enum *prop_enum; struct drm_mode_property_enum __user *enum_ptr; uint64_t __user *values_ptr;

• __u32 prop_id, flags, count_values, count_enum_blobs;
• char *name;

if (!drm_core_check_feature(dev, DRIVER_MODESET)) return -EOPNOTSUPP;

• property = drm_property_find(dev, file_priv, out_resp->prop_id);

• if (drm_test_compat64()) {

• values_ptr = compat_ptr(out_resp32->values_ptr);
  

• enum_ptr = compat_ptr(out_resp32->enum_blob_ptr);
  

• prop_id = out_resp32->prop_id;
  

• name = out_resp32->name;
  

• count_values = out_resp32->count_values;
  

• count_enum_blobs = out_resp32->count_enum_blobs;
  

• } else {

• values_ptr = (uint64_t __user *)out_resp->values_ptr;
  

• enum_ptr = (struct drm_mode_property_enum __user *)out_resp->enum_blob_ptr;
  

• prop_id = out_resp->prop_id;
  

• name = out_resp->name;
  

• count_values = out_resp->count_values;
  

• count_enum_blobs = out_resp->count_enum_blobs;
  

• }
• property = drm_property_find(dev, file_priv, prop_id); if (!property) return -ENOENT;

• strscpy_pad(out_resp->name, property->name, DRM_PROP_NAME_LEN);
• out_resp->flags = property->flags;

• strscpy_pad(name, property->name, DRM_PROP_NAME_LEN);
• flags = property->flags;

value_count = property->num_values;

• values_ptr = u64_to_user_ptr(out_resp->values_ptr);

for (i = 0; i < value_count; i++) {

• if (i < out_resp->count_values &&
  

• if (i < count_values &&
  

  put_user(property->values[i], values_ptr + i)) { return -EFAULT; }

@@ -488,13 +515,12 @@ int drm_mode_getproperty_ioctl(struct drm_device *dev, out_resp->count_values = value_count;

Should be deleted too.

copied = 0;

• enum_ptr = u64_to_user_ptr(out_resp->enum_blob_ptr);

if (drm_property_type_is(property, DRM_MODE_PROP_ENUM) || drm_property_type_is(property, DRM_MODE_PROP_BITMASK)) { list_for_each_entry(prop_enum, &property->enum_list, head) { enum_count++;

• 	if (out_resp->count_enum_blobs < enum_count)
  

• 	if (count_enum_blobs < enum_count)
  	continue;
  

if (copy_to_user(&enum_ptr[copied].value, @@ -506,7 +532,7 @@ int drm_mode_getproperty_ioctl(struct drm_device *dev, return -EFAULT; copied++; }

• out_resp->count_enum_blobs = enum_count;
  

• count_enum_blobs = enum_count;
  

  }

/* @@ -518,7 +544,18 @@ int drm_mode_getproperty_ioctl(struct drm_device *dev, * the property itself. */ if (drm_property_type_is(property, DRM_MODE_PROP_BLOB))

• out_resp->count_enum_blobs = 0;
  

• count_enum_blobs = 0;
  

Nit: empty line here.

• if (drm_test_compat64()) {

• out_resp32->prop_id = prop_id;
  

• out_resp32->flags = flags;
  

• out_resp32->count_values = count_values;
  

• out_resp32->count_enum_blobs = count_enum_blobs;
  

• } else {

• out_resp->prop_id = prop_id;
  

• out_resp->flags = flags;
  

• out_resp->count_values = count_values;
  

• out_resp->count_enum_blobs = count_enum_blobs;
  

• }

return 0; } @@ -754,29 +791,53 @@ EXPORT_SYMBOL(drm_property_replace_blob); int drm_mode_getblob_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv) {

• struct drm_mode_get_blob32 {

• __u32 blob_id;
  

• __u32 length;
  

• __u64 data;
  

• };
• struct drm_mode_get_blob32 *out_resp32 = data; struct drm_mode_get_blob *out_resp = data; struct drm_property_blob *blob; int ret = 0;
• __u32 blob_id;
• __u32 length;
• uint8_t __user *blob_data;
• if (drm_test_compat64()) {

• blob_id = out_resp32->blob_id;
  

• length = out_resp32->length;
  

• blob_data = compat_ptr(out_resp32->data);
  

• } else {

• blob_id = out_resp->blob_id;
  

• length = out_resp->length;
  

• blob_data = (uint8_t __user *)(out_resp->data);
  

• }

if (!drm_core_check_feature(dev, DRIVER_MODESET)) return -EOPNOTSUPP;

• blob = drm_property_lookup_blob(dev, out_resp->blob_id);

• blob = drm_property_lookup_blob(dev, blob_id); if (!blob) return -ENOENT;

• if (out_resp->length == blob->length) {

• if (copy_to_user(u64_to_user_ptr(out_resp->data),
  

• if (length == blob->length) {

• if (copy_to_user(blob_data,
  	 blob->data,
  	 blob->length)) {
  ret = -EFAULT;
  goto unref;
  

  } }

• out_resp->length = blob->length;

• length = blob->length;

unref: drm_property_blob_put(blob);

• if (drm_test_compat64()) {

• out_resp32->length = length;
  

• } else {

• out_resp->length = length;
  

• } return ret;

} diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h index e2640dc64e08..05e048c16c10 100644 --- a/include/drm/drm_drv.h +++ b/include/drm/drm_drv.h @@ -34,6 +34,15 @@ #include <drm/drm_device.h> +static inline bool drm_test_compat64(void)

We now have in_compat64_syscall() that does exactly the same thing (but isn't arm64-specific).

+{ +#ifdef CONFIG_COMPAT

• return test_thread_flag(TIF_64BIT_COMPAT);

+#else

• return false;

+#endif +}

[...] struct drm_mode_fb_cmd { @@ -1029,7 +1029,7 @@ struct drm_mode_crtc_page_flip_target { __u32 fb_id; __u32 flags; __u32 sequence;

• __u64 user_data;

• __kernel_uintptr_t user_data;

Aren't we missing the corresponding compat handling for this change?

}; /** @@ -1135,7 +1135,7 @@ struct drm_mode_atomic { __u64 props_ptr; __u64 prop_values_ptr; __u64 reserved;

• __u64 user_data;

• __kernel_uintptr_t user_data;

Ditto here.

Kevin

}; struct drm_format_modifier_blob { diff --git a/tools/include/uapi/drm/drm.h b/tools/include/uapi/drm/drm.h index de723566c5ae..7a0524d7d9f5 100644 --- a/tools/include/uapi/drm/drm.h +++ b/tools/include/uapi/drm/drm.h @@ -971,7 +971,7 @@ struct drm_crtc_queue_sequence { __u32 crtc_id; __u32 flags; __u64 sequence; /* on input, target sequence. on output, actual sequence */

• __u64 user_data; /* user data passed to event */

• uintptr_t user_data; /* user data passed to event */

}; #if defined(__cplusplus) @@ -1289,7 +1289,7 @@ struct drm_event_vblank { */ struct drm_event_crtc_sequence { struct drm_event base;

• __u64 user_data;

• uintptr_t user_data; __s64 time_ns; __u64 sequence;

};