linux-kernel - Re: [PATCH] mmc: core: Don't allocate IDA for OF aliases

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]

Message-ID: <161851496169.46595.399410018266490859@swboyd.mtv.corp.google.com>
Date:   Thu, 15 Apr 2021 12:29:21 -0700
From:   Stephen Boyd <swboyd@...omium.org>
To:     Ulf Hansson <ulf.hansson@...aro.org>
Cc:     Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-mmc <linux-mmc@...r.kernel.org>,
        Matthias Schiffer <matthias.schiffer@...tq-group.com>,
        Sujit Kautkar <sujitka@...omium.org>,
        Zubin Mithra <zsm@...omium.org>
Subject: Re: [PATCH] mmc: core: Don't allocate IDA for OF aliases

Quoting Ulf Hansson (2021-04-15 01:56:12)
> On Tue, 13 Apr 2021 at 02:36, Stephen Boyd <swboyd@...omium.org> wrote:
> >
> > -       err = ida_simple_get(&mmc_host_ida, min_idx, max_idx, GFP_KERNEL);
> > -       if (err < 0) {
> > -               kfree(host);
> > -               return NULL;
> > +               index = ida_simple_get(&mmc_host_ida, min_idx, max_idx, GFP_KERNEL);
> > +               if (index < 0) {
> > +                       kfree(host);
> > +                       return NULL;
> > +               }
> 
> This means that a DTB that is screwed up in a way that it has two mmc
> aliases with the same index, would be allowed to use the same index.
> 
> What will happen when we continue the probe sequence in such a case?

Yeah I thought about this after sending the patch. So the problem would
be like this right?

	aliases {
		mmc1 = &sdhci0;
		mmc1 = &sdhci1;
	};

I have good news! DT won't compile it because it saw the same alias
assigned to twice. I tried it on my sc7180 board. 

arch/arm64/boot/dts/qcom/sc7180.dtsi:35.3-18:
ERROR (duplicate_property_names): /aliases:mmc1: Duplicate property name
ERROR: Input tree has errors, aborting (use -f to force output)
arch/arm64/boot/dts/qcom/sc7180-idp.dtb] Error 2

I suppose if someone forced the compilation it may be bad, but do we
really care?

TL;DR: this seems like it isn't a problem.

> 
> >         }
> >
> > -       host->index = err;
> > +       host->index = index;
> >
> >         dev_set_name(&host->class_dev, "mmc%d", host->index);
> >         host->ws = wakeup_source_register(NULL, dev_name(&host->class_dev));
> 
> Another concern that could potentially be a problem, is that the
> "thread" that holds the reference that prevents ida from being
> removed, how would that react to a new mmc device to become
> re-registered with the same index?
> 
> I wonder if we perhaps should return -EPROBE_DEFER instead, when
> ida_simple_get() fails?
> 

Don't think so. The device (with the kobject inside) is removed, and
thus the mmc1 device will be removed, but the kobject's release function
is delayed due to the config. This means that
mmc_host_classdev_release() is called at a later time. The only thing
inside that function is the IDA removal and the kfree of the container
object. Given that nothing else is in that release function I believe it
is safe to skip IDA allocation as it won't be blocking anything in the
reserved alias case. 

Furthermore, when the device is deleted in mmc_remove_host() there could
be other users of the device that haven't called put_device() yet.
Either way, those other users are keeping the device memory alive, but
otherwise device_del() has unlinked it from the various driver core
lists and sysfs has removed it too so it's in a state where code may be
referencing it but it's on the way out so users of the device will not
be able to do much with it during this time.

This sort of problem (if it exists which I don't think it does) would
have been there all along and can't be fixed at this level. When a
device that has an alias calls the mmc_alloc_host() function twice it
gets two different device structures created so there are two distinct
kobjects that will need to be released at some point. The index is
usually different for those two kobjects, but with aliases it turns out
it is the same. When it comes to registering that device with the same
name the second one will fail because a device with that name already
exists on the bus. This would be really hard to do given that it would
need to be the same aliased device in DT calling the mmc_add_host()
function without calling mmc_remove_host() for the first one it added in
between.

(Sorry if that is long. I'm sort of stream of conciousness writing it to
you here and not rewriting it to be more concise)