[PATCH 1/4] drm/i915: Sanity check DP AUX message buffer and size

[PATCH 1/4] drm/i915: Sanity check DP AUX message buffer and size

[Imre Deak]

While we are calling intel_dp_aux_transfer() with msg->size=0 whenever
msg->buffer is NULL, passing NULL to memcpy() is undefined according to
the ISO C standard. I haven't found any notes about this in the GNU C's
or the kernel's documentation of the function and can't imagine what it
would do with the NULL ptr. To better document this use of the
parameters it still make sense to add an explicit check for this to the
code.

Caught by Coverity.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index e2bea710..2aed36e 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -979,7 +979,10 @@ intel_dp_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
 		if (WARN_ON(txsize > 20))
 			return -E2BIG;
 
-		memcpy(txbuf + HEADER_SIZE, msg->buffer, msg->size);
+		if (msg->buffer)
+			memcpy(txbuf + HEADER_SIZE, msg->buffer, msg->size);
+		else
+			WARN_ON(msg->size);
 
 		ret = intel_dp_aux_ch(intel_dp, txbuf, txsize, rxbuf, rxsize);
 		if (ret > 0) {
-- 
2.5.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/4] drm/i915/chv: Fix error path in GPU freq helpers

[Imre Deak]

Atm we wouldn't catch these errors or on the error path we would end up
with a division-by-zero, fix this up.

Caught by Coverity.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 20bf854..740a0e3 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -7189,9 +7189,10 @@ static int chv_gpu_freq(struct drm_i915_private *dev_priv, int val)
 {
 	int div, czclk_freq = DIV_ROUND_CLOSEST(dev_priv->czclk_freq, 1000);
 
-	div = vlv_gpu_freq_div(czclk_freq) / 2;
+	div = vlv_gpu_freq_div(czclk_freq);
 	if (div < 0)
 		return div;
+	div /= 2;
 
 	return DIV_ROUND_CLOSEST(czclk_freq * val, 2 * div) / 2;
 }
@@ -7200,9 +7201,10 @@ static int chv_freq_opcode(struct drm_i915_private *dev_priv, int val)
 {
 	int mul, czclk_freq = DIV_ROUND_CLOSEST(dev_priv->czclk_freq, 1000);
 
-	mul = vlv_gpu_freq_div(czclk_freq) / 2;
+	mul = vlv_gpu_freq_div(czclk_freq);
 	if (mul < 0)
 		return mul;
+	mul /= 2;
 
 	/* CHV needs even values */
 	return DIV_ROUND_CLOSEST(val * 2 * mul, czclk_freq) * 2;
-- 
2.5.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/4] drm/i915/chv: Fix error path in GPU freq helpers

[David Weinehall]

On Fri, Jan 29, 2016 at 02:52:27PM +0200, Imre Deak wrote:
> Atm we wouldn't catch these errors or on the error path we would end up
> with a division-by-zero, fix this up.
> 
> Caught by Coverity.
> 
> Signed-off-by: Imre Deak <imre.deak@intel.com>

Reviewed-by: David Weinehall <david.weinehall@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_pm.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 20bf854..740a0e3 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -7189,9 +7189,10 @@ static int chv_gpu_freq(struct drm_i915_private *dev_priv, int val)
>  {
>  	int div, czclk_freq = DIV_ROUND_CLOSEST(dev_priv->czclk_freq, 1000);
>  
> -	div = vlv_gpu_freq_div(czclk_freq) / 2;
> +	div = vlv_gpu_freq_div(czclk_freq);
>  	if (div < 0)
>  		return div;
> +	div /= 2;
>  
>  	return DIV_ROUND_CLOSEST(czclk_freq * val, 2 * div) / 2;
>  }
> @@ -7200,9 +7201,10 @@ static int chv_freq_opcode(struct drm_i915_private *dev_priv, int val)
>  {
>  	int mul, czclk_freq = DIV_ROUND_CLOSEST(dev_priv->czclk_freq, 1000);
>  
> -	mul = vlv_gpu_freq_div(czclk_freq) / 2;
> +	mul = vlv_gpu_freq_div(czclk_freq);
>  	if (mul < 0)
>  		return mul;
> +	mul /= 2;
>  
>  	/* CHV needs even values */
>  	return DIV_ROUND_CLOSEST(val * 2 * mul, czclk_freq) * 2;
> -- 
> 2.5.0
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 3/4] drm/i915: Add debug info for failed MSI enabling

