- 15 Jul, 2022 5 commits
-
-
Tom Rix authored
clang static analysis reports drivers/gpu/drm/nouveau/nvkm/subdev/bios/pmu.c:68:17: warning: The right operand of '*' is a garbage value [core.UndefinedBinaryOperatorResult] switch (!!data * *ver) { ^ ~~~~ A switch statement with only a default should be reduced to an if. If nvbios_pmuEp() returns 0, via the data variable, the output info parameter is not used. So set info only when data is not 0. The struct nvbios_pmuE only has the type and data elements. Since both of these are explicitly set, memset is not needed. So remove it. Signed-off-by:Tom Rix <trix@redhat.com> Reviewed-by:
Lyude Paul <lyude@redhat.com> Signed-off-by:
-
Christian König authored
This re-applys commit 708d19d9. The original problem this was reverted for was found and the correct fix will be merged to drm-misc-next-fixes. Signed-off-by:
Christian König <christian.koenig@amd.com> Reviewed-by:
Arunpravin Paneer Selvam <Arunpravin.PaneerSelvam@amd.com> Link: https://patchwork.freedesktop.org/patch/msgid/20220714132315.587217-2-christian.koenig@amd.com
-
Christian König authored
This re-applys commit 5e3f1e77. The original problem this was reverted for was found and the correct fix will be merged to drm-misc-next-fixes. Signed-off-by:
-
Javier Martinez Canillas authored
There are some struct drm_driver fields that are required by drivers since drm_copy_field() attempts to copy them to user-space via DRM_IOCTL_VERSION. But it can be possible that a driver has a bug and did not set some of the fields, which leads to drm_copy_field() attempting to copy a NULL pointer: [ +10.395966] Unable to handle kernel access to user memory outside uaccess routines at virtual address 0000000000000000 [ +0.010955] Mem abort info: [ +0.002835] ESR = 0x0000000096000004 [ +0.003872] EC = 0x25: DABT (current EL), IL = 32 bits [ +0.005395] SET = 0, FnV = 0 [ +0.003113] EA = 0, S1PTW = 0 [ +0.003182] FSC = 0x04: level 0 translation fault [ +0.004964] Data abort info: [ +0.002919] ISV = 0, ISS = 0x00000004 [ +0.003886] CM = 0, WnR = 0 [ +0.003040] user pgtable: 4k pages, 48-bit VAs, pgdp=0000000115dad000 [ +0.006536] [0000000000000000] pgd=0000000000000000, p4d=0000000000000000 [ +0.006925] Internal error: Oops: 96000004 [#1] SMP ... [ +0.011113] pstate: 80400005 (Nzcv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--) [ +0.007061] pc : __pi_strlen+0x14/0x150 [ +0.003895] lr : drm_copy_field+0x30/0x1a4 [ +0.004156] sp : ffff8000094b3a50 [ +0.003355] x29: ffff8000094b3a50 x28: ffff8000094b3b70 x27: 0000000000000040 [ +0.007242] x26: ffff443743c2ba00 x25: 0000000000000000 x24: 0000000000000040 [ +0.007243] x23: ffff443743c2ba00 x22: ffff8000094b3b70 x21: 0000000000000000 [ +0.007241] x20: 0000000000000000 x19: ffff8000094b3b90 x18: 0000000000000000 [ +0.007241] x17: 0000000000000000 x16: 0000000000000000 x15: 0000aaab14b9af40 [ +0.007241] x14: 0000000000000000 x13: 0000000000000000 x12: 0000000000000000 [ +0.007239] x11: 0000000000000000 x10: 0000000000000000 x9 : ffffa524ad67d4d8 [ +0.007242] x8 : 0101010101010101 x7 : 7f7f7f7f7f7f7f7f x6 : 6c6e6263606e7141 [ +0.007239] x5 : 0000000000000000 x4 : 0000000000000000 x3 : 0000000000000000 [ +0.007241] x2 : 0000000000000000 x1 : ffff8000094b3b90 x0 : 0000000000000000 [ +0.007240] Call trace: [ +0.002475] __pi_strlen+0x14/0x150 [ +0.003537] drm_version+0x84/0xac [ +0.003448] drm_ioctl_kernel+0xa8/0x16c [ +0.003975] drm_ioctl+0x270/0x580 [ +0.003448] __arm64_sys_ioctl+0xb8/0xfc [ +0.003978] invoke_syscall+0x78/0x100 [ +0.003799] el0_svc_common.constprop.0+0x4c/0xf4 [ +0.004767] do_el0_svc+0x38/0x4c [ +0.003357] el0_svc+0x34/0x100 [ +0.003185] el0t_64_sync_handler+0x11c/0x150 [ +0.004418] el0t_64_sync+0x190/0x194 [ +0.003716] Code: 92402c04 b200c3e8 f13fc09f 5400088c (a9400c02) [ +0.006180] ---[ end trace 0000000000000000 ]--- Reported-by:
Peter Robinson <pbrobinson@gmail.com> Signed-off-by:
Javier Martinez Canillas <javierm@redhat.com> Acked-by:
Thomas Zimmermann <tzimmermann@suse.de> Link: https://patchwork.freedesktop.org/patch/msgid/20220705100215.572498-3-javierm@redhat.com
-
Javier Martinez Canillas authored
The strlen() function returns a size_t which is an unsigned int on 32-bit arches and an unsigned long on 64-bit arches. But in the drm_copy_field() function, the strlen() return value is assigned to an 'int len' variable. Later, the len variable is passed as copy_from_user() third argument that is an unsigned long parameter as well. In theory, this can lead to an integer overflow via type conversion. Since the assignment happens to a signed int lvalue instead of a size_t lvalue. In practice though, that's unlikely since the values copied are set by DRM drivers and not controlled by userspace. But using a size_t for len is the correct thing to do anyways. Signed-off-by:
-
- 13 Jul, 2022 35 commits
-
-
Maxime Ripard authored
devm_pm_runtime_enable() simplifies the driver a bit since it will call pm_runtime_disable() automatically through a device-managed action. Acked-by:
Maxime Ripard <maxime@cerno.tech> Link: https://lore.kernel.org/r/20220711173939.1132294-70-maxime@cerno.tech
-
Maxime Ripard authored
At bind time, vc4_v3d_bind() will read a register to retrieve the v3d version and make sure it's a version we're compatible with. However, the v3d has an optional clock that is enabled only after the register read-out and a power domain that wasn't enabled at all in the bind implementation. This was working fine at boot because both were enabled, but resulted in the version check failing if we were unbinding and rebinding the driver because the unbinding would have turned them off. The fix isn't as easy as calling pm_runtime_resume_and_get() prior to the register access to power up the power domain though. Indeed, the runtime_resume implementation will enable the clock mentioned above, call vc4_v3d_init_hw() and then vc4_irq_enable(). Prior to the previous patch, vc4_irq_enable() needed to occur after our call to platform_get_irq() and vc4_irq_install(), since vc4_irq_enable() used to call enable_irq() and vc4_irq_install() will call request_irq(). vc4_irq_install() will also do some register access, so needs the power domain to be on. So we ended up in a situation where vc4_v3d_runtime_resume() needed vc4_irq_install() to have been called before, and vc4_irq_install() needed vc4_v3d_runtime_resume(). The previous patch removed the enable_irq() call in vc4_irq_enable() and thus removed the dependency of vc4_v3d_runtime_resume() on vc4_irq_install(). Thus, we can now rework our bind implementation to call pm_runtime_resume_and_get() before our register access to make sure the power domain is on. vc4_v3d_runtime_resume() also takes care of turning the clock on and calling vc4_v3d_init_hw() so we can remove them from bind. Acked-by:
-
Maxime Ripard authored
The vc4_irq_disable(), among other things, will call disable_irq() to complete any in-flight interrupts. This requires its counterpart, vc4_irq_enable(), to call enable_irq() which causes issues addressed in a later patch. However, vc4_irq_disable() is called by two callees: vc4_irq_uninstall() and vc4_v3d_runtime_suspend(). vc4_irq_uninstall() also calls free_irq() which already disables the interrupt line. We thus don't require an explicit disable_irq() for that call site. vc4_v3d_runtime_suspend() doesn't have any other code. However, the rest of vc4_irq_disable() masks the interrupts coming from the v3d, so explictly disabling the interrupt line is also redundant. The only thing we really care about is thus to make sure we don't have any handler in-flight, as suggested by the comment. We can thus replace disable_irq() by synchronize_irq(). Acked-by:
-
Maxime Ripard authored
vc4_perfmon_open_file() will instantiate a mutex for that file instance, but we never call mutex_destroy () in vc4_perfmon_close_file(). Let's add that missing call. Acked-by:
-
Maxime Ripard authored
mutex_init is supposed to be balanced by a call to mutex_destroy that we were never doing in the vc4 driver. Since a DRM-managed mutex_init variant has been introduced, let's just switch to it. Acked-by:
-
Maxime Ripard authored
The vc4 has a custom API to allow components to register a debugfs file before the DRM driver has been registered and the debugfs_init hook has been called. However, the .late_register hook allows to have the debugfs file creation deferred after that time already. Let's remove our custom code to only register later our debugfs entries as part of either debugfs_init or after it. Acked-by:
-
Maxime Ripard authored
vc4_debugfs_add_file() can fail, so let's propagate its error code instead of silencing it. Acked-by:
-
Maxime Ripard authored
Our current code now mixes some resources whose lifetime are tied to the device (clocks, IO mappings, etc.) and some that are tied to the DRM device (encoder, bridge). The device one will be freed at unbind time, but the DRM one will only be freed when the last user of the DRM device closes its file handle. So we end up with a time window during which we can call the encoder hooks, but we don't have access to the underlying resources and device. Let's protect all those sections with drm_dev_enter() and drm_dev_exit() so that we bail out if we are during that window. Acked-by:
-
Maxime Ripard authored
devm_pm_runtime_enable() simplifies the driver a bit since it will call pm_runtime_disable() automatically through a device-managed action. Acked-by:
-
Maxime Ripard authored
Whenever the device and driver are unbound, the main device and all the subdevices will be removed by calling their unbind() method. However, the DRM device itself will only be freed when the last user will have closed it. It means that there is a time window where the device and its resources aren't there anymore, but the userspace can still call into our driver. Fortunately, the DRM framework provides the drm_dev_enter() and drm_dev_exit() functions to make sure our underlying device is still there for the section protected by those calls. Let's add them to the VEC driver. Acked-by:
-
Maxime Ripard authored
The current code will call drm_connector_unregister() and drm_connector_cleanup() when the device is unbound. However, by then, there might still be some references held to that connector, including by the userspace that might still have the DRM device open. Let's switch to a DRM-managed initialization to clean up after ourselves only once the DRM device has been last closed. Acked-by:
-
Maxime Ripard authored
The current code will call drm_encoder_cleanup() when the device is unbound. However, by then, there might still be some references held to that encoder, including by the userspace that might still have the DRM device open. Let's switch to a DRM-managed initialization to clean up after ourselves only once the DRM device has been last closed. Acked-by:
-
Maxime Ripard authored
drm_connector_unregister() is only to be used for connectors that have been registered through drm_connector_register() after drm_dev_register() has been called. This is our case here so let's remove the call. Acked-by:
-
Maxime Ripard authored
Our internal structure that stores the DRM entities structure is allocated through a device-managed kzalloc. This means that this will eventually be freed whenever the device is removed. In our case, the most likely source of removal is that the main device is going to be unbound, and component_unbind_all() is being run. However, it occurs while the DRM device is still registered, which will create dangling pointers, eventually resulting in use-after-free. Switch to a DRM-managed allocation to keep our structure until the DRM driver doesn't need it anymore. Acked-by:
-
Maxime Ripard authored
The VC4 VEC driver private structure contains only a pointer to the encoder and connector it implements. This makes the overall structure somewhat inconsistent with the rest of the driver, and complicates its initialisation without any apparent gain. Let's embed the drm_encoder structure (through the vc4_encoder one) and drm_connector into struct vc4_vec to fix both issues. Acked-by:
-
Maxime Ripard authored
There's no user for that pointer so let's just get rid of it. Acked-by:
-
Maxime Ripard authored
Our current code now mixes some resources whose lifetime are tied to the device (clocks, IO mappings, etc.) and some that are tied to the DRM device (encoder, bridge). The device one will be freed at unbind time, but the DRM one will only be freed when the last user of the DRM device closes its file handle. So we end up with a time window during which we can call the encoder hooks, but we don't have access to the underlying resources and device. Let's protect all those sections with drm_dev_enter() and drm_dev_exit() so that we bail out if we are during that window. Acked-by:
-
Maxime Ripard authored
drm_connector_unregister() is only to be used for connectors that have been registered through drm_connector_register() after drm_dev_register() has been called. This is our case here so let's remove the call. Acked-by:
-
Maxime Ripard authored
Our internal structure that stores the DRM entities structure is allocated through a device-managed kzalloc. This means that this will eventually be freed whenever the device is removed. In our case, the most likely source of removal is that the main device is going to be unbound, and component_unbind_all() is being run. However, it occurs while the DRM device is still registered, which will create dangling pointers, eventually resulting in use-after-free. Switch to a DRM-managed allocation to keep our structure until the DRM driver doesn't need it anymore. Acked-by:
-
Maxime Ripard authored
There's already a regset in the vc4_crtc structure so there's no need to duplicate it in vc4_txp. Acked-by:
-
Maxime Ripard authored
There's no user for that pointer so let's just get rid of it. Acked-by:
-
Maxime Ripard authored
devm_pm_runtime_enable() simplifies the driver a bit since it will call pm_runtime_disable() automatically through a device-managed action. Acked-by:
-
Maxime Ripard authored
Whenever the device and driver are unbound, the main device and all the subdevices will be removed by calling their unbind() method. However, the DRM device itself will only be freed when the last user will have closed it. It means that there is a time window where the device and its resources aren't there anymore, but the userspace can still call into our driver. Fortunately, the DRM framework provides the drm_dev_enter() and drm_dev_exit() functions to make sure our underlying device is still there for the section protected by those calls. Let's add them to the HDMI driver. Acked-by:
-
Maxime Ripard authored
The HDMI driver unbind hook doesn't have any ALSA-related code anymore, so let's move the ALSA sanity checks and comments we have to some other part of the driver dedicated to ALSA. Acked-by:
-
Maxime Ripard authored
Commit 776efe80 ("drm/vc4: hdmi: Drop devm interrupt handler for hotplug interrupts") dropped the device-managed interrupt registration because it was creating bugs and races whenever an interrupt was coming in while the device was removed. However, our latest patches to the HDMI controller driver fix this as well, so we can use device-managed interrupt handlers again. Acked-by:
-
Maxime Ripard authored
The current code to build the registers set later exposed in debugfs for the HDMI controller relies on traditional allocations, that are later free'd as part of the driver unbind hook. Since krealloc doesn't have a DRM-managed equivalent, let's add an action to free the buffer later on. Acked-by:
-
Maxime Ripard authored
The reference to the DDC controller device needs to be put back when we're done with it. Let's use a device-managed action to simplify the driver. Acked-by:
-
Maxime Ripard authored
The current code to unregister our CEC device needs to be undone manually when we remove the HDMI driver. Since the CEC framework will allocate its main structure, and will defer its deallocation to when the last user will have closed it, we don't really need to take any particular measure to prevent any use-after-free and can thus use any managed action. Acked-by:
-
Maxime Ripard authored
The current code to unregister our ALSA device needs to be undone manually when we remove the HDMI driver. Since ALSA doesn't seem to support any mechanism to defer freeing something until the last user of the ALSA device is gone, we can either use a device-managed or a DRM-managed action. The consistent way would be to use a DRM-managed one, just like pretty much any framework-facing structure should be doing. However, ALSA does a lot of allocation and registration using device-managed calls. Thus, if we're going that way, by the time the DRM-managed action would run all of those allocation would have been freed and we would end up with a use-after-free. Thus, let's do a device-managed action. It's been tested with KASAN enabled and doesn't seem to trigger any issue, so it's as good as anything. Acked-by:
-
Maxime Ripard authored
The current code will call drm_connector_unregister() and drm_connector_cleanup() when the device is unbound. However, by then, there might still be some references held to that connector, including by the userspace that might still have the DRM device open. Let's switch to a DRM-managed initialization to clean up after ourselves only once the DRM device has been last closed. Acked-by:
-
Maxime Ripard authored
The current code will call drm_encoder_cleanup() when the device is unbound. However, by then, there might still be some references held to that encoder, including by the userspace that might still have the DRM device open. Let's switch to a DRM-managed initialization to clean up after ourselves only once the DRM device has been last closed. Acked-by:
-
Maxime Ripard authored
drm_connector_unregister() is only to be used for connectors that have been registered through drm_connector_register() after drm_dev_register() has been called. This is our case here so let's remove the call. Acked-by:
-
Maxime Ripard authored
Our internal structure that stores the DRM entities structure is allocated through a device-managed kzalloc. This means that this will eventually be freed whenever the device is removed. In our case, the most likely source of removal is that the main device is going to be unbound, and component_unbind_all() is being run. However, it occurs while the DRM device is still registered, which will create dangling pointers, eventually resulting in use-after-free. Switch to a DRM-managed allocation to keep our structure until the DRM driver doesn't need it anymore. Acked-by:
-
Maxime Ripard authored
devm_pm_runtime_enable() simplifies the driver a bit since it will call pm_runtime_disable() automatically through a device-managed action. Acked-by:
-
Maxime Ripard authored
The vc4_dsi structure is currently allocated through a device-managed allocation. This can lead to use-after-free issues however in the unbinding path since the DRM entities will stick around, but the underlying structure has been freed. However, we can't just fix it by using a DRM-managed allocation like we did for the other drivers since the DSI case is a bit more intricate. Indeed, the structure will be allocated at probe time, when we don't have a DRM device yet, to be able to register the DSI bus driver. We will then reuse it at bind time to register our KMS entities in the framework. In order to work around both constraints, we can use a kref to track the users of the structure (DSI host, and KMS), and then put our structure when the DSI host will have been unregistered, and through a DRM-managed action that will execute once we won't need the KMS entities anymore. Acked-by:
-
