Re: [Intel-xe] [PATCH 2/2] drm/xe: properly check bounds for xe_wait_user_fence_ioctl()

["Zanoni, Paulo R" <paulo.r.zanoni@intel.com>]

On Tue, 2023-06-27 at 16:30 -0300, Lucas De Marchi wrote:
> On Mon, Jun 26, 2023 at 02:22:21PM -0700, Paulo Zanoni wrote:
> > If !no_engines, then we use copy_from_user to copy to the 'eci' array,
> > which has XE_HW_ENGINE_MAX_INSTANCE members. The amount of members
> > copied is given by the user in args->num_engines, so add code to check
> > that args->num_engines does not exceed XE_HW_ENGINE_MAX_INSTANCE. It's
> > an unsigned value so there's no need to check for negative values.
> > 
> > Fixes error messages such as:
> > 
> >    Buffer overflow detected (54 < 18446744073709551520)!
> > 
> > Very simple reproducer:
> > 
> >    https://people.freedesktop.org/~pzanoni/wait-user-fence-bug/
> 
> same comment as in the previous patch. It'd be better to have a code
> snippet in the commit message.
> 
> > 
> > Reviewed-by: José Roberto de Souza <jose.souza@intel.com>
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > ---
> > drivers/gpu/drm/xe/xe_wait_user_fence.c | 3 +++
> > 1 file changed, 3 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_wait_user_fence.c b/drivers/gpu/drm/xe/xe_wait_user_fence.c
> > index 3122374341d6..098e2a4cff3f 100644
> > --- a/drivers/gpu/drm/xe/xe_wait_user_fence.c
> > +++ b/drivers/gpu/drm/xe/xe_wait_user_fence.c
> > @@ -121,6 +121,9 @@ int xe_wait_user_fence_ioctl(struct drm_device *dev, void *data,
> > 			 addr & 0x7))
> > 		return -EINVAL;
> > 
> > +	if (XE_IOCTL_ERR(xe, args->num_engines > XE_HW_ENGINE_MAX_INSTANCE))
> 
> XE_IOCTL_ERR() is soon going away, but we don't need to wait for it.
> 
> Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com>
> 
> For these 2 commits, let me know if you are sending a new version or if
> I should just add the code snippet while applying.

Feel free to amend the messages while applying, whatever approach you
choose is fine. Thanks!

> 
> thanks
> Lucas De Marchi
> 
> > +		return -EINVAL;
> > +
> > 	if (!no_engines) {
> > 		err = copy_from_user(eci, user_eci,
> > 				     sizeof(struct drm_xe_engine_class_instance) *
> > -- 
> > 2.39.2
> >