| Message ID | 1454071949-24677-1-git-send-email-imre.deak@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 >
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) {
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) { >
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 >
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) {
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(-)