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

[Dave Jiang <dave.jiang@intel.com>]

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

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