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

[Romain Gantois]

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@xxxxxxxxxxx>
> > ---
> > 
> >   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
https://bootlin.com