From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752539AbdKHUAt (ORCPT ); Wed, 8 Nov 2017 15:00:49 -0500 Received: from mail-ot0-f195.google.com ([74.125.82.195]:49458 "EHLO mail-ot0-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752436AbdKHUAs (ORCPT ); Wed, 8 Nov 2017 15:00:48 -0500 X-Google-Smtp-Source: AGs4zMY+kygpbkl2lbtqrLzj35fTL+Jo3au/2+L9c0OLqGZpXBkEGyCuDqx20sHNQw5Ryxqx+obYvw== Reply-To: minyard@acm.org Subject: Re: [PATCH] drivers/char/ipmi_si: prevent null deref during module exit To: Andrew Banman Cc: Dimitri Sivanich , "Anderson, Russ" , Mike Travis , openipmi-developer@lists.sourceforge.net, linux-kernel@vger.kernel.org References: <988aec06-2127-3120-47a4-5d0131f59a61@hpe.com> From: Corey Minyard Message-ID: <09f6a9da-5bee-4a78-9d0f-5ac8336bd394@acm.org> Date: Wed, 8 Nov 2017 14:00:44 -0600 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Content-Language: en-GB Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 11/08/2017 11:11 AM, Andrew Banman wrote: > On 11/8/17 11:06 AM, Andrew Banman wrote: >> If there are uninitialized SMIs in the smi_infos list, i.e. with no >> handlers set, then disable_si_irq() in cleanup_smi_one() will hit a null >> pointer dereference when the former attempts to start the check enables >> transaction. Thus, we panic during module exit. > > I think this points to a broader problem of holding uninitialized > smi_info > structs in smi_infos list. There are many places where handlers and > other struct > members are assumed. Maybe a better design would be to remove SMIs > from the list > if we have no intention of initializing them? > This begs the question: How did you produce this?  From what I can tell, there is no way you can get to this code if you don't have a working and initialized smi_info structure, and that's not the only place this would have to be fixed if it wasn't.  So it's not what you assume, I don't think it's an uninitialized smi_info structure on the list. As usual with these sorts of things, please send tracebacks and reproduction procedures. If you are removing the module and this happens, there may be a race conditions, but this is the wrong fix.  More likely that the structure gets cleaned up and this function is called afterwards. -corey > Andrew > >> >> Avoid panicking when there are uninitialized SMIs by checking for a >> handler >> pointer before starting the check enables transaction. >> >> Signed-off-by: Andrew Banman >> --- >>   drivers/char/ipmi/ipmi_si_intf.c | 2 +- >>   1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/char/ipmi/ipmi_si_intf.c >> b/drivers/char/ipmi/ipmi_si_intf.c >> index cb5719e..6c0b1b3 100644 >> --- a/drivers/char/ipmi/ipmi_si_intf.c >> +++ b/drivers/char/ipmi/ipmi_si_intf.c >> @@ -442,7 +442,7 @@ static void start_check_enables(struct smi_info >> *smi_info, bool start_timer) >> >>       if (start_timer) >>           start_new_msg(smi_info, msg, 2); >> -    else >> +    else if (smi_info->handlers) >> smi_info->handlers->start_transaction(smi_info->si_sm, msg, 2); >>       smi_info->si_state = SI_CHECKING_ENABLES; >>   } >