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

[Richard Cheng]

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@xxxxxxxxxx>
> > ---
> >  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 ?

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