Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1570090 > unrolled thread
| Started by | Sean Paul <seanpaul@chromium.org> |
|---|---|
| First post | 2017-01-30 21:20 +0100 |
| Last post | 2017-01-31 15:50 +0100 |
| Articles | 3 — 2 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
Re: [PATCH v3 06/24] drm/rockchip: dw-mipi-dsi: avoid out-of-bounds read on tx_buf Sean Paul <seanpaul@chromium.org> - 2017-01-30 21:20 +0100
Re: [PATCH v3 06/24] drm/rockchip: dw-mipi-dsi: avoid out-of-bounds read on tx_buf John Keeping <john@metanate.com> - 2017-01-31 12:50 +0100
Re: [PATCH v3 06/24] drm/rockchip: dw-mipi-dsi: avoid out-of-bounds read on tx_buf Sean Paul <seanpaul@chromium.org> - 2017-01-31 15:50 +0100
| From | Sean Paul <seanpaul@chromium.org> |
|---|---|
| Date | 2017-01-30 21:20 +0100 |
| Subject | Re: [PATCH v3 06/24] drm/rockchip: dw-mipi-dsi: avoid out-of-bounds read on tx_buf |
| Message-ID | <t5qAG-8qt-9@gated-at.bofh.it> |
On Mon, Jan 30, 2017 at 06:16:36PM +0000, John Keeping wrote:
> On Mon, 30 Jan 2017 13:01:46 -0500, Sean Paul wrote:
>
> > On Sun, Jan 29, 2017 at 01:24:26PM +0000, John Keeping wrote:
> > > As a side-effect of this, encode the endianness explicitly rather than
> > > casting a u16.
> > >
> > > Signed-off-by: John Keeping <john@metanate.com>
> > > Reviewed-by: Chris Zhong <zyw@rock-chips.com>
> > > ---
> > > v3:
> > > - Add Chris' Reviewed-by
> > > Unchanged in v2
> > >
> > > drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 9 +++++++--
> > > 1 file changed, 7 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
> > > index 4be1ff3a42bb..2e6ad4591ebf 100644
> > > --- a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
> > > +++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
> > > @@ -572,8 +572,13 @@ static int dw_mipi_dsi_gen_pkt_hdr_write(struct dw_mipi_dsi *dsi, u32 hdr_val)
> > > static int dw_mipi_dsi_dcs_short_write(struct dw_mipi_dsi *dsi,
> > > const struct mipi_dsi_msg *msg)
> > > {
> > > - const u16 *tx_buf = msg->tx_buf;
> > > - u32 val = GEN_HDATA(*tx_buf) | GEN_HTYPE(msg->type);
> > > + const u8 *tx_buf = msg->tx_buf;
> > > + u32 val = GEN_HTYPE(msg->type);
> > > +
> > > + if (msg->tx_len > 0)
> > > + val |= GEN_HDATA(tx_buf[0]);
> > > + if (msg->tx_len > 1)
> > > + val |= GEN_HDATA(tx_buf[1] << 8);
> >
> > You should probably update the mask inside GEN_HDATA to mask off 8 bits instead of
> > 16.
>
> Won't that mask off the data written by "tx_buf[1] << 8"?
I would move the shift outside the mask, ie:
val |= GEN_HDATA(tx_buf[1]) << 8;
Sean
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Sean Paul, Software Engineer, Google / Chromium OS
[toc] | [next] | [standalone]
| From | John Keeping <john@metanate.com> |
|---|---|
| Date | 2017-01-31 12:50 +0100 |
| Message-ID | <t5F6F-bX-13@gated-at.bofh.it> |
| In reply to | #1570090 |
On Mon, 30 Jan 2017 15:09:55 -0500, Sean Paul wrote:
> On Mon, Jan 30, 2017 at 06:16:36PM +0000, John Keeping wrote:
> > On Mon, 30 Jan 2017 13:01:46 -0500, Sean Paul wrote:
> >
> > > On Sun, Jan 29, 2017 at 01:24:26PM +0000, John Keeping wrote:
> > > > As a side-effect of this, encode the endianness explicitly rather than
> > > > casting a u16.
> > > >
> > > > Signed-off-by: John Keeping <john@metanate.com>
> > > > Reviewed-by: Chris Zhong <zyw@rock-chips.com>
> > > > ---
> > > > v3:
> > > > - Add Chris' Reviewed-by
> > > > Unchanged in v2
> > > >
> > > > drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 9 +++++++--
> > > > 1 file changed, 7 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
> > > > index 4be1ff3a42bb..2e6ad4591ebf 100644
> > > > --- a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
> > > > +++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
> > > > @@ -572,8 +572,13 @@ static int dw_mipi_dsi_gen_pkt_hdr_write(struct dw_mipi_dsi *dsi, u32 hdr_val)
> > > > static int dw_mipi_dsi_dcs_short_write(struct dw_mipi_dsi *dsi,
> > > > const struct mipi_dsi_msg *msg)
> > > > {
> > > > - const u16 *tx_buf = msg->tx_buf;
> > > > - u32 val = GEN_HDATA(*tx_buf) | GEN_HTYPE(msg->type);
> > > > + const u8 *tx_buf = msg->tx_buf;
> > > > + u32 val = GEN_HTYPE(msg->type);
> > > > +
> > > > + if (msg->tx_len > 0)
> > > > + val |= GEN_HDATA(tx_buf[0]);
> > > > + if (msg->tx_len > 1)
> > > > + val |= GEN_HDATA(tx_buf[1] << 8);
> > >
> > > You should probably update the mask inside GEN_HDATA to mask off 8 bits instead of
> > > 16.
> >
> > Won't that mask off the data written by "tx_buf[1] << 8"?
>
> I would move the shift outside the mask, ie:
>
> val |= GEN_HDATA(tx_buf[1]) << 8;
I can do that, but that doesn't seem to match the intention of the
macros which are about encoding the placement and size of fields within
registers.
Maybe it would be clearer to do:
u16 data = 0;
if (msg->tx_len > 0)
data |= tx_buf[0];
if (msg->tx_len > 1)
data |= tx_buf[1];
val = GEN_HDATA(data) | GEN_HTYPE(msg->type);
?
[toc] | [prev] | [next] | [standalone]
| From | Sean Paul <seanpaul@chromium.org> |
|---|---|
| Date | 2017-01-31 15:50 +0100 |
| Message-ID | <t5HUS-1S1-25@gated-at.bofh.it> |
| In reply to | #1570639 |
On Tue, Jan 31, 2017 at 11:45:48AM +0000, John Keeping wrote:
> On Mon, 30 Jan 2017 15:09:55 -0500, Sean Paul wrote:
>
> > On Mon, Jan 30, 2017 at 06:16:36PM +0000, John Keeping wrote:
> > > On Mon, 30 Jan 2017 13:01:46 -0500, Sean Paul wrote:
> > >
> > > > On Sun, Jan 29, 2017 at 01:24:26PM +0000, John Keeping wrote:
> > > > > As a side-effect of this, encode the endianness explicitly rather than
> > > > > casting a u16.
> > > > >
> > > > > Signed-off-by: John Keeping <john@metanate.com>
> > > > > Reviewed-by: Chris Zhong <zyw@rock-chips.com>
> > > > > ---
> > > > > v3:
> > > > > - Add Chris' Reviewed-by
> > > > > Unchanged in v2
> > > > >
> > > > > drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 9 +++++++--
> > > > > 1 file changed, 7 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
> > > > > index 4be1ff3a42bb..2e6ad4591ebf 100644
> > > > > --- a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
> > > > > +++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
> > > > > @@ -572,8 +572,13 @@ static int dw_mipi_dsi_gen_pkt_hdr_write(struct dw_mipi_dsi *dsi, u32 hdr_val)
> > > > > static int dw_mipi_dsi_dcs_short_write(struct dw_mipi_dsi *dsi,
> > > > > const struct mipi_dsi_msg *msg)
> > > > > {
> > > > > - const u16 *tx_buf = msg->tx_buf;
> > > > > - u32 val = GEN_HDATA(*tx_buf) | GEN_HTYPE(msg->type);
> > > > > + const u8 *tx_buf = msg->tx_buf;
> > > > > + u32 val = GEN_HTYPE(msg->type);
> > > > > +
> > > > > + if (msg->tx_len > 0)
> > > > > + val |= GEN_HDATA(tx_buf[0]);
> > > > > + if (msg->tx_len > 1)
> > > > > + val |= GEN_HDATA(tx_buf[1] << 8);
> > > >
> > > > You should probably update the mask inside GEN_HDATA to mask off 8 bits instead of
> > > > 16.
> > >
> > > Won't that mask off the data written by "tx_buf[1] << 8"?
> >
> > I would move the shift outside the mask, ie:
> >
> > val |= GEN_HDATA(tx_buf[1]) << 8;
>
> I can do that, but that doesn't seem to match the intention of the
> macros which are about encoding the placement and size of fields within
> registers.
>
> Maybe it would be clearer to do:
>
> u16 data = 0;
>
> if (msg->tx_len > 0)
> data |= tx_buf[0];
> if (msg->tx_len > 1)
> data |= tx_buf[1];
Yep, this looks good to me, but I think you need:
data |= tx_buf[1] << 8;
Sean
>
> val = GEN_HDATA(data) | GEN_HTYPE(msg->type);
>
> ?
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Sean Paul, Software Engineer, Google / Chromium OS
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web