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

[Richard Cheng]

On Fri, Jun 26, 2026 at 09:17:03AM +0800, Dave Jiang wrote:
> 
> 
> On 6/26/26 12:27 AM, Richard Cheng wrote:
> > 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.
> 
> Hi Richard, I see sashiko pointed out some existing issues on the series. Will you create fixes for them or should I go do that?
> 
> DJ
>

Hi Dave,

I've sent out v3 of this here.

https://lore.kernel.org/all/20260626104102.53892-1-icheng@xxxxxxxxxx/

I've made the fixes of the bug sashiko-bot pointed out here in that serie.

I also see it's mentioning more pre-existing bugs on v3, I'll do them in seperate patches,
or are you refering to the issue in this one ? if you're talking about the latter, I've
included them in v3.

Please take a look while you're available, thanks.

Best regards,
Richard Cheng.

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