[Imre Deak]

While not being able to enable MSI interrupts may be a normal
circumstance, for debugging it may still be a useful information, so
emit an info about this.

Caught by Coverity.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_dma.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index db9b0c6..fd3a6d2 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1078,8 +1078,10 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
 	 * be lost or delayed, but we use them anyways to avoid
 	 * stuck interrupts on some machines.
 	 */
-	if (!IS_I945G(dev) && !IS_I945GM(dev))
-		pci_enable_msi(dev->pdev);
+	if (!IS_I945G(dev) && !IS_I945GM(dev)) {
+		if (pci_enable_msi(dev->pdev) < 0)
+			DRM_DEBUG_DRIVER("can't enable MSI");
+	}
 
 	intel_device_info_runtime_init(dev);
 
-- 
2.5.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/4] drm/i915: Add debug info for failed MSI enabling

[David Weinehall]

On Fri, Jan 29, 2016 at 02:52:28PM +0200, Imre Deak wrote:
> While not being able to enable MSI interrupts may be a normal
> circumstance, for debugging it may still be a useful information, so
> emit an info about this.
> 
> Caught by Coverity.
> 
> Signed-off-by: Imre Deak <imre.deak@intel.com>

Reviewed-by: David Weinehall <david.weinehall@intel.com>

> ---
>  drivers/gpu/drm/i915/i915_dma.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index db9b0c6..fd3a6d2 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -1078,8 +1078,10 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
>  	 * be lost or delayed, but we use them anyways to avoid
>  	 * stuck interrupts on some machines.
>  	 */
> -	if (!IS_I945G(dev) && !IS_I945GM(dev))
> -		pci_enable_msi(dev->pdev);
> +	if (!IS_I945G(dev) && !IS_I945GM(dev)) {
> +		if (pci_enable_msi(dev->pdev) < 0)
> +			DRM_DEBUG_DRIVER("can't enable MSI");
> +	}
>  
>  	intel_device_info_runtime_init(dev);
>  
> -- 
> 2.5.0
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 4/4] drm/i915: Properly terminate KMS mode name string during tv init

[Imre Deak]

Caught by Coverity.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/intel_tv.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c
index 948cbff..5034b00 100644
--- a/drivers/gpu/drm/i915/intel_tv.c
+++ b/drivers/gpu/drm/i915/intel_tv.c
@@ -1420,6 +1420,7 @@ intel_tv_get_modes(struct drm_connector *connector)
 		if (!mode_ptr)
 			continue;
 		strncpy(mode_ptr->name, input->name, DRM_DISPLAY_MODE_LEN);
+		mode_ptr->name[DRM_DISPLAY_MODE_LEN - 1] = '\0';
 
 		mode_ptr->hdisplay = hactive_s;
 		mode_ptr->hsync_start = hactive_s + 1;
-- 
2.5.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/4] drm/i915: Properly terminate KMS mode name string during tv init

[David Weinehall]

On Fri, Jan 29, 2016 at 02:52:29PM +0200, Imre Deak wrote:
> Caught by Coverity.
> 
> Signed-off-by: Imre Deak <imre.deak@intel.com>

Reviewed-by: David Weinehall <david.weinehall@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_tv.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c
> index 948cbff..5034b00 100644
> --- a/drivers/gpu/drm/i915/intel_tv.c
> +++ b/drivers/gpu/drm/i915/intel_tv.c
> @@ -1420,6 +1420,7 @@ intel_tv_get_modes(struct drm_connector *connector)
>  		if (!mode_ptr)
>  			continue;
>  		strncpy(mode_ptr->name, input->name, DRM_DISPLAY_MODE_LEN);
> +		mode_ptr->name[DRM_DISPLAY_MODE_LEN - 1] = '\0';
>  
>  		mode_ptr->hdisplay = hactive_s;
>  		mode_ptr->hsync_start = hactive_s + 1;
> -- 
> 2.5.0
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: failure for series starting with [1/4] drm/i915: Sanity check DP AUX message buffer and size

[Patchwork]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for series starting with [1/4] drm/i915: Sanity check DP AUX message buffer and size

[Patchwork]

== Summary ==

Series 2922v1 Series without cover letter
http://patchwork.freedesktop.org/api/1.0/series/2922/revisions/1/mbox/


