Re: [PATCH 1/3] zram: fix zstd dict use-after-free on per-CPU error path

[Sergey Senozhatsky]

On (26/07/28 00:06), haoqin huang wrote:
> > > zstd_setup_params() creates global cdict and ddict stored in
> > > params->drv_data, shared across all per-CPU contexts. When a
> > > per-CPU zstd_create() failed, its error path called
> > > zstd_release_params() which freed those shared objects while
> > > other per-CPU contexts might already hold references to them.
> >
> > zstd_release_params() sets ->drv_data to NULL so we can free params
> > only once.  In addition, "while other per-CPU contexts might
> > already hold references to them" -- other CPUs cannot do anything
> > with those params, the device is not setup and we cannot handle any
> > IO requests, right?  There is no double-free nor UAF there as far
> > as I can tell.
> >
> 
> You're right.  On the init failure path, drv_data=NULL prevents a
> double-free, no IO is ever submitted, and cpuhp_rollback_install()
> properly tears down the per-CPU contexts that succeeded (which only
> calls zstd_destroy(), never touching drv_data). So the system doesn't
> crash or leak -- it just means the global cdict/ddict get released by
> the per-CPU error path rather than by zcomp_init()'s cleanup label.
> The original commit message was poorly worded, I'll fix it.
> 
> > > Remove the premature zstd_release_params() from the per-CPU
> > > error path, the global cdict/ddict are properly released later
> > > by zstd_release_params(), called from zcomp_init()'s cleanup
> > > or from zcomp_destroy().
> > >
> > > Fixes: 6a559ecd6e7e ("zram: add dictionary support to zstd backend")
> >
> > What exactly does this fix?
> 
> It's more of a correctness / ownership fix than a crash fix.

OK, fair. Agreed, let's rewrite commit message and then we are good
to go.