From patchwork Thu May 27 04:19:40 2010 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: David Miller X-Patchwork-Id: 53678 X-Patchwork-Delegate: davem@davemloft.net Return-Path: X-Original-To: patchwork-incoming@ozlabs.org Delivered-To: patchwork-incoming@ozlabs.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by ozlabs.org (Postfix) with ESMTP id 387BAB7D29 for ; Thu, 27 May 2010 14:19:32 +1000 (EST) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751253Ab0E0ETb (ORCPT ); Thu, 27 May 2010 00:19:31 -0400 Received: from 74-93-104-97-Washington.hfc.comcastbusiness.net ([74.93.104.97]:40929 "EHLO sunset.davemloft.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751237Ab0E0ETa (ORCPT ); Thu, 27 May 2010 00:19:30 -0400 Received: from localhost (localhost [127.0.0.1]) by sunset.davemloft.net (Postfix) with ESMTP id D35C424C08E; Wed, 26 May 2010 21:19:40 -0700 (PDT) Date: Wed, 26 May 2010 21:19:40 -0700 (PDT) Message-Id: <20100526.211940.200373719.davem@davemloft.net> To: julia@diku.dk Cc: sparclinux@vger.kernel.org Subject: Re: question about drivers/serial/sunsu.c From: David Miller In-Reply-To: References: X-Mailer: Mew version 6.3 on Emacs 23.1 / Mule 6.0 (HANACHIRUSATO) Mime-Version: 1.0 Sender: sparclinux-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: sparclinux@vger.kernel.org From: Julia Lawall Date: Sat, 15 May 2010 10:15:17 +0200 (CEST) > The following function in drivers/serial/sunsu.c contains a possible use > after free: ... > Is it safe to simply move the if (up->port.membase) of_iounmap(...); up > above the other ifs, or should something else be done? No, that isn't safe. We have to unregister the device from the SERIO layer before we do that. Practically speaking, these remove methods never really ever execute on sparc64 since 1) sunsu is usually built statically so we can get a console and 2) hotplug of these devices is basically a non-occurance as well. Anyways, I'm going to fix this bug as follows, thanks for spotting it! -------------------- sunsu: Fix use after free in su_remove(). Real serial port 'up' objects are statically allocated from an array in the driver. Keyboard and mouse ports, on the other hand, are dynamically allocated. Unfortunately, we free these dynamic 'up' objects before we unmap the I/O registers. Rearrange su_remove() so that this does not happen. Noticed by Julia Lawall. Signed-off-by: David S. Miller --- drivers/serial/sunsu.c | 13 +++++++++---- 1 files changed, 9 insertions(+), 4 deletions(-) diff --git a/drivers/serial/sunsu.c b/drivers/serial/sunsu.c index 234459c..ffbf455 100644 --- a/drivers/serial/sunsu.c +++ b/drivers/serial/sunsu.c @@ -1500,20 +1500,25 @@ out_unmap: static int __devexit su_remove(struct of_device *op) { struct uart_sunsu_port *up = dev_get_drvdata(&op->dev); + bool kbdms = false; if (up->su_type == SU_PORT_MS || - up->su_type == SU_PORT_KBD) { + up->su_type == SU_PORT_KBD) + kbdms = true; + + if (kbdms) { #ifdef CONFIG_SERIO serio_unregister_port(&up->serio); #endif - kfree(up); - } else if (up->port.type != PORT_UNKNOWN) { + } else if (up->port.type != PORT_UNKNOWN) uart_remove_one_port(&sunsu_reg, &up->port); - } if (up->port.membase) of_iounmap(&op->resource[0], up->port.membase, up->reg_size); + if (kbdms) + kfree(up); + dev_set_drvdata(&op->dev, NULL); return 0;