Re: [PATCH v3 8/9] i2c: Support dynamic address translation

Romain Gantois
2024-12-09T12:42:47Z

Hi Tomi,

On vendredi 29 novembre 2024 10:54:35 heure normale d’Europe centrale Tomi
Valkeinen wrote:
> Hi Romain,
>
> On 25/11/2024 10:45, Romain Gantois wrote:
> > The i2c-atr module keeps a list of associations between I2C client aliases
...
> > i2c_atr_dynamic_attach/detach_addr from racing with the bus notifier
> > handler to modify alias_list.
> >
> > Signed-off-by: Romain Gantois <romain.gantois@bootlin.com>
> > ---
> >
> > drivers/i2c/i2c-atr.c | 244
> > ++++++++++++++++++++++++++++++++----------
> > drivers/media/i2c/ds90ub960.c | 2 +-
> > include/linux/i2c-atr.h | 13 ++-
> > 3 files changed, 202 insertions(+), 57 deletions(-)
>
> This fails with:
>
> WARNING: CPU: 1 PID: 360 at lib/list_debug.c:35
> __list_add_valid_or_report+0xe4/0x100
>
> as the i2c_atr_create_c2a() calls list_add(), but i2c_atr_attach_addr(),
> which is changed to use i2c_atr_create_c2a(), also calls list_add().
>
> Also, if you add i2c_atr_create_c2a() which hides the allocation and
> list_add, I think it makes sense to add a i2c_atr_destroy_c2a() to
> revert that.
>
> There's also a memory error "BUG: KASAN: slab-use-after-free in
> __lock_acquire+0xc4/0x375c" (see below) when unloading the ub960 or
> ub953 driver. I haven't looked at that yet.

I think I've found what's causing this KASAN splat. i2c_atr_del_adapter is
freeing it's alias pool before setting atr->adapter[chan_id] to NULL. So
there's a time window during which bus notifications can trigger a call
to i2c_atr_attach_addr, leading to a UAF on the alias pool struct.

I'll fix this in v4.

Thanks,

--
Romain Gantois, Bootlin
Embedded Linux and Kernel engineering