drm/i915/gvt: Fix the validation on size field of dp aux header

Submitted by changbin.du@intel.com on April 10, 2018, 5:02 a.m.

Details

Message ID 1523336550-6081-1-git-send-email-changbin.du@intel.com
State New
Headers show
Series "drm/i915/gvt: Fix the validation on size field of dp aux header" ( rev: 1 ) in Intel GVT devel

Not browsing as part of any series.

Commit Message

changbin.du@intel.com April 10, 2018, 5:02 a.m.
From: Changbin Du <changbin.du@intel.com>

The assertion for len is wrong, so fix it. And for where to validate
user input, we should not warn by call trace.

[ 290.584739] WARNING: CPU: 0 PID: 1471 at drivers/gpu/drm/i915/gvt/handlers.c:969 dp_aux_ch_ctl_mmio_write+0x394/0x430 [i915]
[ 290.586113] task: ffff880111fe8000 task.stack: ffffc90044a9c000
[ 290.586192] RIP: e030:dp_aux_ch_ctl_mmio_write+0x394/0x430 [i915]
[ 290.586258] RSP: e02b:ffffc90044a9fd88 EFLAGS: 00010282
[ 290.586315] RAX: 0000000000000017 RBX: 0000000000000003 RCX: ffffffff82461148
[ 290.586391] RDX: 0000000000000000 RSI: 0000000000000001 RDI: 0000000000000201
[ 290.586468] RBP: ffffc90043ed1000 R08: 0000000000000248 R09: 00000000000003d8
[ 290.586544] R10: ffffc90044bdd314 R11: 0000000000000011 R12: 0000000000064310
[ 290.586621] R13: 00000000fe4003ff R14: ffffc900432d1008 R15: ffff88010fa7cb40
[ 290.586701] FS: 0000000000000000(0000) GS:ffff880123200000(0000) knlGS:0000000000000000
[ 290.586787] CS: e033 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 290.586849] CR2: 00007f67ea44e000 CR3: 0000000116078000 CR4: 0000000000042660
[ 290.586926] Call Trace:
[ 290.586958] ? __switch_to_asm+0x40/0x70
[ 290.587017] intel_vgpu_mmio_reg_rw+0x1ec/0x3c0 [i915]
[ 290.587087] intel_vgpu_emulate_mmio_write+0xa8/0x2c0 [i915]
[ 290.587151] xengt_emulation_thread+0x501/0x7a0 [xengt]
[ 290.587208] ? __schedule+0x3c6/0x890
[ 290.587250] ? wait_woken+0x80/0x80
[ 290.587290] kthread+0xfc/0x130
[ 290.587326] ? xengt_gpa_to_va+0x1f0/0x1f0 [xengt]
[ 290.587378] ? kthread_create_on_node+0x70/0x70
[ 290.587429] ? do_group_exit+0x3a/0xa0
[ 290.587471] ret_from_fork+0x35/0x40

Signed-off-by: Changbin Du <changbin.du@intel.com>
---
 drivers/gpu/drm/i915/gvt/display.h  |  2 +-
 drivers/gpu/drm/i915/gvt/handlers.c | 13 +++++++++----
 2 files changed, 10 insertions(+), 5 deletions(-)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/i915/gvt/display.h b/drivers/gpu/drm/i915/gvt/display.h
index b46b868..ea7c1c5 100644
--- a/drivers/gpu/drm/i915/gvt/display.h
+++ b/drivers/gpu/drm/i915/gvt/display.h
@@ -67,7 +67,7 @@ 
 #define AUX_NATIVE_REPLY_NAK    (0x1 << 4)
 #define AUX_NATIVE_REPLY_DEFER  (0x2 << 4)
 
-#define AUX_BURST_SIZE          16
+#define AUX_BURST_SIZE          20
 
 /* DPCD addresses */
 #define DPCD_REV			0x000
diff --git a/drivers/gpu/drm/i915/gvt/handlers.c b/drivers/gpu/drm/i915/gvt/handlers.c
index a33c1c3e..b77adcc 100644
--- a/drivers/gpu/drm/i915/gvt/handlers.c
+++ b/drivers/gpu/drm/i915/gvt/handlers.c
@@ -900,11 +900,14 @@  static int dp_aux_ch_ctl_mmio_write(struct intel_vgpu *vgpu,
 		}
 
 		/*
-		 * Write request format: (command + address) occupies
-		 * 3 bytes, followed by (len + 1) bytes of data.
+		 * Write request format: Headr (command + address + size) occupies
+		 * 4 bytes, followed by (len + 1) bytes of data. See details at
+		 * intel_dp_aux_transfer().
 		 */
