Re: [PATCH 1/3] cxl/features: Reject feature offset that overflows 16-bit field

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

On 7/2/26 1:15 AM, Richard Cheng wrote:
> On Tue, Jun 30, 2026 at 08:54:29AM +0800, Dave Jiang wrote:
>>
>>
>> On 6/30/26 12:46 AM, Richard Cheng wrote:
>>> cxl_get_feature() and cxl_set_feature() build the mailbox command's
>>> offset as cpu_to_le16(offset + data_rcvd_size/data_sent_size), but never
>>> check the sum fits in the 16-bit field. Via fwctl, a user-supplied
>>> offset plus count/op_size summing over 65535 silently wraps, steering
>>> the device to the wrong feature offset.
>>>
>>> Fixes: 5e5ac21f629d ("cxl/mbox: Add GET_FEATURE mailbox command")
>>> Fixes: 14d502cc2718 ("cxl/mbox: Add SET_FEATURE mailbox command")
>>> Signed-off-by: Richard Cheng <icheng@nvidia.com>
>>> ---
>>>  drivers/cxl/core/features.c | 6 ++++++
>>>  1 file changed, 6 insertions(+)
>>>
>>> diff --git a/drivers/cxl/core/features.c b/drivers/cxl/core/features.c
>>> index 85185af46b72..db5964ea184f 100644
>>> --- a/drivers/cxl/core/features.c
>>> +++ b/drivers/cxl/core/features.c
>>> @@ -237,6 +237,9 @@ size_t cxl_get_feature(struct cxl_mailbox *cxl_mbox, const uuid_t *feat_uuid,
>>>  	if (!feat_out || !feat_out_size)
>>>  		return 0;
>>>  
>>> +	if (offset + feat_out_size > U16_MAX)
>>> +		return 0;
>>
>> Should this return -EINVAL?
>>
> 
> I don't think so, the function's signature is size_t cxl_get_feature() , it returns the number
> of bytes received from the device, not an errno.
> If you really want this we need to change the signature.
> 
> But I agree that we shouldn't silently fail here, maybe move the check into cxlctl_get_feature()
> and return ERR_PTR(-EINVAL) ? would that work for you ?

We definitely do not want to silently fail. Given that the function has more than one checks that can fail, not sure if pulling this up is the right way to go. Also it gets called numerous times in core/edac.c. We can either change the return to ssize_t or return SIZE_MAX? Will need to fix up all the calling sites in cxl_edac code though. 

DJ

> 
> Btw, for sashiko-bot's review, I think it stands, I'll send v2 with the fix.
> 
> --Richard
>  
>>> +
>>>  	size_out = min(feat_out_size, cxl_mbox->payload_size);
>>>  	uuid_copy(&pi.uuid, feat_uuid);
>>>  	pi.selection = selection;
>>> @@ -287,6 +290,9 @@ int cxl_set_feature(struct cxl_mailbox *cxl_mbox,
>>>  	if (return_code)
>>>  		*return_code = CXL_MBOX_CMD_RC_INPUT;
>>>  
>>> +	if (offset + feat_data_size > U16_MAX)
>>> +		return -EINVAL;
>>> +
>>>  	struct cxl_mbox_set_feat_in *pi __free(kfree) =
>>>  			kzalloc(cxl_mbox->payload_size, GFP_KERNEL);
>>>  	if (!pi)
>>