linux-kernel - Re: [PATCH] drm/tegra: Fix crash caused by reference count imbalance

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Wed, 18 May 2016 12:26:47 +0200
From:	Daniel Vetter <daniel@...ll.ch>
To:	Jon Hunter <jonathanh@...dia.com>
Cc:	Daniel Vetter <daniel@...ll.ch>,
	Thierry Reding <thierry.reding@...il.com>,
	David Airlie <airlied@...ux.ie>,
	Stephen Warren <swarren@...dotorg.org>,
	Alexandre Courbot <gnurou@...il.com>,
	"linux-tegra@...r.kernel.org" <linux-tegra@...r.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	dri-devel <dri-devel@...ts.freedesktop.org>
Subject: Re: [PATCH] drm/tegra: Fix crash caused by reference count imbalance

On Wed, May 18, 2016 at 10:18:52AM +0100, Jon Hunter wrote:
> 
> On 17/05/16 18:36, Daniel Vetter wrote:
> > On Tue, May 17, 2016 at 7:29 PM, Jon Hunter <jonathanh@...dia.com> wrote:
> >>>> @@ -764,6 +769,9 @@ tegra_dsi_connector_duplicate_state(struct drm_connector *connector)
> >>>>      if (!copy)
> >>>>              return NULL;
> >>>>
> >>>> +    if (copy->base.crtc)
> >>>> +            drm_connector_reference(connector);
> >>>> +
> >>>
> >>> Please use __drm_atomic_helper_connector_duplicate_state instead of
> >>> open-coding it.
> >>
> >> Unfortunately, tegra is allocating and duplicating memory for the entire
> >> tegra_dsi_state structure (of which drm_connector_state is a member) in
> >> this function and so I was not able to do that. However, may be Thierry
> >> can comment on whether that is completely necessary and if we can move
> >> to using __drm_atomic_helper_connector_duplicate_state() instead.
> > 
> > Check out how other drivers are using this helper - it is explicitly
> > for the case where you duplicate the entire struct, and it just
> > initializes the core part from drm. You can then add your own fixup
> > code afterwards. It also doesn't matter whether you do kmalloc or
> > kcalloc or kmemdup - it does a memcpy of its own to make sure state
> > gets copied.
> 
> I had a look but I don't see anyone using the
> __drm_atomic_helper_connector_duplicate_state() helper, I only see that
>  drivers are using drm_atomic_helper_connector_duplicate_state()
> directly.  
> 
> Yes I understand that this helper is doing an explicit copy of the
> entire drm_connector_state struct and yes I could do something like the
> following ...
> 
>  static struct drm_connector_state *
>  tegra_dsi_connector_duplicate_state(struct drm_connector *connector)
>  {
>          struct tegra_dsi_state *state = to_dsi_state(connector->state);
>          struct tegra_dsi_state *copy;
>  
>          copy = kmemdup(state, sizeof(*state), GFP_KERNEL);
>          if (!copy)
>                  return NULL;
> 
>          __drm_atomic_helper_connector_duplicate_state(connector,
> 						       &copy->base);
>  
>          return &copy->base;
>  }
> 
> ... however, this means that I am copying the drm_connector_state twice
> and this is what I was trying to avoid. Sorry if I am misunderstanding
> you here, but I don't see how I can avoid the 2nd copy if I use 
> __drm_atomic_helper_connector_duplicate_state().

The copying twice should be harmless - this function is only called when
changing connector states, i.e. full modeset. And modesets aren't fast
anyway.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch