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

[Romain Gantois]

Hi Tomi,

On lundi 9 décembre 2024 13:42:29 heure normale d’Europe centrale Romain 
Gantois wrote:
> Hi Tomi,
> 
...
> > 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.

It seems like my initial theory was wrong. The bus notifier wouldn't trigger 
after the adapter has been removed.

However, the "alias_pool->shared" flag is not set anywhere in the i2c-atr 
module! So a more likely theory is that the common alias pool is being
freed when the first channel is deleted. Thus, the second channel is
still referencing a freed alias pool during it's deletion, hence the UAF.

Properly setting the "shared" flag of alias pools owned by the i2c_atr struct
should fix this.

Thanks,

-- 
Romain Gantois, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com