| 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
| ||
|
Message-ID: <CAJ9a7VjAnyoz+5y8xvYDPr8Mg3Hp-kTAOs4c_iwj+fqiiVs10w@mail.gmail.com>
Date: Wed, 17 Jun 2020 10:20:15 +0100
From: Mike Leach <mike.leach@...aro.org>
To: Dan Carpenter <dan.carpenter@...cle.com>
Cc: kbuild@...ts.01.org, lkp@...el.com, kbuild-all@...ts.01.org,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Mathieu Poirier <mathieu.poirier@...aro.org>
Subject: Re: [kbuild] drivers/hwtracing/coresight/coresight-cti.c:862
cti_probe() error: we previously assumed 'drvdata' could be null (see line 759)
Hi Dan,
Thanks for the comprehensive review of this.
On Fri, 12 Jun 2020 at 14:47, Dan Carpenter <dan.carpenter@...cle.com> wrote:
>
>
> Hi Mike,
>
> Here is the buggy line:
>
> 861 err_out:
> 862 cti_pm_release(drvdata);
> ^^^^^^^
>
Yes - caling cti_pm_release() if drvdata == NULL would result in bad
things happening!
I would certainly be aiming to fix this, but as you have provided a
patch on another mail - I have reviewed and compiled that, so will
respond further there
Regards
Mike
> To me it's a red flag any time there is a label called "out:". The
> label should say what is being released like "goto unregister_notifier;".
> The style of error handling here is called a "free everything function"
> and it is the most error prone style of error handling.
>
> A better way to write error handling is to track the most recently
> allocated resource and free it with a well named goto.
>
> a = alloc();
> if (!a)
> return -ENOMEM;
>
> b = alloc();
> if (!b) {
> ret = -ENOMEM;
> goto free_a;
> }
>
> c = alloc();
> if (!c) {
> ret = -ENOMEM;
> goto free_b;
>
> ...
> return 0;
>
> free_b:
> free(b);
> free_a:
> free(a);
>
> The advantage of this is that 1) You only have to track the most recent
> allocation. 2) You can easily verify that the most recent allocation
> is freed. 3) Now you can create a free function by copy and pasting
> and adding a free(c);
>
> void my_free(struct whatever *p)
> {
> free(c);
> free(b);
> free(a);
> }
>
> This style uses about the same number of lines of code because although
> we duplicate the free(b) and free(a), we can remove some if statements
> so it ends up being about the same.
>
> The main problem with free everything function is that they free things
> which have not been allocated. I have added more explanation at the
> bottom of this bug report.
>
> I am also sending a patch which hopefully is clear but I can't actually
> compile it. :(
>
> tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
> head: b791d1bdf9212d944d749a5c7ff6febdba241771
> commit: e9b880581d555c8f7b58c7d19cc3f8f9016a1b5f coresight: cti: Add CPU Hotplug handling to CTI driver
> config: arm-randconfig-m031-20200612 (attached as .config)
> compiler: arm-linux-gnueabi-gcc (GCC) 9.3.0
>
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@...el.com>
> Reported-by: Dan Carpenter <dan.carpenter@...cle.com>
>
> smatch warnings:
> drivers/hwtracing/coresight/coresight-cti.c:862 cti_probe() error: we previously assumed 'drvdata' could be null (see line 759)
>
> # https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=e9b880581d555c8f7b58c7d19cc3f8f9016a1b5f
> git remote add linus https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
> git remote update linus
> git checkout e9b880581d555c8f7b58c7d19cc3f8f9016a1b5f
> vim +/drvdata +862 drivers/hwtracing/coresight/coresight-cti.c
>
> 835d722ba10ac92 Mike Leach 2020-03-20 747 static int cti_probe(struct amba_device *adev, const struct amba_id *id)
> 835d722ba10ac92 Mike Leach 2020-03-20 748 {
> 835d722ba10ac92 Mike Leach 2020-03-20 749 int ret = 0;
> 835d722ba10ac92 Mike Leach 2020-03-20 750 void __iomem *base;
> 835d722ba10ac92 Mike Leach 2020-03-20 751 struct device *dev = &adev->dev;
> 835d722ba10ac92 Mike Leach 2020-03-20 752 struct cti_drvdata *drvdata = NULL;
> 835d722ba10ac92 Mike Leach 2020-03-20 753 struct coresight_desc cti_desc;
> 835d722ba10ac92 Mike Leach 2020-03-20 754 struct coresight_platform_data *pdata = NULL;
> 835d722ba10ac92 Mike Leach 2020-03-20 755 struct resource *res = &adev->res;
> 835d722ba10ac92 Mike Leach 2020-03-20 756
> 835d722ba10ac92 Mike Leach 2020-03-20 757 /* driver data*/
> 835d722ba10ac92 Mike Leach 2020-03-20 758 drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
> 835d722ba10ac92 Mike Leach 2020-03-20 @759 if (!drvdata) {
> 835d722ba10ac92 Mike Leach 2020-03-20 760 ret = -ENOMEM;
> 835d722ba10ac92 Mike Leach 2020-03-20 761 dev_info(dev, "%s, mem err\n", __func__);
>
> No need to print an error message for kmalloc() failures. It already
> has a stack trace built in.
>
> 835d722ba10ac92 Mike Leach 2020-03-20 762 goto err_out;
> ^^^^^^^^^^^^
>
> 835d722ba10ac92 Mike Leach 2020-03-20 763 }
> 835d722ba10ac92 Mike Leach 2020-03-20 764
> 835d722ba10ac92 Mike Leach 2020-03-20 765 /* Validity for the resource is already checked by the AMBA core */
> 835d722ba10ac92 Mike Leach 2020-03-20 766 base = devm_ioremap_resource(dev, res);
> 835d722ba10ac92 Mike Leach 2020-03-20 767 if (IS_ERR(base)) {
> 835d722ba10ac92 Mike Leach 2020-03-20 768 ret = PTR_ERR(base);
> 835d722ba10ac92 Mike Leach 2020-03-20 769 dev_err(dev, "%s, remap err\n", __func__);
>
> At this point "drvdata->ctidev.cpu" is zero.
>
> 835d722ba10ac92 Mike Leach 2020-03-20 770 goto err_out;
> 835d722ba10ac92 Mike Leach 2020-03-20 771 }
> 835d722ba10ac92 Mike Leach 2020-03-20 772 drvdata->base = base;
> 835d722ba10ac92 Mike Leach 2020-03-20 773
> 835d722ba10ac92 Mike Leach 2020-03-20 774 dev_set_drvdata(dev, drvdata);
> 835d722ba10ac92 Mike Leach 2020-03-20 775
> 835d722ba10ac92 Mike Leach 2020-03-20 776 /* default CTI device info */
> 835d722ba10ac92 Mike Leach 2020-03-20 777 drvdata->ctidev.cpu = -1;
> ^^^^^^^^^^^^^^^^^^^^^^^^
>
> 835d722ba10ac92 Mike Leach 2020-03-20 778 drvdata->ctidev.nr_trig_con = 0;
> 835d722ba10ac92 Mike Leach 2020-03-20 779 drvdata->ctidev.ctm_id = 0;
> 835d722ba10ac92 Mike Leach 2020-03-20 780 INIT_LIST_HEAD(&drvdata->ctidev.trig_cons);
> 835d722ba10ac92 Mike Leach 2020-03-20 781
> 835d722ba10ac92 Mike Leach 2020-03-20 782 spin_lock_init(&drvdata->spinlock);
> 835d722ba10ac92 Mike Leach 2020-03-20 783
> 835d722ba10ac92 Mike Leach 2020-03-20 784 /* initialise CTI driver config values */
> 835d722ba10ac92 Mike Leach 2020-03-20 785 cti_set_default_config(dev, drvdata);
> 835d722ba10ac92 Mike Leach 2020-03-20 786
> 835d722ba10ac92 Mike Leach 2020-03-20 787 pdata = coresight_cti_get_platform_data(dev);
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> This function sets drvdata->ctidev.cpu on some success paths and also
> on certain failure paths.
>
> 835d722ba10ac92 Mike Leach 2020-03-20 788 if (IS_ERR(pdata)) {
> 835d722ba10ac92 Mike Leach 2020-03-20 789 dev_err(dev, "coresight_cti_get_platform_data err\n");
> 835d722ba10ac92 Mike Leach 2020-03-20 790 ret = PTR_ERR(pdata);
> 835d722ba10ac92 Mike Leach 2020-03-20 791 goto err_out;
> 835d722ba10ac92 Mike Leach 2020-03-20 792 }
> 835d722ba10ac92 Mike Leach 2020-03-20 793
> 835d722ba10ac92 Mike Leach 2020-03-20 794 /* default to powered - could change on PM notifications */
> 835d722ba10ac92 Mike Leach 2020-03-20 795 drvdata->config.hw_powered = true;
> 835d722ba10ac92 Mike Leach 2020-03-20 796
> 835d722ba10ac92 Mike Leach 2020-03-20 797 /* set up device name - will depend if cpu bound or otherwise */
> 835d722ba10ac92 Mike Leach 2020-03-20 798 if (drvdata->ctidev.cpu >= 0)
> 835d722ba10ac92 Mike Leach 2020-03-20 799 cti_desc.name = devm_kasprintf(dev, GFP_KERNEL, "cti_cpu%d",
> 835d722ba10ac92 Mike Leach 2020-03-20 800 drvdata->ctidev.cpu);
> 835d722ba10ac92 Mike Leach 2020-03-20 801 else
> 835d722ba10ac92 Mike Leach 2020-03-20 802 cti_desc.name = coresight_alloc_device_name(&cti_sys_devs, dev);
> 835d722ba10ac92 Mike Leach 2020-03-20 803 if (!cti_desc.name) {
> 835d722ba10ac92 Mike Leach 2020-03-20 804 ret = -ENOMEM;
> 835d722ba10ac92 Mike Leach 2020-03-20 805 goto err_out;
> 835d722ba10ac92 Mike Leach 2020-03-20 806 }
> 835d722ba10ac92 Mike Leach 2020-03-20 807
> e9b880581d555c8 Mike Leach 2020-05-18 808 /* setup CPU power management handling for CPU bound CTI devices. */
> e9b880581d555c8 Mike Leach 2020-05-18 809 if (drvdata->ctidev.cpu >= 0) {
> e9b880581d555c8 Mike Leach 2020-05-18 810 cti_cpu_drvdata[drvdata->ctidev.cpu] = drvdata;
> e9b880581d555c8 Mike Leach 2020-05-18 811 if (!nr_cti_cpu++) {
> ^^^^^^^^^^^^
>
> e9b880581d555c8 Mike Leach 2020-05-18 812 cpus_read_lock();
> e9b880581d555c8 Mike Leach 2020-05-18 813 ret = cpuhp_setup_state_nocalls_cpuslocked(
> e9b880581d555c8 Mike Leach 2020-05-18 814 CPUHP_AP_ARM_CORESIGHT_CTI_STARTING,
> e9b880581d555c8 Mike Leach 2020-05-18 815 "arm/coresight_cti:starting",
> e9b880581d555c8 Mike Leach 2020-05-18 816 cti_starting_cpu, cti_dying_cpu);
> e9b880581d555c8 Mike Leach 2020-05-18 817
> e9b880581d555c8 Mike Leach 2020-05-18 818 cpus_read_unlock();
> e9b880581d555c8 Mike Leach 2020-05-18 819 if (ret)
> e9b880581d555c8 Mike Leach 2020-05-18 820 goto err_out;
> e9b880581d555c8 Mike Leach 2020-05-18 821 }
> e9b880581d555c8 Mike Leach 2020-05-18 822 }
> e9b880581d555c8 Mike Leach 2020-05-18 823
> 3c5597e398124e6 Mike Leach 2020-03-20 824 /* create dynamic attributes for connections */
> 3c5597e398124e6 Mike Leach 2020-03-20 825 ret = cti_create_cons_sysfs(dev, drvdata);
> 3c5597e398124e6 Mike Leach 2020-03-20 826 if (ret) {
> 3c5597e398124e6 Mike Leach 2020-03-20 827 dev_err(dev, "%s: create dynamic sysfs entries failed\n",
> 3c5597e398124e6 Mike Leach 2020-03-20 828 cti_desc.name);
> 3c5597e398124e6 Mike Leach 2020-03-20 829 goto err_out;
> 3c5597e398124e6 Mike Leach 2020-03-20 830 }
> 3c5597e398124e6 Mike Leach 2020-03-20 831
> 835d722ba10ac92 Mike Leach 2020-03-20 832 /* set up coresight component description */
> 835d722ba10ac92 Mike Leach 2020-03-20 833 cti_desc.pdata = pdata;
> 835d722ba10ac92 Mike Leach 2020-03-20 834 cti_desc.type = CORESIGHT_DEV_TYPE_ECT;
> 835d722ba10ac92 Mike Leach 2020-03-20 835 cti_desc.subtype.ect_subtype = CORESIGHT_DEV_SUBTYPE_ECT_CTI;
> 835d722ba10ac92 Mike Leach 2020-03-20 836 cti_desc.ops = &cti_ops;
> 3c5597e398124e6 Mike Leach 2020-03-20 837 cti_desc.groups = drvdata->ctidev.con_groups;
> 835d722ba10ac92 Mike Leach 2020-03-20 838 cti_desc.dev = dev;
> 835d722ba10ac92 Mike Leach 2020-03-20 839 drvdata->csdev = coresight_register(&cti_desc);
> 835d722ba10ac92 Mike Leach 2020-03-20 840 if (IS_ERR(drvdata->csdev)) {
> 835d722ba10ac92 Mike Leach 2020-03-20 841 ret = PTR_ERR(drvdata->csdev);
> 835d722ba10ac92 Mike Leach 2020-03-20 842 goto err_out;
> 835d722ba10ac92 Mike Leach 2020-03-20 843 }
> 835d722ba10ac92 Mike Leach 2020-03-20 844
> 835d722ba10ac92 Mike Leach 2020-03-20 845 /* add to list of CTI devices */
> 835d722ba10ac92 Mike Leach 2020-03-20 846 mutex_lock(&ect_mutex);
> 835d722ba10ac92 Mike Leach 2020-03-20 847 list_add(&drvdata->node, &ect_net);
> 177af8285b59a38 Mike Leach 2020-03-20 848 /* set any cross references */
> 177af8285b59a38 Mike Leach 2020-03-20 849 cti_update_conn_xrefs(drvdata);
> 835d722ba10ac92 Mike Leach 2020-03-20 850 mutex_unlock(&ect_mutex);
> 835d722ba10ac92 Mike Leach 2020-03-20 851
> 835d722ba10ac92 Mike Leach 2020-03-20 852 /* set up release chain */
> 835d722ba10ac92 Mike Leach 2020-03-20 853 drvdata->csdev_release = drvdata->csdev->dev.release;
> 835d722ba10ac92 Mike Leach 2020-03-20 854 drvdata->csdev->dev.release = cti_device_release;
> 835d722ba10ac92 Mike Leach 2020-03-20 855
> 835d722ba10ac92 Mike Leach 2020-03-20 856 /* all done - dec pm refcount */
> 835d722ba10ac92 Mike Leach 2020-03-20 857 pm_runtime_put(&adev->dev);
> 835d722ba10ac92 Mike Leach 2020-03-20 858 dev_info(&drvdata->csdev->dev, "CTI initialized\n");
> 835d722ba10ac92 Mike Leach 2020-03-20 859 return 0;
> 835d722ba10ac92 Mike Leach 2020-03-20 860
> 835d722ba10ac92 Mike Leach 2020-03-20 861 err_out:
> e9b880581d555c8 Mike Leach 2020-05-18 @862 cti_pm_release(drvdata);
> ^^^^^^^
> 835d722ba10ac92 Mike Leach 2020-03-20 863 return ret;
> 835d722ba10ac92 Mike Leach 2020-03-20 864 }
>
>
> 750 /* release PM registrations */
> 751 static void cti_pm_release(struct cti_drvdata *drvdata)
> 752 {
> 753 if (drvdata->ctidev.cpu >= 0) {
> ^^^^^^^
> We are dereferencing this when it wasn't allocated.
>
> 754 if (--nr_cti_cpu == 0) {
> ^^^^^^^^^^^^
> If devm_kasprintf() fails then we are decrementing this when it wasn't
> incremented so now it can be negative.
>
> 755 cpu_pm_unregister_notifier(&cti_cpu_pm_nb);
>
> If the cpu_pm_register_notifier() fails then we are unregistering this
> when it wasn't registered. It turns out this is harmless but if we
> only free things which have been allocated then it becomes a lot
> easier to audit the code.
>
> 756
> 757 cpuhp_remove_state_nocalls(
> 758 CPUHP_AP_ARM_CORESIGHT_CTI_STARTING);
>
> If cpuhp_setup_state_nocalls_cpuslocked() failed then this wasn't
> allocated. I believe this is harmless.
>
> 759 }
> 760 cti_cpu_drvdata[drvdata->ctidev.cpu] = NULL;
> 761 }
> 762 }
>
> ---
> 0-DAY CI Kernel Test Service, Intel Corporation
> https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
> _______________________________________________
> kbuild mailing list -- kbuild@...ts.01.org
> To unsubscribe send an email to kbuild-leave@...ts.01.org
--
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK
Powered by blists - more mailing lists