-		if (WARN_ON((len + 4) > AUX_BURST_SIZE))
+		if ((len + 1 + 4) > AUX_BURST_SIZE) {
+			gvt_vgpu_err("dp_aux_header: len %d is too large\n", len);
 			return -EINVAL;
+		}
 
 		/* unpack data from vreg to buf */
 		for (t = 0; t < 4; t++) {
@@ -968,8 +971,10 @@  static int dp_aux_ch_ctl_mmio_write(struct intel_vgpu *vgpu,
 		/*
 		 * Read reply format: ACK (1 byte) plus (len + 1) bytes of data.
 		 */
-		if (WARN_ON((len + 2) > AUX_BURST_SIZE))
+		if ((len + 2) > AUX_BURST_SIZE) {
+			gvt_vgpu_err("dp_aux_header: len %d is too large\n", len);
 			return -EINVAL;
+		}
 
 		/* read from virtual DPCD to vreg */
 		/* first 4 bytes: [ACK][addr][addr+1][addr+2] */

Comments

Fixes line pls.

On 2018.04.10 13:02:30 +0800, changbin.du@intel.com wrote:
> From: Changbin Du <changbin.du@intel.com>
> 
> The assertion for len is wrong, so fix it. And for where to validate
> user input, we should not warn by call trace.
> 
> [ 290.584739] WARNING: CPU: 0 PID: 1471 at drivers/gpu/drm/i915/gvt/handlers.c:969 dp_aux_ch_ctl_mmio_write+0x394/0x430 [i915]
> [ 290.586113] task: ffff880111fe8000 task.stack: ffffc90044a9c000
> [ 290.586192] RIP: e030:dp_aux_ch_ctl_mmio_write+0x394/0x430 [i915]
> [ 290.586258] RSP: e02b:ffffc90044a9fd88 EFLAGS: 00010282
> [ 290.586315] RAX: 0000000000000017 RBX: 0000000000000003 RCX: ffffffff82461148
> [ 290.586391] RDX: 0000000000000000 RSI: 0000000000000001 RDI: 0000000000000201
> [ 290.586468] RBP: ffffc90043ed1000 R08: 0000000000000248 R09: 00000000000003d8
> [ 290.586544] R10: ffffc90044bdd314 R11: 0000000000000011 R12: 0000000000064310
> [ 290.586621] R13: 00000000fe4003ff R14: ffffc900432d1008 R15: ffff88010fa7cb40
> [ 290.586701] FS: 0000000000000000(0000) GS:ffff880123200000(0000) knlGS:0000000000000000
> [ 290.586787] CS: e033 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 290.586849] CR2: 00007f67ea44e000 CR3: 0000000116078000 CR4: 0000000000042660
> [ 290.586926] Call Trace:
> [ 290.586958] ? __switch_to_asm+0x40/0x70
> [ 290.587017] intel_vgpu_mmio_reg_rw+0x1ec/0x3c0 [i915]
> [ 290.587087] intel_vgpu_emulate_mmio_write+0xa8/0x2c0 [i915]
> [ 290.587151] xengt_emulation_thread+0x501/0x7a0 [xengt]
> [ 290.587208] ? __schedule+0x3c6/0x890
> [ 290.587250] ? wait_woken+0x80/0x80
> [ 290.587290] kthread+0xfc/0x130
> [ 290.587326] ? xengt_gpa_to_va+0x1f0/0x1f0 [xengt]
> [ 290.587378] ? kthread_create_on_node+0x70/0x70
> [ 290.587429] ? do_group_exit+0x3a/0xa0
> [ 290.587471] ret_from_fork+0x35/0x40
> 
> Signed-off-by: Changbin Du <changbin.du@intel.com>
> ---
>  drivers/gpu/drm/i915/gvt/display.h  |  2 +-
>  drivers/gpu/drm/i915/gvt/handlers.c | 13 +++++++++----
>  2 files changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gvt/display.h b/drivers/gpu/drm/i915/gvt/display.h
> index b46b868..ea7c1c5 100644
> --- a/drivers/gpu/drm/i915/gvt/display.h
> +++ b/drivers/gpu/drm/i915/gvt/display.h
> @@ -67,7 +67,7 @@
>  #define AUX_NATIVE_REPLY_NAK    (0x1 << 4)
>  #define AUX_NATIVE_REPLY_DEFER  (0x2 << 4)
>  
> -#define AUX_BURST_SIZE          16
> +#define AUX_BURST_SIZE          20
>  
>  /* DPCD addresses */
>  #define DPCD_REV			0x000
> diff --git a/drivers/gpu/drm/i915/gvt/handlers.c b/drivers/gpu/drm/i915/gvt/handlers.c
> index a33c1c3e..b77adcc 100644
> --- a/drivers/gpu/drm/i915/gvt/handlers.c
> +++ b/drivers/gpu/drm/i915/gvt/handlers.c
> @@ -900,11 +900,14 @@ static int dp_aux_ch_ctl_mmio_write(struct intel_vgpu *vgpu,
>  		}
>  
>  		/*
> -		 * Write request format: (command + address) occupies
> -		 * 3 bytes, followed by (len + 1) bytes of data.
> +		 * Write request format: Headr (command + address + size) occupies
> +		 * 4 bytes, followed by (len + 1) bytes of data. See details at
> +		 * intel_dp_aux_transfer().
>  		 */
> -		if (WARN_ON((len + 4) > AUX_BURST_SIZE))
> +		if ((len + 1 + 4) > AUX_BURST_SIZE) {
> +			gvt_vgpu_err("dp_aux_header: len %d is too large\n", len);
>  			return -EINVAL;
> +		}
>  
>  		/* unpack data from vreg to buf */
>  		for (t = 0; t < 4; t++) {
> @@ -968,8 +971,10 @@ static int dp_aux_ch_ctl_mmio_write(struct intel_vgpu *vgpu,
>  		/*
>  		 * Read reply format: ACK (1 byte) plus (len + 1) bytes of data.
>  		 */
> -		if (WARN_ON((len + 2) > AUX_BURST_SIZE))
> +		if ((len + 2) > AUX_BURST_SIZE) {
> +			gvt_vgpu_err("dp_aux_header: len %d is too large\n", len);
>  			return -EINVAL;
> +		}
>  
>  		/* read from virtual DPCD to vreg */
>  		/* first 4 bytes: [ACK][addr][addr+1][addr+2] */
> -- 
> 2.7.4
> 
> _______________________________________________
> intel-gvt-dev mailing list
> intel-gvt-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev
On Wed, Apr 11, 2018 at 02:22:01PM +0800, Zhenyu Wang wrote:
> 
> Fixes line pls.
> 
This is pretty old code (maybe from initial state), still need Fixes tag?
Sometimes it is not easy to find original change which introduced misstakes.


> On 2018.04.10 13:02:30 +0800, changbin.du@intel.com wrote:
> > From: Changbin Du <changbin.du@intel.com>
> > 
> > The assertion for len is wrong, so fix it. And for where to validate
> > user input, we should not warn by call trace.
> > 
> > [ 290.584739] WARNING: CPU: 0 PID: 1471 at drivers/gpu/drm/i915/gvt/handlers.c:969 dp_aux_ch_ctl_mmio_write+0x394/0x430 [i915]
> > [ 290.586113] task: ffff880111fe8000 task.stack: ffffc90044a9c000
> > [ 290.586192] RIP: e030:dp_aux_ch_ctl_mmio_write+0x394/0x430 [i915]
> > [ 290.586258] RSP: e02b:ffffc90044a9fd88 EFLAGS: 00010282
> > [ 290.586315] RAX: 0000000000000017 RBX: 0000000000000003 RCX: ffffffff82461148
> > [ 290.586391] RDX: 0000000000000000 RSI: 0000000000000001 RDI: 0000000000000201
> > [ 290.586468] RBP: ffffc90043ed1000 R08: 0000000000000248 R09: 00000000000003d8
> > [ 290.586544] R10: ffffc90044bdd314 R11: 0000000000000011 R12: 0000000000064310
> > [ 290.586621] R13: 00000000fe4003ff R14: ffffc900432d1008 R15: ffff88010fa7cb40
> > [ 290.586701] FS: 0000000000000000(0000) GS:ffff880123200000(0000) knlGS:0000000000000000
> > [ 290.586787] CS: e033 DS: 0000 ES: 0000 CR0: 0000000080050033
> > [ 290.586849] CR2: 00007f67ea44e000 CR3: 0000000116078000 CR4: 0000000000042660
> > [ 290.586926] Call Trace:
> > [ 290.586958] ? __switch_to_asm+0x40/0x70
> > [ 290.587017] intel_vgpu_mmio_reg_rw+0x1ec/0x3c0 [i915]
> > [ 290.587087] intel_vgpu_emulate_mmio_write+0xa8/0x2c0 [i915]
> > [ 290.587151] xengt_emulation_thread+0x501/0x7a0 [xengt]
> > [ 290.587208] ? __schedule+0x3c6/0x890
> > [ 290.587250] ? wait_woken+0x80/0x80
> > [ 290.587290] kthread+0xfc/0x130
> > [ 290.587326] ? xengt_gpa_to_va+0x1f0/0x1f0 [xengt]
> > [ 290.587378] ? kthread_create_on_node+0x70/0x70
> > [ 290.587429] ? do_group_exit+0x3a/0xa0
> > [ 290.587471] ret_from_fork+0x35/0x40
> > 
> > Signed-off-by: Changbin Du <changbin.du@intel.com>
> > ---
> >  drivers/gpu/drm/i915/gvt/display.h  |  2 +-
> >  drivers/gpu/drm/i915/gvt/handlers.c | 13 +++++++++----
> >  2 files changed, 10 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/gvt/display.h b/drivers/gpu/drm/i915/gvt/display.h
> > index b46b868..ea7c1c5 100644
> > --- a/drivers/gpu/drm/i915/gvt/display.h
> > +++ b/drivers/gpu/drm/i915/gvt/display.h
> > @@ -67,7 +67,7 @@
> >  #define AUX_NATIVE_REPLY_NAK    (0x1 << 4)
> >  #define AUX_NATIVE_REPLY_DEFER  (0x2 << 4)
> >  
> > -#define AUX_BURST_SIZE          16
> > +#define AUX_BURST_SIZE          20
> >  
> >  /* DPCD addresses */
> >  #define DPCD_REV			0x000
> > diff --git a/drivers/gpu/drm/i915/gvt/handlers.c b/drivers/gpu/drm/i915/gvt/handlers.c
> > index a33c1c3e..b77adcc 100644
> > --- a/drivers/gpu/drm/i915/gvt/handlers.c
> > +++ b/drivers/gpu/drm/i915/gvt/handlers.c
> > @@ -900,11 +900,14 @@ static int dp_aux_ch_ctl_mmio_write(struct intel_vgpu *vgpu,
> >  		}
> >  
> >  		/*
> > -		 * Write request format: (command + address) occupies
> > -		 * 3 bytes, followed by (len + 1) bytes of data.
> > +		 * Write request format: Headr (command + address + size) occupies
> > +		 * 4 bytes, followed by (len + 1) bytes of data. See details at
> > +		 * intel_dp_aux_transfer().
> >  		 */
> > -		if (WARN_ON((len + 4) > AUX_BURST_SIZE))
> > +		if ((len + 1 + 4) > AUX_BURST_SIZE) {
> > +			gvt_vgpu_err("dp_aux_header: len %d is too large\n", len);
> >  			return -EINVAL;
> > +		}
> >  
> >  		/* unpack data from vreg to buf */
> >  		for (t = 0; t < 4; t++) {
> > @@ -968,8 +971,10 @@ static int dp_aux_ch_ctl_mmio_write(struct intel_vgpu *vgpu,
> >  		/*
> >  		 * Read reply format: ACK (1 byte) plus (len + 1) bytes of data.
> >  		 */
> > -		if (WARN_ON((len + 2) > AUX_BURST_SIZE))
> > +		if ((len + 2) > AUX_BURST_SIZE) {
> > +			gvt_vgpu_err("dp_aux_header: len %d is too large\n", len);
> >  			return -EINVAL;
> > +		}
> >  
> >  		/* read from virtual DPCD to vreg */
> >  		/* first 4 bytes: [ACK][addr][addr+1][addr+2] */
> > -- 
> > 2.7.4
> > 
> > _______________________________________________
> > intel-gvt-dev mailing list
> > intel-gvt-dev@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev
> 
> -- 
> Open Source Technology Center, Intel ltd.
> 
> $gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827
On 2018.04.11 16:11:15 +0800, Du, Changbin wrote:
> On Wed, Apr 11, 2018 at 02:22:01PM +0800, Zhenyu Wang wrote:
> > 
> > Fixes line pls.
> > 
> This is pretty old code (maybe from initial state), still need Fixes tag?
> Sometimes it is not easy to find original change which introduced misstakes.
>

yeah, should find by git-blame on the related change for this even it's very old commit.
On Wed, Apr 11, 2018 at 04:35:05PM +0800, Zhenyu Wang wrote:
> On 2018.04.11 16:11:15 +0800, Du, Changbin wrote:
> > On Wed, Apr 11, 2018 at 02:22:01PM +0800, Zhenyu Wang wrote:
> > > 
> > > Fixes line pls.
> > > 
> > This is pretty old code (maybe from initial state), still need Fixes tag?
> > Sometimes it is not easy to find original change which introduced misstakes.
> >
> 
> yeah, should find by git-blame on the related change for this even it's very old commit.
> 
ok, will add it for this one. Just note sometimes the code can be moved or restructured.

> -- 
> Open Source Technology Center, Intel ltd.
> 
> $gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827