Re: [PATCH 3/3] drm/xe/oa: Limit num_syncs to prevent oversized allocations

[Matthew Brost <matthew.brost@intel.com>]

On Fri, Dec 05, 2025 at 12:36:01PM -0800, Dixit, Ashutosh wrote:
> On Fri, 05 Dec 2025 11:05:10 -0800, Shuicheng Lin wrote:
> >
> 
> Hi Shuicheng,
> 
> > The OA open parameters did not validate num_syncs, allowing
> > userspace to pass arbitrarily large values, potentially
> > leading to excessive allocations.
> >
> > Add checks to ensure that num_syncs does not exceed XE_MAX_SYNCS,
> > returning -EINVAL when the limit is violated.
> >
> > Fixes: c8507a25cebd ("drm/xe/oa/uapi: Define and parse OA sync properties")
> > Cc: Matthew Brost <matthew.brost@intel.com>
> > Cc: Ashutosh Dixit <ashutosh.dixit@intel.com>
> > Signed-off-by: Shuicheng Lin <shuicheng.lin@intel.com>
> > ---
> >  drivers/gpu/drm/xe/xe_oa.c | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/xe/xe_oa.c b/drivers/gpu/drm/xe/xe_oa.c
> > index cc48663c2b48..7477c27a4ba2 100644
> > --- a/drivers/gpu/drm/xe/xe_oa.c
> > +++ b/drivers/gpu/drm/xe/xe_oa.c
> > @@ -1254,6 +1254,11 @@ static int xe_oa_set_no_preempt(struct xe_oa *oa, u64 value,
> >  static int xe_oa_set_prop_num_syncs(struct xe_oa *oa, u64 value,
> >				    struct xe_oa_open_param *param)
> >  {
> > +	if (value > XE_MAX_SYNCS) {
> > +		drm_dbg(&oa->xe->drm, "num_syncs %llu must be <= %u\n",
> > +			value, XE_MAX_SYNCS);
> > +		return -EINVAL;
> > +	}
> 
> Like the other patches, let's just do:
> 
> 	if (XE_IOCTL_DBG(xe, value > XE_MAX_SYNCS))
> 		return -EINVAL;
> 
> I might change the other functions here to also use XE_IOCTL_DBG(). They
> may have been written before XE_IOCTL_DBG() was implemented.
> 
> >	param->num_syncs = value;
> >	return 0;
> >  }
> > @@ -1404,6 +1409,12 @@ static int xe_oa_parse_syncs(struct xe_oa *oa,
> >	}
> >
> >	if (param->num_syncs) {
> > +		if (param->num_syncs > XE_MAX_SYNCS) {
> > +			drm_dbg(&oa->xe->drm, "num_syncs %d must be <= %u\n",
> > +				param->num_syncs, XE_MAX_SYNCS);
> > +			ret = -EINVAL;
> > +			goto exit;
> > +		}
> 
> This is not needed. If we return -EINVAL from the code at the top, this
> will never get executed.
> 
> Also, maybe squash the 3 patches into a single patch? Not sure if we need 3
> patches, a single patch with XE_IOCTL_DBG() in all 3 places might make more
> sense?
> 

When did OA land? Before or after force probe removal? If after then
best at least keep the OA part in a different patch so backport patches
(hopefully) just apply.

Matt 

> >		param->syncs = kcalloc(param->num_syncs, sizeof(*param->syncs), GFP_KERNEL);
> >		if (!param->syncs) {
> >			ret = -ENOMEM;
> > --
> > 2.50.1
> >
> 
> Thanks.
> --
> Ashutosh