linux-kernel - Re: [RFC PATCH] coresight: dynamic-replicator: Fix handling of multiple connections

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: <a7074f44ebbde720b5e0189801eab7c9@codeaurora.org>
Date:   Tue, 07 Apr 2020 20:48:54 +0530
From:   Sai Prakash Ranjan <saiprakash.ranjan@...eaurora.org>
To:     Suzuki K Poulose <suzuki.poulose@....com>
Cc:     mike.leach@...aro.org, mathieu.poirier@...aro.org,
        leo.yan@...aro.org, alexander.shishkin@...ux.intel.com,
        swboyd@...omium.org, linux-arm-kernel@...ts.infradead.org,
        linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org
Subject: Re: [RFC PATCH] coresight: dynamic-replicator: Fix handling of
 multiple connections

Hi Suzuki,

On 2020-04-07 20:23, Suzuki K Poulose wrote:
> On 04/07/2020 02:56 PM, Sai Prakash Ranjan wrote:
>> Hi Suzuki,
>> 
>> On 2020-04-07 18:38, Suzuki K Poulose wrote:
>>> On 04/07/2020 12:29 PM, Sai Prakash Ranjan wrote:
>>>> Hi Suzuki,
>>>> 
>>>> Thanks for looking into this issue.
>>>> 
>>>> On 2020-04-07 15:54, Suzuki K Poulose wrote:
>>>>> On 04/07/2020 10:46 AM, Sai Prakash Ranjan wrote:
>>>>> 
>>>>> There seems to be two replicators back to back here. What is 
>>>>> connected
>>>>> to the other output of both of them ? Are there any TPIUs ? What 
>>>>> happens
>>>>> if you choose a sink on the other end of "swao_replicator" (ETB ?)
>>>>> 
>>>> 
>>>> The other outport of swao replicator is connected to EUD which is a
>>>> QCOM specific HW which can be used as a sink like USB.
>>>> And the other outport of other replicator(replicator_out) is 
>>>> connected to
>>>> TPIU.
>>>> 
>>>>> After boot, what do the idfilter registers read for both the 
>>>>> replicators ?
>>>>> 
>>>> 
>>>> Added some prints in replicator_probe.
>>>> 
>>>>   replicator probe ret=-517 devname=6046000.replicator idfilter0=0x0 
>>>> idfilter1=0x0
>>>>   replicator probe ret=0 devname=6b06000.replicator idfilter0=0xff 
>>>> idfilter1=0xff
>>>>   replicator probe ret=0 devname=6046000.replicator idfilter0=0xff 
>>>> idfilter1=0xff
>>> 
>>> Curious to see how the idfilterX is set to 0:
>>>      if that is never used.
>>>         Or
>>>      if the user doesn't reset it back to 0xff.
>>> 
>> 
>> For both replicators, the default value seems to be 0x0.
>> 
>>   replicator probe in res ret=0 devname=6046000.replicator 
>> idfilter0=0x0 idfilter1=0x0
>>   replicator probe ret=-517 devname=6046000.replicator idfilter0=0x0 
>> idfilter1=0x0
>>   replicator probe in res ret=0 devname=6b06000.replicator 
>> idfilter0=0x0 idfilter1=0x0
>>   replicator probe ret=0 devname=6b06000.replicator idfilter0=0xff 
>> idfilter1=0xff
>>   replicator probe in res ret=0 devname=6046000.replicator 
>> idfilter0=0x0 idfilter1=0x0
>>   replicator probe ret=0 devname=6046000.replicator idfilter0=0xff 
>> idfilter1=0xff
> 
> I am not sure how you have added the debugs, but it looks like the
> drivers set 0xff for both the port filters on a successful probe.
> 

Yes, thats done by replicator_reset in probe right? Below is the diff:

@@ -242,6 +244,9 @@ static int replicator_probe(struct device *dev, 
struct resource *res)
                 }
                 drvdata->base = base;
                 desc.groups = replicator_groups;
+               pr_info("replicator probe in res ret=%d devname=%s 
idfilter0=%#lx idfilter1=%#lx\n",
+                       ret, dev_name(dev), (readl_relaxed(base + 
REPLICATOR_IDFILTER0)),
+                       (readl_relaxed(base + REPLICATOR_IDFILTER1)));
         }

         dev_set_drvdata(dev, drvdata);
@@ -272,6 +277,12 @@ static int replicator_probe(struct device *dev, 
struct resource *res)
  out_disable_clk:
         if (ret && !IS_ERR_OR_NULL(drvdata->atclk))
                 clk_disable_unprepare(drvdata->atclk);
+
+       if (res)
+               pr_info("replicator probe ret=%d devname=%s 
idfilter0=%#lx idfilter1=%#lx\n",
+                       ret, dev_name(dev), (readl_relaxed(base + 
REPLICATOR_IDFILTER0)),
+                       (readl_relaxed(base + REPLICATOR_IDFILTER1)));
+
         return ret;
  }

>> 
>>> Does your test ever touch EUD (enable the port for EUD at
>>> swao-replicator) ? What are the values before you run your test ?
>>> 
>>> 
>> 
>> No, we do not use EUD, downstream it is used as dummy sink.
>> And I just try to select the ETR as the sink and enable ETM0 as the 
>> trace source.
>> 
>> echo 1 > /sys/bus/coresight/devices/tmc_etr0/enable_sink
>> echo 1 > /sys/bus/coresight/devices/etm0/enable_source
>> 
>> Also I see the KASAN warning but that seems like some other issue.
>> 
> 
> Does your funnel have sparse input described ? I think we have an
> issue with the "refcnt" tracking for funnels (especially). When we
> have a sparse input ports described (ie. if only input ports 0, 3,
> 5 are described to protect the secure side connections), we could
> end up accessing beyond the memory allocated for csdev->refcnts.
> i.e, csdev->pdata->nr_inport = 3, and we could access 
> csdev->refcnts[5],
> while sizeof(csdev->refcnts) = sizeof(atomic_t) * 3.
> 
> I will send a patch.
> 

Thanks, I can test it out.

Thanks,
Sai

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a 
member
of Code Aurora Forum, hosted by The Linux Foundation