bdw-nuci7        total:156  pass:147  dwarn:0   dfail:0   fail:0   skip:9  
bdw-ultra        total:159  pass:147  dwarn:0   dfail:0   fail:0   skip:12 
bsw-nuc-2        total:159  pass:129  dwarn:0   dfail:0   fail:0   skip:30 
hsw-brixbox      total:159  pass:146  dwarn:0   dfail:0   fail:0   skip:13 
hsw-gt2          total:159  pass:149  dwarn:0   dfail:0   fail:0   skip:10 
ilk-hp8440p      total:159  pass:111  dwarn:0   dfail:0   fail:0   skip:48 
ivb-t430s        total:159  pass:145  dwarn:0   dfail:0   fail:0   skip:14 
skl-i5k-2        total:159  pass:144  dwarn:1   dfail:0   fail:0   skip:14 
snb-dellxps      total:159  pass:137  dwarn:0   dfail:0   fail:0   skip:22 
snb-x220t        total:159  pass:137  dwarn:0   dfail:0   fail:1   skip:21 

Results at /archive/results/CI_IGT_test/Patchwork_1320/

5de97b25e5f3c5a63ee243a9d3b22d30792f7d3e drm-intel-nightly: 2016y-01m-29d-07h-32m-09s UTC integration manifest
57b87311f5051584bdfc69424b99e4db48c1872f drm/i915: Properly terminate KMS mode name string during tv init
54e3ae2b95671fb9e613a3bb7bdf598c456382d6 drm/i915: Add debug info for failed MSI enabling
1beca05c239fe210a28fb40d62f9ada1446d1376 drm/i915/chv: Fix error path in GPU freq helpers
39e351cc5d9bd16e84dd896445878a57b90b3ce1 drm/i915: Sanity check DP AUX message buffer and size

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: ✓ Fi.CI.BAT: success for series starting with [1/4] drm/i915: Sanity check DP AUX message buffer and size

[Imre Deak]

On pe, 2016-01-29 at 13:19 +0000, Patchwork wrote:
> == Summary ==
> 
> Series 2922v1 Series without cover letter
> http://patchwork.freedesktop.org/api/1.0/series/2922/revisions/1/mbox
> /

Thanks for the review, I pushed the patchset to dinq.

--Imre

> 
> 
> bdw-
> nuci7        total:156  pass:147  dwarn:0   dfail:0   fail:0   skip:9
>   
> bdw-
> ultra        total:159  pass:147  dwarn:0   dfail:0   fail:0   skip:1
> 2 
> bsw-nuc-
> 2        total:159  pass:129  dwarn:0   dfail:0   fail:0   skip:30 
> hsw-
> brixbox      total:159  pass:146  dwarn:0   dfail:0   fail:0   skip:1
> 3 
> hsw-
> gt2          total:159  pass:149  dwarn:0   dfail:0   fail:0   skip:1
> 0 
> ilk-
> hp8440p      total:159  pass:111  dwarn:0   dfail:0   fail:0   skip:4
> 8 
> ivb-
> t430s        total:159  pass:145  dwarn:0   dfail:0   fail:0   skip:1
> 4 
> skl-i5k-
> 2        total:159  pass:144  dwarn:1   dfail:0   fail:0   skip:14 
> snb-
> dellxps      total:159  pass:137  dwarn:0   dfail:0   fail:0   skip:2
> 2 
> snb-
> x220t        total:159  pass:137  dwarn:0   dfail:0   fail:1   skip:2
> 1 
> 
> Results at /archive/results/CI_IGT_test/Patchwork_1320/
> 
> 5de97b25e5f3c5a63ee243a9d3b22d30792f7d3e drm-intel-nightly: 2016y-
> 01m-29d-07h-32m-09s UTC integration manifest
> 57b87311f5051584bdfc69424b99e4db48c1872f drm/i915: Properly terminate
> KMS mode name string during tv init
> 54e3ae2b95671fb9e613a3bb7bdf598c456382d6 drm/i915: Add debug info for
> failed MSI enabling
> 1beca05c239fe210a28fb40d62f9ada1446d1376 drm/i915/chv: Fix error path
> in GPU freq helpers
> 39e351cc5d9bd16e84dd896445878a57b90b3ce1 drm/i915: Sanity check DP
> AUX message buffer and size
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/4] drm/i915: Sanity check DP AUX message buffer and size

[David Weinehall]

