| Message ID | 1378807612-18399-2-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Sep 10, 2013 at 12:06:51PM +0200, Daniel Vetter wrote: > We've failed to properly clear out the flags when converting a dtd to > a drm mode. For more paranoia just memset the entire structure (and > drop the now redundant clears). > > Also since > > commit 135c81b8c3c9a70d7b55758c9c2a247a4abb7b64 > Author: Daniel Vetter <daniel.vetter@ffwll.ch> > Date: Sun Jul 21 21:37:09 2013 +0200 > > drm/i915: clean up crtc timings computation > > we don't update the crtc timings any more properly, so do that again. > > Cc: Rodrigo Vivi <rodrigo.vivi@gmail.com> > Cc: Jesse Barnes <jbarnes@virtuousgeek.org> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Reported-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > drivers/gpu/drm/i915/intel_sdvo.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c > index 5033c74..d4a046d 100644 > --- a/drivers/gpu/drm/i915/intel_sdvo.c > +++ b/drivers/gpu/drm/i915/intel_sdvo.c > @@ -788,6 +788,8 @@ static void intel_sdvo_get_dtd_from_mode(struct intel_sdvo_dtd *dtd, > uint16_t h_sync_offset, v_sync_offset; > int mode_clock; > > + memset(dtd, 0, sizeof(*dtd)); > + > width = mode->hdisplay; > height = mode->vdisplay; > > @@ -830,14 +832,14 @@ static void intel_sdvo_get_dtd_from_mode(struct intel_sdvo_dtd *dtd, > if (mode->flags & DRM_MODE_FLAG_PVSYNC) > dtd->part2.dtd_flags |= DTD_FLAG_VSYNC_POSITIVE; > > - dtd->part2.sdvo_flags = 0; > dtd->part2.v_sync_off_high = v_sync_offset & 0xc0; > - dtd->part2.reserved = 0; > } > > static void intel_sdvo_get_mode_from_dtd(struct drm_display_mode * mode, > const struct intel_sdvo_dtd *dtd) > { > + memset(mode, 0, sizeof(*mode)); I have a theoretical worry that someone might end up calling this on a mode that sits on some list or was actually allocated and has a proper object id which we'd leak here. To make it totally safe you could populate a pristine mode struct and use drm_mode_copy() to overwrite adjusted_mode. Assuming we're not so short on stack space that our oversized mode struct would cause issues. Other options would be to add some WARNs to catch wrongdoers, or embed a temp mode for this purpose inside the intel_sdvo struct. > + > mode->hdisplay = dtd->part1.h_active; > mode->hdisplay += ((dtd->part1.h_high >> 4) & 0x0f) << 8; > mode->hsync_start = mode->hdisplay + dtd->part2.h_sync_off; > @@ -872,6 +874,8 @@ static void intel_sdvo_get_mode_from_dtd(struct drm_display_mode * mode, Somewhere around these parts we have: 'mode->flags &= ~(DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC);' It can now be eliminated. > mode->flags |= DRM_MODE_FLAG_PVSYNC; > else > mode->flags |= DRM_MODE_FLAG_NVSYNC; > + > + drm_mode_set_crtcinfo(mode); > } > > static bool intel_sdvo_check_supp_encode(struct intel_sdvo *intel_sdvo) > -- > 1.8.4.rc3
On Tue, Sep 10, 2013 at 12:26 PM, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: >> static void intel_sdvo_get_mode_from_dtd(struct drm_display_mode * mode, >> const struct intel_sdvo_dtd *dtd) >> { >> + memset(mode, 0, sizeof(*mode)); > > I have a theoretical worry that someone might end up calling this on a > mode that sits on some list or was actually allocated and has a proper > object id which we'd leak here. > > To make it totally safe you could populate a pristine mode struct and > use drm_mode_copy() to overwrite adjusted_mode. Assuming we're not so > short on stack space that our oversized mode struct would cause issues. > Other options would be to add some WARNs to catch wrongdoers, or embed > a temp mode for this purpose inside the intel_sdvo struct. We can't really check for this since list_empty on stack garbage won't work too well, either. And e.g. ->get_config has the pipe config on the stack. So I think we just need to do review here. I also think the risk is pretty low, this is all used in internal structures around pipe_config, where the mode is never linked. >> + >> mode->hdisplay = dtd->part1.h_active; >> mode->hdisplay += ((dtd->part1.h_high >> 4) & 0x0f) << 8; >> mode->hsync_start = mode->hdisplay + dtd->part2.h_sync_off; >> @@ -872,6 +874,8 @@ static void intel_sdvo_get_mode_from_dtd(struct drm_display_mode * mode, > > Somewhere around these parts we have: > > 'mode->flags &= ~(DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC);' > > It can now be eliminated. Will fix and resend. -Daniel
On Tue, Sep 10, 2013 at 12:50:25PM +0200, Daniel Vetter wrote: > On Tue, Sep 10, 2013 at 12:26 PM, Ville Syrjälä > <ville.syrjala@linux.intel.com> wrote: > >> static void intel_sdvo_get_mode_from_dtd(struct drm_display_mode * mode, > >> const struct intel_sdvo_dtd *dtd) > >> { > >> + memset(mode, 0, sizeof(*mode)); > > > > I have a theoretical worry that someone might end up calling this on a > > mode that sits on some list or was actually allocated and has a proper > > object id which we'd leak here. > > > > To make it totally safe you could populate a pristine mode struct and > > use drm_mode_copy() to overwrite adjusted_mode. Assuming we're not so > > short on stack space that our oversized mode struct would cause issues. > > Other options would be to add some WARNs to catch wrongdoers, or embed > > a temp mode for this purpose inside the intel_sdvo struct. > > We can't really check for this since list_empty on stack garbage won't > work too well, either. And e.g. ->get_config has the pipe config on > the stack. So I think we just need to do review here. I also think the > risk is pretty low, this is all used in internal structures around > pipe_config, where the mode is never linked. Well, another idea would be to add drm_mode_clear() what would do the memset() but preserve the id and list head.
On Tue, Sep 10, 2013 at 1:00 PM, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > On Tue, Sep 10, 2013 at 12:50:25PM +0200, Daniel Vetter wrote: >> On Tue, Sep 10, 2013 at 12:26 PM, Ville Syrjälä >> <ville.syrjala@linux.intel.com> wrote: >> >> static void intel_sdvo_get_mode_from_dtd(struct drm_display_mode * mode, >> >> const struct intel_sdvo_dtd *dtd) >> >> { >> >> + memset(mode, 0, sizeof(*mode)); >> > >> > I have a theoretical worry that someone might end up calling this on a >> > mode that sits on some list or was actually allocated and has a proper >> > object id which we'd leak here. >> > >> > To make it totally safe you could populate a pristine mode struct and >> > use drm_mode_copy() to overwrite adjusted_mode. Assuming we're not so >> > short on stack space that our oversized mode struct would cause issues. >> > Other options would be to add some WARNs to catch wrongdoers, or embed >> > a temp mode for this purpose inside the intel_sdvo struct. >> >> We can't really check for this since list_empty on stack garbage won't >> work too well, either. And e.g. ->get_config has the pipe config on >> the stack. So I think we just need to do review here. I also think the >> risk is pretty low, this is all used in internal structures around >> pipe_config, where the mode is never linked. > > Well, another idea would be to add drm_mode_clear() what would do the > memset() but preserve the id and list head. At least for the adjusted mode embedded into the pipe config that won't work either since we want to memset the entire thing to not miss any fields ... -Daniel
On Tue, Sep 10, 2013 at 02:26:10PM +0200, Daniel Vetter wrote: > On Tue, Sep 10, 2013 at 1:00 PM, Ville Syrjälä > <ville.syrjala@linux.intel.com> wrote: > > On Tue, Sep 10, 2013 at 12:50:25PM +0200, Daniel Vetter wrote: > >> On Tue, Sep 10, 2013 at 12:26 PM, Ville Syrjälä > >> <ville.syrjala@linux.intel.com> wrote: > >> >> static void intel_sdvo_get_mode_from_dtd(struct drm_display_mode * mode, > >> >> const struct intel_sdvo_dtd *dtd) > >> >> { > >> >> + memset(mode, 0, sizeof(*mode)); > >> > > >> > I have a theoretical worry that someone might end up calling this on a > >> > mode that sits on some list or was actually allocated and has a proper > >> > object id which we'd leak here. > >> > > >> > To make it totally safe you could populate a pristine mode struct and > >> > use drm_mode_copy() to overwrite adjusted_mode. Assuming we're not so > >> > short on stack space that our oversized mode struct would cause issues. > >> > Other options would be to add some WARNs to catch wrongdoers, or embed > >> > a temp mode for this purpose inside the intel_sdvo struct. > >> > >> We can't really check for this since list_empty on stack garbage won't > >> work too well, either. And e.g. ->get_config has the pipe config on > >> the stack. So I think we just need to do review here. I also think the > >> risk is pretty low, this is all used in internal structures around > >> pipe_config, where the mode is never linked. > > > > Well, another idea would be to add drm_mode_clear() what would do the > > memset() but preserve the id and list head. > > At least for the adjusted mode embedded into the pipe config that > won't work either since we want to memset the entire thing to not miss > any fields ... drm_mode_clear() would skip only the obj id and list head just like drm_mode_copy(). Also isn't the pipe config supposed to be entirely zeroed to start with anyway? And we already use drm_mode_copy() to fill the initial values for adjusted_mode. drm_mode_clear() would overwrite exactly the same fields as drm_mode_copy() filled in.
diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c index 5033c74..d4a046d 100644 --- a/drivers/gpu/drm/i915/intel_sdvo.c +++ b/drivers/gpu/drm/i915/intel_sdvo.c @@ -788,6 +788,8 @@ static void intel_sdvo_get_dtd_from_mode(struct intel_sdvo_dtd *dtd, uint16_t h_sync_offset, v_sync_offset; int mode_clock; + memset(dtd, 0, sizeof(*dtd)); + width = mode->hdisplay; height = mode->vdisplay; @@ -830,14 +832,14 @@ static void intel_sdvo_get_dtd_from_mode(struct intel_sdvo_dtd *dtd, if (mode->flags & DRM_MODE_FLAG_PVSYNC) dtd->part2.dtd_flags |= DTD_FLAG_VSYNC_POSITIVE; - dtd->part2.sdvo_flags = 0; dtd->part2.v_sync_off_high = v_sync_offset & 0xc0; - dtd->part2.reserved = 0; } static void intel_sdvo_get_mode_from_dtd(struct drm_display_mode * mode, const struct intel_sdvo_dtd *dtd) { + memset(mode, 0, sizeof(*mode)); + mode->hdisplay = dtd->part1.h_active; mode->hdisplay += ((dtd->part1.h_high >> 4) & 0x0f) << 8; mode->hsync_start = mode->hdisplay + dtd->part2.h_sync_off; @@ -872,6 +874,8 @@ static void intel_sdvo_get_mode_from_dtd(struct drm_display_mode * mode, mode->flags |= DRM_MODE_FLAG_PVSYNC; else mode->flags |= DRM_MODE_FLAG_NVSYNC; + + drm_mode_set_crtcinfo(mode); } static bool intel_sdvo_check_supp_encode(struct intel_sdvo *intel_sdvo)
We've failed to properly clear out the flags when converting a dtd to a drm mode. For more paranoia just memset the entire structure (and drop the now redundant clears). Also since commit 135c81b8c3c9a70d7b55758c9c2a247a4abb7b64 Author: Daniel Vetter <daniel.vetter@ffwll.ch> Date: Sun Jul 21 21:37:09 2013 +0200 drm/i915: clean up crtc timings computation we don't update the crtc timings any more properly, so do that again. Cc: Rodrigo Vivi <rodrigo.vivi@gmail.com> Cc: Jesse Barnes <jbarnes@virtuousgeek.org> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Reported-by: Ville Syrjälä <ville.syrjala@linux.intel.com> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> --- drivers/gpu/drm/i915/intel_sdvo.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)