linux-kernel - Re: [PATCH v3 2/6] irqchip: Add interrupt controller support for Realtek DHC SoCs

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, 8 Dec 2023 11:43:25 +0300
From:   Dan Carpenter <dan.carpenter@...aro.org>
To:     James Tai [戴志峰] <james.tai@...ltek.com>
Cc:     Thomas Gleixner <tglx@...utronix.de>,
        Marc Zyngier <maz@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Conor Dooley <conor+dt@...nel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        kernel test robot <lkp@...el.com>,
        Dan Carpenter <error27@...il.com>
Subject: Re: [PATCH v3 2/6] irqchip: Add interrupt controller support for
 Realtek DHC SoCs

On Fri, Dec 08, 2023 at 08:21:10AM +0000, James Tai [戴志峰] wrote:
> Hi Dan,
> 
> >> devm_ allocations are cleaned up automatically so there is no need to
> >> call devm_kfree() before returning.
> >>
> >> regards,
> >> dan carpenter
> >
> I will remove it. 
> 
> >> > +   }
> >> > +
> >> > +   data->info = info;
> >> > +
> >> > +   raw_spin_lock_init(&data->lock);
> >> > +
> >> > +   data->domain = irq_domain_add_linear(node, 32,
> >> > + &realtek_intc_domain_ops, data);
> >
> >Btw, as I was testing the other static checker warning for <= 0, my static
> >checker really wants this irq_domain_add_linear() to be cleaned up on the error
> >path.
> >
> >Otherwise it probably leads to a use after free because we free data
> >(automatically or manually) but it's still on a list somewhere.
> >
> I will add 'irq_domain_remove()' to release it. 
> 
> >> > +   if (!data->domain) {
> >> > +           ret = -ENOMEM;
> >> > +           goto out_cleanup;
> >> > +   }
> >> > +
> >> > +   data->subset_data_num = info->cfg_num;
> >> > +   for (i = 0; i < info->cfg_num; i++) {
> >> > +           ret = realtek_intc_subset(node, data, i);
> >> > +           if (ret) {
> >> > +                   WARN(ret, "failed to init subset %d: %d", i, ret);
> >> > +                   ret = -ENOMEM;
> >> > +                   goto out_cleanup;
> >
> >This error path.
> >
> >regards,
> >dan carpenter
> >
> I will add 'irq_domain_remove()' before goto cleanup.
> 
> 	for (i = 0; i < info->cfg_num; i++) {
> 		ret = realtek_intc_subset(node, data, i);
> 		if (ret) {
> 			WARN(ret, "failed to init subset %d: %d", i, ret);
> 			irq_domain_remove(data->domain);
> 			ret = -ENOMEM;
> 			goto out_cleanup;
> 		}
> 	}
> 
> Thank you for your feedback.

You're running into the issue because you're using One Err Label style
error handling.  It would be better to use normal unwind laddering.
See my blog for more info:

https://staticthinking.wordpress.com/2022/04/28/free-the-last-thing-style/

regards,
dan carpenter