mbox series

[RFC,0/9] ALSA: Don't embed struct devices

Message ID 20230816160252.23396-1-tiwai@suse.de (mailing list archive)
Headers show
Series ALSA: Don't embed struct devices | expand

Message

Takashi Iwai Aug. 16, 2023, 4:02 p.m. UTC
Hi,

this is another set of patches to attempt papering over the UAF
problems that are seen when the delayed kobject release is enabled, as
initially reported by Curtis:
  https://lore.kernel.org/r/20230801171928.1460120-1-cujomalainey@chromium.org

There was a previous patch set with a different approach (using the
device refcount dependencies), but this is a sort of step-back to the
old way.
  https://lore.kernel.org/r/20230807135207.17708-1-tiwai@suse.de

After discussions and evaluations, we agreed that decoupling the
struct device from each sound component object is the safest (and
easiest) way as of now.  For applying the changes more consistently, I
introduced a new helper for the struct device allocation and
initialization, and applied all components.

A couple of more changes for card_dev refcount managed aren't included
in this patch set, though.  They might be good to have, but this patch
set should suffice for the currently seen UAF problems.

For a long-term solution, we may restructure the device management,
then the struct devices may be embedded again in each object.  But,
it'll need lots of other changes and cleanups, a big TODO.

The latest patches are found in topic/dev-split branch of sound.git
tree.


Takashi

===

Takashi Iwai (9):
  ALSA: core: Introduce snd_device_alloc()
  ALSA: control: Don't embed ctl_dev
  ALSA: pcm: Don't embed device
  ALSA: hwdep: Don't embed device
  ALSA: rawmidi: Don't embed device
  ALSA: compress: Don't embed device
  ALSA: timer: Create device with snd_device_alloc()
  ALSA: seq: Create device with snd_device_alloc()
  ALSA: core: Drop snd_device_initialize()

 include/sound/compress_driver.h |  2 +-
 include/sound/core.h            |  4 ++--
 include/sound/hwdep.h           |  2 +-
 include/sound/pcm.h             |  2 +-
 include/sound/rawmidi.h         |  2 +-
 sound/aoa/soundbus/i2sbus/pcm.c |  4 ++--
 sound/core/compress_offload.c   | 16 ++++++++------
 sound/core/control.c            | 14 ++++++------
 sound/core/control_led.c        |  4 ++--
 sound/core/hwdep.c              | 38 ++++++++++++++++++---------------
 sound/core/init.c               | 28 +++++++++++++++---------
 sound/core/pcm.c                | 22 +++++++++++--------
 sound/core/rawmidi.c            | 29 +++++++++++--------------
 sound/core/seq/seq_clientmgr.c  | 16 ++++++++------
 sound/core/timer.c              | 16 ++++++++------
 sound/core/ump.c                |  8 +++----
 sound/pci/hda/hda_hwdep.c       |  4 ++--
 sound/usb/media.c               |  4 ++--
 18 files changed, 119 insertions(+), 96 deletions(-)

Comments

Jaroslav Kysela Aug. 16, 2023, 4:45 p.m. UTC | #1
On 16. 08. 23 18:02, Takashi Iwai wrote:
> Hi,
> 
> this is another set of patches to attempt papering over the UAF
> problems that are seen when the delayed kobject release is enabled, as
> initially reported by Curtis:
>    https://lore.kernel.org/r/20230801171928.1460120-1-cujomalainey@chromium.org
> 
> There was a previous patch set with a different approach (using the
> device refcount dependencies), but this is a sort of step-back to the
> old way.
>    https://lore.kernel.org/r/20230807135207.17708-1-tiwai@suse.de
> 
> After discussions and evaluations, we agreed that decoupling the
> struct device from each sound component object is the safest (and
> easiest) way as of now.  For applying the changes more consistently, I
> introduced a new helper for the struct device allocation and
> initialization, and applied all components.
> 
> A couple of more changes for card_dev refcount managed aren't included
> in this patch set, though.  They might be good to have, but this patch
> set should suffice for the currently seen UAF problems.
> 
> For a long-term solution, we may restructure the device management,
> then the struct devices may be embedded again in each object.  But,
> it'll need lots of other changes and cleanups, a big TODO.
> 
> The latest patches are found in topic/dev-split branch of sound.git
> tree.
> 
> 
> Takashi
> 
> ===
> 
> Takashi Iwai (9):
>    ALSA: core: Introduce snd_device_alloc()
>    ALSA: control: Don't embed ctl_dev
>    ALSA: pcm: Don't embed device
>    ALSA: hwdep: Don't embed device
>    ALSA: rawmidi: Don't embed device
>    ALSA: compress: Don't embed device
>    ALSA: timer: Create device with snd_device_alloc()
>    ALSA: seq: Create device with snd_device_alloc()
>    ALSA: core: Drop snd_device_initialize()

