Re: [Intel-gfx][PATCH v2] drm/i915/gvt: Prevent NULL pointer dereference in intel_vgpu_reg_rw_edid()

[Aho Sior <aho@sior.be>]

Perfect!
I can confirm it resolves the issue as well, thank you very much.

Have a great day,
Alejandro Sior.


On Fri, 11 Sep 2020 at 07:58, Zhenyu Wang <zhenyuw@linux.intel.com> wrote:
>
> On 2020.09.08 20:11:21 +0200, Alejandro Sior wrote:
> > In the function intel_vgpu_reg_rw_edid of kvmgt.c, pos can be equal
> > to NULL for GPUs that do not properly support EDID. In those cases, when
> > pos gets passed to the handle_edid functions, it gets added a short offset
> > then it's dereferenced in memcpy's, leading to NULL pointer
> > dereference kernel oops.
> >
> > More concretely, that kernel oops renders some Broadwell GPUs users
> > unable to set up virtual machines with virtual GPU passthrough (virtual
> > machines hang indefinitely when trying to make use of the virtual GPU),
> > and make them unable to remove the virtual GPUs once the kernel oops has
> > happened (it hangs indefinitely, and notably too when the kernel tries to
> > shutdown). The issues that this causes and steps to reproduce are
> > discussed in more details in this github issue post:
> > https://github.com/intel/gvt-linux/issues/170#issuecomment-685806160
> >
> > Check if pos is equal to NULL, and if it is, set ret to a negative
> > value, making the module simply indicate that the access to EDID region
> > has failed, without any fatal repercussion.
> >
> > Signed-off-by: Alejandro Sior <aho@sior.be>
> >
> > ---
> > Changes in v2:
> > - removed middle name of author to comply with git name
> > - rephrased the patch description with imperative phrasing
> > - removed useless paragraph
> > - made a paragraph more concise
> > - fixed typos
> > - made individual lines shorter than 75 chars
> >
> >  drivers/gpu/drm/i915/gvt/kvmgt.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c b/drivers/gpu/drm/i915/gvt/kvmgt.c
> > index ad8a9df49f29..49163363ba4a 100644
> > --- a/drivers/gpu/drm/i915/gvt/kvmgt.c
> > +++ b/drivers/gpu/drm/i915/gvt/kvmgt.c
> > @@ -557,7 +557,9 @@ static size_t intel_vgpu_reg_rw_edid(struct intel_vgpu *vgpu, char *buf,
> >               (struct vfio_edid_region *)kvmgt_vdev(vgpu)->region[i].data;
> >       loff_t pos = *ppos & VFIO_PCI_OFFSET_MASK;
> >
> > -     if (pos < region->vfio_edid_regs.edid_offset) {
> > +     if (pos == NULL) {
> > +             ret = -EINVAL;
> > +     } else if (pos < region->vfio_edid_regs.edid_offset) {
> >               ret = handle_edid_regs(vgpu, region, buf, count, pos, iswrite);
> >       } else {
> >               pos -= EDID_BLOB_OFFSET;
>
> Thanks for reporting this! Sorry that we failed to do sanity validation on older
> platform when enabling vfio edid function for all platform.
>
> Could you try below one to see if it fixes your problem? Instead of refusing
> possible read of edid, this fixes port number for BDW.
>
> From d5d9304b6bfdc31356fd2feb1ddbbf28073fe3d4 Mon Sep 17 00:00:00 2001
> From: Zhenyu Wang <zhenyuw@linux.intel.com>
> Date: Fri, 11 Sep 2020 13:50:20 +0800
> Subject: [PATCH] drm/i915/gvt: Fix port number for BDW on EDID region setup
>
> Current BDW virtual display port is initialized as PORT_B, so need
> to use same port for VFIO EDID region, otherwise invalid EDID blob
> pointer is assigned.
>
> Fixes: 0178f4ce3c3b ("drm/i915/gvt: Enable vfio edid for all GVT supported platform")
> Signed-off-by: Zhenyu Wang <zhenyuw@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/gvt/vgpu.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/gvt/vgpu.c b/drivers/gpu/drm/i915/gvt/vgpu.c
> index 8fa9b31a2484..f6d7e33c7099 100644
> --- a/drivers/gpu/drm/i915/gvt/vgpu.c
> +++ b/drivers/gpu/drm/i915/gvt/vgpu.c
> @@ -368,6 +368,7 @@ void intel_gvt_destroy_idle_vgpu(struct intel_vgpu *vgpu)
>  static struct intel_vgpu *__intel_gvt_create_vgpu(struct intel_gvt *gvt,
>                 struct intel_vgpu_creation_params *param)
>  {
> +       struct drm_i915_private *dev_priv = gvt->gt->i915;
>         struct intel_vgpu *vgpu;
>         int ret;
>
> @@ -436,7 +437,10 @@ static struct intel_vgpu *__intel_gvt_create_vgpu(struct intel_gvt *gvt,
>         if (ret)
>                 goto out_clean_sched_policy;
>
> -       ret = intel_gvt_hypervisor_set_edid(vgpu, PORT_D);
> +       if (IS_BROADWELL(dev_priv))
> +               ret = intel_gvt_hypervisor_set_edid(vgpu, PORT_B);
> +       else
> +               ret = intel_gvt_hypervisor_set_edid(vgpu, PORT_D);
>         if (ret)
>                 goto out_clean_sched_policy;
>
> --
> 2.28.0
>
>
>
> --
>
> $gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827