| Message ID | 20170323122230.19064-1-colin.king@canonical.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Mar 23, 2017 at 12:22:30PM +0000, Colin King wrote: > From: Colin Ian King <colin.king@canonical.com> > > info is being checked to see if it is a null pointer, however, vpgu is > dereferencing info before this check, leading to a potential null > pointer dereference. If info is null, then the error message being > printed by macro gvt_vgpu_err and this requires vpgu to exist. We can > use a null vpgu as the macro has a sanity check to see if vpgu is null, > so this is OK. It is never NULL, it gets checked by its only caller. -Chris
On Thu, Mar 23, 2017 at 1:22 PM, Colin King <colin.king@canonical.com> wrote: > From: Colin Ian King <colin.king@canonical.com> > > info is being checked to see if it is a null pointer, however, vpgu is > dereferencing info before this check, leading to a potential null > pointer dereference. If info is null, then the error message being > printed by macro gvt_vgpu_err and this requires vpgu to exist. We can > use a null vpgu as the macro has a sanity check to see if vpgu is null, > so this is OK. > > Detected with CoverityScan, CID#1420672 ("Dereference nefore null check") s,nefore,before, > > Fixes: 695fbc08d80f ("drm/i915/gvt: replace the gvt_err with gvt_vgpu_err") > Signed-off-by: Colin Ian King <colin.king@canonical.com> > --- > drivers/gpu/drm/i915/gvt/kvmgt.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c b/drivers/gpu/drm/i915/gvt/kvmgt.c > index 1ea3eb270de8..f8619a772c44 100644 > --- a/drivers/gpu/drm/i915/gvt/kvmgt.c > +++ b/drivers/gpu/drm/i915/gvt/kvmgt.c > @@ -1339,9 +1339,9 @@ static int kvmgt_guest_init(struct mdev_device *mdev) > > static bool kvmgt_guest_exit(struct kvmgt_guest_info *info) > { > - struct intel_vgpu *vgpu = info->vgpu; > - > if (!info) { > + struct intel_vgpu *vgpu = NULL; > + > gvt_vgpu_err("kvmgt_guest_info invalid\n"); > return false; > } Does this mean the original gvt_err() macro is no longer there? And apparently gvt_vgpu_err is a macro that depends on the specifics of its environment? Yikes. Cheers, Frans
Dropping the irrelevant Cc's. On to, 2017-03-23 at 12:39 +0000, Chris Wilson wrote: > On Thu, Mar 23, 2017 at 12:22:30PM +0000, Colin King wrote: > > > > From: Colin Ian King <colin.king@canonical.com> > > > > info is being checked to see if it is a null pointer, however, vpgu is > > dereferencing info before this check, leading to a potential null > > pointer dereference. If info is null, then the error message being > > printed by macro gvt_vgpu_err and this requires vpgu to exist. We can > > use a null vpgu as the macro has a sanity check to see if vpgu is null, > > so this is OK. > > It is never NULL, it gets checked by its only caller. Took me a while to make any sense of the code as gvt_vgpu_err depends on a vgpu variable being declared in the scope without taking it as a parameter and that is a one big no-no: https://01.org/linuxgraphics/gfx-docs/drm/process/coding-style.html#macros-enums-and-rtl Regards, Joonas
On 2017.03.23 16:11:00 +0200, Joonas Lahtinen wrote: > Dropping the irrelevant Cc's. > > On to, 2017-03-23 at 12:39 +0000, Chris Wilson wrote: > > On Thu, Mar 23, 2017 at 12:22:30PM +0000, Colin King wrote: > > > > > > From: Colin Ian King <colin.king@canonical.com> > > > > > > info is being checked to see if it is a null pointer, however, vpgu is > > > dereferencing info before this check, leading to a potential null > > > pointer dereference. If info is null, then the error message being > > > printed by macro gvt_vgpu_err and this requires vpgu to exist. We can > > > use a null vpgu as the macro has a sanity check to see if vpgu is null, > > > so this is OK. > > > > It is never NULL, it gets checked by its only caller. > > Took me a while to make any sense of the code as gvt_vgpu_err depends > on a vgpu variable being declared in the scope without taking it as a > parameter and that is a one big no-no: > > https://01.org/linuxgraphics/gfx-docs/drm/process/coding-style.html#macros-enums-and-rtl > Thanks for comment, Joonas. Current gvt dbg print is still a mess, we will try to clean it up.
On 2017.03.23 14:43:44 +0100, Frans Klaver wrote: > On Thu, Mar 23, 2017 at 1:22 PM, Colin King <colin.king@canonical.com> wrote: > > From: Colin Ian King <colin.king@canonical.com> > > > > info is being checked to see if it is a null pointer, however, vpgu is > > dereferencing info before this check, leading to a potential null > > pointer dereference. If info is null, then the error message being > > printed by macro gvt_vgpu_err and this requires vpgu to exist. We can > > use a null vpgu as the macro has a sanity check to see if vpgu is null, > > so this is OK. > > > > Detected with CoverityScan, CID#1420672 ("Dereference nefore null check") > > s,nefore,before, > > > > > > Fixes: 695fbc08d80f ("drm/i915/gvt: replace the gvt_err with gvt_vgpu_err") > > Signed-off-by: Colin Ian King <colin.king@canonical.com> > > --- > > drivers/gpu/drm/i915/gvt/kvmgt.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c b/drivers/gpu/drm/i915/gvt/kvmgt.c > > index 1ea3eb270de8..f8619a772c44 100644 > > --- a/drivers/gpu/drm/i915/gvt/kvmgt.c > > +++ b/drivers/gpu/drm/i915/gvt/kvmgt.c > > @@ -1339,9 +1339,9 @@ static int kvmgt_guest_init(struct mdev_device *mdev) > > > > static bool kvmgt_guest_exit(struct kvmgt_guest_info *info) > > { > > - struct intel_vgpu *vgpu = info->vgpu; > > - > > if (!info) { > > + struct intel_vgpu *vgpu = NULL; > > + > > gvt_vgpu_err("kvmgt_guest_info invalid\n"); > > return false; > > } > > Does this mean the original gvt_err() macro is no longer there? > > And apparently gvt_vgpu_err is a macro that depends on the specifics > of its environment? Yikes. > The null check is redundant there, we can just remove that block and extra vgpu variable.
diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c b/drivers/gpu/drm/i915/gvt/kvmgt.c index 1ea3eb270de8..f8619a772c44 100644 --- a/drivers/gpu/drm/i915/gvt/kvmgt.c +++ b/drivers/gpu/drm/i915/gvt/kvmgt.c @@ -1339,9 +1339,9 @@ static int kvmgt_guest_init(struct mdev_device *mdev) static bool kvmgt_guest_exit(struct kvmgt_guest_info *info) { - struct intel_vgpu *vgpu = info->vgpu; - if (!info) { + struct intel_vgpu *vgpu = NULL; + gvt_vgpu_err("kvmgt_guest_info invalid\n"); return false; }