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

[Alison Schofield <alison.schofield@intel.com>]

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@nvidia.com>
> > Reviewed-by: Koba Ko <kobak@nvidia.com>
> > Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> > Signed-off-by: Richard Cheng <icheng@nvidia.com>
> > ---
> > 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@gmail.com/
which looks like it covers one of Sashikos's complaints.

> 
> -- 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
> > 
>