Re: [PATCH v2] cxl/features: Reject Get Feature count larger than the output buffer

[Richard Cheng]

On Wed, Jun 24, 2026 at 02:10:26PM +0800, Alison Schofield wrote:
> On Wed, Jun 24, 2026 at 01:54:50PM -0700, Alison Schofield wrote:
> > On Wed, Jun 24, 2026 at 09:47:37PM +0800, Richard Cheng wrote:
> > > cxlctl_get_feature() sizes its output buffer from the user's
> > > fwctl_rpc.out_len, but the device is told to write
> > > cxl_mbox_get_feat_in.count bytes into rpc_out->payload, which is a
> > > separate user-controlled value. Nothing bounds count against out_len, so
> > > a small out_len with a large count overflows the kvzalloc()'d buffer.
> > > A heap OOB write reachable from FWCTL_RPC.
> > > 
> > > Reject requests where count exceeds the available payload room, before
> > > allocating.
> > > 
> > > Fixes: 5908f3ed6dc2 ("cxl: Add support to handle user feature commands for get feature")
> > > Reviewed-by: Kai-Heng Feng <kaihengf@xxxxxxxxxx>
> > > Reviewed-by: Koba Ko <kobak@xxxxxxxxxx>
> > > Reviewed-by: Dave Jiang <dave.jiang@xxxxxxxxx>
> > > Signed-off-by: Richard Cheng <icheng@xxxxxxxxxx>
> > > ---
> > > Changelog:
> > > 
> > > v1 -> v2:
> > >     - Drop the reproducer and trim the KASAN splat in the commit message
> > >     - Sent the reproducer as a regression test in ndctl separately.
> > 
> > This patch itself looks good. Looking at the other bounds checks
> > Sashiko suggests, I'd rather see this all fixed up in one patch or
> > patchset, rather than dribble in as multiple patches.
> > 
> > Maybe it all fits into one patch, like this:
> > cxl/features; Add bounds checking for get/set feature commands
> > or maybe it works better as a set.
> > 
> > Either way, doing in one swoop would be nice!
> 
> Oh, seems I'm reading patches out of order.
> 
> Now I see this: 
> https://lore.kernel.org/linux-cxl/20260620-cxl-fwctl-oob-v1-1-5758e34d784a@xxxxxxxxx/
> which looks like it covers one of Sashikos's complaints.
>

Hi Alison,

Thanks for pointing this out.

Then that would be no problem for me, I'll append my fixes with Zhenhao's patch.

so my original fix + sashiko's complain about cxlctl_set_feature() and cxl_get_feature().
I'll put them in my work and send as a serie.

--Richard
 
> > 
> > -- Alison
> > 
> > > 
> > > Best regards,
> > > Richard Cheng
> > > ---
> > >  drivers/cxl/core/features.c | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > > 
> > > diff --git a/drivers/cxl/core/features.c b/drivers/cxl/core/features.c
> > > index 85185af46b72..9c714ee42a41 100644
> > > --- a/drivers/cxl/core/features.c
> > > +++ b/drivers/cxl/core/features.c
> > > @@ -471,6 +471,10 @@ static void *cxlctl_get_feature(struct cxl_features_state *cxlfs,
> > >  	if (!count)
> > >  		return ERR_PTR(-EINVAL);
> > >  
> > > +	if (out_size < offsetof(struct fwctl_rpc_cxl_out, payload) ||
> > > +	    count > out_size - offsetof(struct fwctl_rpc_cxl_out, payload))
> > > +		return ERR_PTR(-EINVAL);
> > > +
> > >  	struct fwctl_rpc_cxl_out *rpc_out __free(kvfree) =
> > >  		kvzalloc(out_size, GFP_KERNEL);
> > >  	if (!rpc_out)
> > > 
> > > base-commit: ef0c9f75a19532d7675384708fc8621e10850104
> > > -- 
> > > 2.43.0
> > > 
> >