For all commits:

Reviewed-by: Jaroslav Kysela <perex@perex.cz>
Curtis Malainey Aug. 16, 2023, 10:18 p.m. UTC | #2
On Wed, Aug 16, 2023 at 9:45 AM Jaroslav Kysela <perex@perex.cz> wrote:
>
> On 16. 08. 23 18:02, Takashi Iwai wrote:
> > Hi,
> >
> > this is another set of patches to attempt papering over the UAF
> > problems that are seen when the delayed kobject release is enabled, as
> > initially reported by Curtis:
> >    https://lore.kernel.org/r/20230801171928.1460120-1-cujomalainey@chromium.org
> >
> > There was a previous patch set with a different approach (using the
> > device refcount dependencies), but this is a sort of step-back to the
> > old way.
> >    https://lore.kernel.org/r/20230807135207.17708-1-tiwai@suse.de
> >
> > After discussions and evaluations, we agreed that decoupling the
> > struct device from each sound component object is the safest (and
> > easiest) way as of now.  For applying the changes more consistently, I
> > introduced a new helper for the struct device allocation and
> > initialization, and applied all components.
> >
> > A couple of more changes for card_dev refcount managed aren't included
> > in this patch set, though.  They might be good to have, but this patch
> > set should suffice for the currently seen UAF problems.
> >
> > For a long-term solution, we may restructure the device management,
> > then the struct devices may be embedded again in each object.  But,
> > it'll need lots of other changes and cleanups, a big TODO.

I agree I think we should apply this as proper fixes will be a big
lift. Thanks for refining them.

> >
> > The latest patches are found in topic/dev-split branch of sound.git
> > tree.
> >
> >
> > Takashi
> >
> > ===
> >
> > Takashi Iwai (9):
> >    ALSA: core: Introduce snd_device_alloc()
> >    ALSA: control: Don't embed ctl_dev
> >    ALSA: pcm: Don't embed device
> >    ALSA: hwdep: Don't embed device
> >    ALSA: rawmidi: Don't embed device
> >    ALSA: compress: Don't embed device
> >    ALSA: timer: Create device with snd_device_alloc()
> >    ALSA: seq: Create device with snd_device_alloc()
> >    ALSA: core: Drop snd_device_initialize()
>
> For all commits:
>
> Reviewed-by: Jaroslav Kysela <perex@perex.cz>
>
> --
> Jaroslav Kysela <perex@perex.cz>
> Linux Sound Maintainer; ALSA Project; Red Hat, Inc.
>

For all

Signed-off-by: Curtis Malainey <cujomalainey@chromium.org>
Tested-by: Curtis Malainey <cujomalainey@chromium.org>
Takashi Iwai Aug. 17, 2023, 7:27 a.m. UTC | #3
On Wed, 16 Aug 2023 18:02:43 +0200,
Takashi Iwai wrote:
> 
> Hi,
> 
> this is another set of patches to attempt papering over the UAF
> problems that are seen when the delayed kobject release is enabled, as
> initially reported by Curtis:
>   https://lore.kernel.org/r/20230801171928.1460120-1-cujomalainey@chromium.org
> 
> There was a previous patch set with a different approach (using the
> device refcount dependencies), but this is a sort of step-back to the
> old way.
>   https://lore.kernel.org/r/20230807135207.17708-1-tiwai@suse.de
> 
> After discussions and evaluations, we agreed that decoupling the
> struct device from each sound component object is the safest (and
> easiest) way as of now.  For applying the changes more consistently, I
> introduced a new helper for the struct device allocation and
> initialization, and applied all components.
> 
> A couple of more changes for card_dev refcount managed aren't included
> in this patch set, though.  They might be good to have, but this patch
> set should suffice for the currently seen UAF problems.
> 
> For a long-term solution, we may restructure the device management,
> then the struct devices may be embedded again in each object.  But,
> it'll need lots of other changes and cleanups, a big TODO.
> 
> The latest patches are found in topic/dev-split branch of sound.git
> tree.
> 
> 
> Takashi
> 
> ===
> 
> Takashi Iwai (9):
>   ALSA: core: Introduce snd_device_alloc()
>   ALSA: control: Don't embed ctl_dev
>   ALSA: pcm: Don't embed device
>   ALSA: hwdep: Don't embed device
>   ALSA: rawmidi: Don't embed device
>   ALSA: compress: Don't embed device
>   ALSA: timer: Create device with snd_device_alloc()
>   ALSA: seq: Create device with snd_device_alloc()
>   ALSA: core: Drop snd_device_initialize()

Although the patch set was sent as RFC, I merged them now for 6.6 with
Acks, as there is no further plan to change.


thanks,

Takashi