On Fri, Jan 29, 2016 at 02:52:26PM +0200, Imre Deak wrote:
> While we are calling intel_dp_aux_transfer() with msg->size=0 whenever
> msg->buffer is NULL, passing NULL to memcpy() is undefined according to
> the ISO C standard. I haven't found any notes about this in the GNU C's
> or the kernel's documentation of the function and can't imagine what it
> would do with the NULL ptr. To better document this use of the
> parameters it still make sense to add an explicit check for this to the
> code.
> 
> Caught by Coverity.
> 
> Signed-off-by: Imre Deak <imre.deak@intel.com>

Reviewed-by: David Weinehall <david.weinehall@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_dp.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index e2bea710..2aed36e 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -979,7 +979,10 @@ intel_dp_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
>  		if (WARN_ON(txsize > 20))
>  			return -E2BIG;
>  
> -		memcpy(txbuf + HEADER_SIZE, msg->buffer, msg->size);
> +		if (msg->buffer)
> +			memcpy(txbuf + HEADER_SIZE, msg->buffer, msg->size);
> +		else
> +			WARN_ON(msg->size);
>  
>  		ret = intel_dp_aux_ch(intel_dp, txbuf, txsize, rxbuf, rxsize);
>  		if (ret > 0) {
> -- 
> 2.5.0
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/4] drm/i915: Sanity check DP AUX message buffer and size

[Thulasimani, Sivakumar]

On 1/29/2016 6:22 PM, Imre Deak wrote:
> While we are calling intel_dp_aux_transfer() with msg->size=0 whenever
> msg->buffer is NULL, passing NULL to memcpy() is undefined according to
> the ISO C standard. I haven't found any notes about this in the GNU C's
> or the kernel's documentation of the function and can't imagine what it
> would do with the NULL ptr. To better document this use of the
> parameters it still make sense to add an explicit check for this to the
> code.
>
> Caught by Coverity.
can you share more info on when is this scenario triggered ?
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_dp.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index e2bea710..2aed36e 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -979,7 +979,10 @@ intel_dp_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
>   		if (WARN_ON(txsize > 20))
>   			return -E2BIG;
>   
> -		memcpy(txbuf + HEADER_SIZE, msg->buffer, msg->size);
> +		if (msg->buffer)
> +			memcpy(txbuf + HEADER_SIZE, msg->buffer, msg->size);
> +		else
> +			WARN_ON(msg->size);
>   
>   		ret = intel_dp_aux_ch(intel_dp, txbuf, txsize, rxbuf, rxsize);
>   		if (ret > 0) {

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/4] drm/i915: Sanity check DP AUX message buffer and size

[Imre Deak]

On ma, 2016-02-01 at 11:42 +0530, Thulasimani, Sivakumar wrote:
> 
> On 1/29/2016 6:22 PM, Imre Deak wrote:
> > While we are calling intel_dp_aux_transfer() with msg->size=0
> > whenever
> > msg->buffer is NULL, passing NULL to memcpy() is undefined
> > according to
> > the ISO C standard. I haven't found any notes about this in the GNU
> > C's
> > or the kernel's documentation of the function and can't imagine
> > what it
> > would do with the NULL ptr. To better document this use of the
> > parameters it still make sense to add an explicit check for this to
> > the
> > code.
> > 
> > Caught by Coverity.
> can you share more info on when is this scenario triggered ?

When sending a bare address packet at the start and end of the I2c over
AUX transfer. See drm_dp_i2c_xfer().

> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >   drivers/gpu/drm/i915/intel_dp.c | 5 ++++-
> >   1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > b/drivers/gpu/drm/i915/intel_dp.c
> > index e2bea710..2aed36e 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -979,7 +979,10 @@ intel_dp_aux_transfer(struct drm_dp_aux *aux,
> > struct drm_dp_aux_msg *msg)
> >   		if (WARN_ON(txsize > 20))
> >   			return -E2BIG;
> >   
> > -		memcpy(txbuf + HEADER_SIZE, msg->buffer, msg-
> > >size);
> > +		if (msg->buffer)
> > +			memcpy(txbuf + HEADER_SIZE, msg->buffer,
> > msg->size);
> > +		else
> > +			WARN_ON(msg->size);
> >   
> >   		ret = intel_dp_aux_ch(intel_dp, txbuf, txsize,
> > rxbuf, rxsize);
> >   		if (ret > 0) {
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx