[PATCH v2 0/2] i2c: fortify the subsystem against user-space induced deadlocks

Bartosz Golaszewski posted 2 patches 2 years, 8 months ago
drivers/i2c/i2c-core-base.c |  26 ++-------
drivers/i2c/i2c-dev.c       | 112 +++++++++++++++++++++++++++++-------
include/linux/i2c.h         |   2 -
3 files changed, 96 insertions(+), 44 deletions(-)
[PATCH v2 0/2] i2c: fortify the subsystem against user-space induced deadlocks
Posted by Bartosz Golaszewski 2 years, 8 months ago
From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Several subsystems in the kernel that export device files to user-space
suffer from a bug where keeping an open file descriptor associated with
this device file, unbinding the device from its driver and then calling
any of the supported system calls on that file descriptor will result in
either a crash or - as is the case with i2c - a deadlock.

This behavior has been blamed on extensive usage of device resource
management interfaces but it seems that devres has nothing to do with it,
the problem would be the same whether using devres or freeing resources
in .remove() that should survive the driver detach.

Many subsystems already deal with this by implementing some kind of flags
in the character device data together with locking preventing the
user-space from dropping the subsystem data from under the open device.

In i2c the deadlock comes from the fact that the function unregistering
the adapter waits for a completion which will not be passed until all
references to the character device are dropped.

The first patch in this series is just a tweak of return values of the
notifier callback. The second addresses the deadlock problem in a way
similar to how we fixed this issue in the GPIO subystem. Details are in
the commit message.

v1 -> v2:
- keep the device release callback and use it to free the IDR number
- rebase on top of v6.2-rc1

Bartosz Golaszewski (2):
  i2c: dev: fix notifier return values
  i2c: dev: don't allow user-space to deadlock the kernel

 drivers/i2c/i2c-core-base.c |  26 ++-------
 drivers/i2c/i2c-dev.c       | 112 +++++++++++++++++++++++++++++-------
 include/linux/i2c.h         |   2 -
 3 files changed, 96 insertions(+), 44 deletions(-)

-- 
2.37.2
Re: [PATCH v2 0/2] i2c: fortify the subsystem against user-space induced deadlocks
Posted by Bartosz Golaszewski 2 years, 8 months ago
On Thu, Dec 29, 2022 at 5:00 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> Several subsystems in the kernel that export device files to user-space
> suffer from a bug where keeping an open file descriptor associated with
> this device file, unbinding the device from its driver and then calling
> any of the supported system calls on that file descriptor will result in
> either a crash or - as is the case with i2c - a deadlock.
>
> This behavior has been blamed on extensive usage of device resource
> management interfaces but it seems that devres has nothing to do with it,
> the problem would be the same whether using devres or freeing resources
> in .remove() that should survive the driver detach.
>
> Many subsystems already deal with this by implementing some kind of flags
> in the character device data together with locking preventing the
> user-space from dropping the subsystem data from under the open device.
>
> In i2c the deadlock comes from the fact that the function unregistering
> the adapter waits for a completion which will not be passed until all
> references to the character device are dropped.
>
> The first patch in this series is just a tweak of return values of the
> notifier callback. The second addresses the deadlock problem in a way
> similar to how we fixed this issue in the GPIO subystem. Details are in
> the commit message.
>
> v1 -> v2:
> - keep the device release callback and use it to free the IDR number
> - rebase on top of v6.2-rc1
>
> Bartosz Golaszewski (2):
>   i2c: dev: fix notifier return values
>   i2c: dev: don't allow user-space to deadlock the kernel
>
>  drivers/i2c/i2c-core-base.c |  26 ++-------
>  drivers/i2c/i2c-dev.c       | 112 +++++++++++++++++++++++++++++-------
>  include/linux/i2c.h         |   2 -
>  3 files changed, 96 insertions(+), 44 deletions(-)
>
> --
> 2.37.2
>

Hi Wolfram,

It's been two weeks without any comments on this series and over a
month since v1 so let me send a gentle ping on this.

Bart
Re: [PATCH v2 0/2] i2c: fortify the subsystem against user-space induced deadlocks
Posted by Wolfram Sang 2 years, 8 months ago
Hi Bart,

> It's been two weeks without any comments on this series and over a
> month since v1 so let me send a gentle ping on this.

This series has a priority for the next merge window. I love it!
However, patch 2 is difficult to review because I need to dive in deeply
into the topic. Hopes are that I can do this next week.

Thanks and happy hacking,

   Wolfram