netdev - Re: [PATCH net-next 01/13] tipc: eliminate case of writing to freed memory

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] [thread-next>] [day] [month] [year] [list]

Date:	Fri, 27 Jun 2014 07:41:02 -0400
From:	Neil Horman <nhorman@...driver.com>
To:	Jon Maloy <maloy@...jonn.com>
Cc:	Jon Maloy <jon.maloy@...csson.com>, davem@...emloft.net,
	netdev@...r.kernel.org,
	Paul Gortmaker <paul.gortmaker@...driver.com>,
	erik.hugne@...csson.com, ying.xue@...driver.com,
	tipc-discussion@...ts.sourceforge.net
Subject: Re: [PATCH net-next 01/13] tipc: eliminate case of writing to freed
 memory

On Thu, Jun 26, 2014 at 10:33:04PM -0500, Jon Maloy wrote:
> On 06/26/2014 05:56 AM, Neil Horman wrote:
> > On Wed, Jun 25, 2014 at 08:41:30PM -0500, Jon Maloy wrote:
> >> In the function tipc_nodesub_notify() we call a function pointer
> >> aggregated into the object to be notified, whereafter we set
> >> the function pointer to NULL. However, in some cases the function
> >> pointed to will free the struct containing the function pointer,
> >> resulting in a write to already freed memory.
> >>
> >> This bug seems to always have been there, without causing any
> >> notable harm.
> >>
> >> In this commit we fix the problem by inverting the order of the
> >> zeroing and the function call.
> >>
> >> Signed-off-by: Jon Maloy <jon.maloy@...csson.com>
> >> ---
> >>  net/tipc/node_subscr.c |    6 ++++--
> >>  1 file changed, 4 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/net/tipc/node_subscr.c b/net/tipc/node_subscr.c
> >> index 7c59ab1..2d13eea 100644
> >> --- a/net/tipc/node_subscr.c
> >> +++ b/net/tipc/node_subscr.c
> >> @@ -84,11 +84,13 @@ void tipc_nodesub_unsubscribe(struct tipc_node_subscr *node_sub)
> >>  void tipc_nodesub_notify(struct list_head *nsub_list)
> >>  {
> >>  	struct tipc_node_subscr *ns, *safe;
> >> +	net_ev_handler handle_node_down;
> >>  
> >>  	list_for_each_entry_safe(ns, safe, nsub_list, nodesub_list) {
> >> -		if (ns->handle_node_down) {
> >> -			ns->handle_node_down(ns->usr_handle);
> >> +		handle_node_down = ns->handle_node_down;
> >> +		if (handle_node_down) {
> >>  			ns->handle_node_down = NULL;
> >> +			handle_node_down(ns->usr_handle);
> >>  		}
> >>  	}
> >>  }
> > If its true that some functions pointed to by handle_node_down free the struct
> > pointing to the function pointer, than this change isn't sufficient.  The only
> > caller to tipc_nodesub_notify is node_lost_contact, which dereferences the same
> > structure right after the call to tipc_nodesub_notify.
> 
> In think you misunderstand. The pointer (n_ptr*) passed as parameter in
> node_sub_notify() points to a struct of type tipc_node. This one is *not*
> freed during the call. But it aggregates a linked list of structs of type
> nsub. Those are the ones containing the mentioned function pointer, and
> the ones potentially being being released during the traversal of the list.
> 
> I cannot see that any of these struct are references after the call to
> node_sub_notify().
> 
> ///jon
>  
You're right, I was looking at Linus' tree, not the net-next tree.  In the
former, the struct as a whole, not the list_head is passed in.

Neil

> 
> 
> >
> > Neil
> >
> >> -- 
> >> 1.7.9.5
> >>
> >> --
> >> To unsubscribe from this list: send the line "unsubscribe netdev" in
> >> the body of a message to majordomo@...r.kernel.org
> >> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >>
> 
> 
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html