linux-kernel - Re: [PATCH 2/2] soc: mediatek: devapc: add devapc-mt6779 driver

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] [day] [month] [year] [list]

Message-ID: <1594620698.22730.17.camel@mtkswgap22>
Date:   Mon, 13 Jul 2020 14:11:38 +0800
From:   Neal Liu <neal.liu@...iatek.com>
To:     Matthias Brugger <matthias.bgg@...il.com>
CC:     Neal Liu <neal.liu@...iatek.com>, Rob Herring <robh+dt@...nel.org>,
        <devicetree@...r.kernel.org>,
        <linux-arm-kernel@...ts.infradead.org>,
        lkml <linux-kernel@...r.kernel.org>,
        "moderated list:ARM/Mediatek SoC support" 
        <linux-mediatek@...ts.infradead.org>, <wsd_upstream@...iatek.com>
Subject: Re: [PATCH 2/2] soc: mediatek: devapc: add devapc-mt6779 driver

[snip]
> >>> +/*
> >>> + * devapc_violation_irq - the devapc Interrupt Service Routine (ISR) will dump
> >>> + *			  violation information including which master violates
> >>> + *			  access slave.
> >>> + */
> >>> +static irqreturn_t devapc_violation_irq(int irq_number,
> >>> +					struct mtk_devapc_context *devapc_ctx)
> >>> +{
> >>> +	const struct mtk_device_info **device_info = devapc_ctx->device_info;
> >>> +	int vio_idx = -1;
> >>> +	int index = -1;
> >>> +	int slave_type;
> >>> +
> >>> +	for (slave_type = 0; slave_type < SLAVE_TYPE_NUM; slave_type++) {
> >>> +		if (!mtk_devapc_dump_vio_dbg(devapc_ctx, slave_type, &vio_idx,
> >>> +					     &index))
> >>> +			continue;
> >>> +
> >>> +		/* Ensure that violation info are written before
> >>> +		 * further operations
> >>> +		 */
> >>> +		smp_mb();
> >>> +
> >>> +		mask_module_irq(devapc_ctx, slave_type, vio_idx, true);
> >>> +
> >>> +		clear_vio_status(devapc_ctx, slave_type, vio_idx);
> >>> +
> >>> +		pr_info(PFX "Violation - slave_type:0x%x, sys_index:0x%x, ctrl_index:0x%x, vio_index:0x%x\n",
> >>> +			slave_type,
> >>> +			device_info[slave_type][index].sys_index,
> >>> +			device_info[slave_type][index].ctrl_index,
> >>> +			device_info[slave_type][index].vio_index);
> >>
> >> How will that then be used? Will there some kind of user-space daemon which will
> >> parse the kernel log to see if a violation happens? What will it do with this
> >> information?
> >>
> >> I still don't understand why we need to do that in the kernel instead of in
> >> TF-A. Can you please explain?
> >>
> > 
> > We would do different extra handle for different bus masters internally.
> 
> Does this mean that this is only one part of the whole story? Are you planning 
> to hook into that code internally to implement the handling?
> In that we would need to support the whole thing in upstream. And this sounds 
> like you will need a new subsystem for bus firewall (or how you want to name 
> it). Which allows you to easily add support for other vendors SoCs.
> 

No, the extra handling is implemented by Mediatek proprietary drivers
which has no plan to upstream. And there is no need for first patch of
mtk-devapc driver.
Why do you think it need to support? and why it will need a new
subsystem?

The basic functionality of this driver is that it will handle violation
interrupt, and print violation information logs for further analysis.
Extra handling helps us to analyze violation more easily, but it's not a
basic functionality of it.

> > Basically, different bus masters have different debug mechanism.
> > And different customers have different severity about devapc violation.
> > For example, kernel exception, external exception, warning, don't
> > care,...
> > 
> > I list 2 reason why I put it in kernel instead of ATF.
> > 1. Rich OS such as Linux kernel has more debug mechanism and tools to
> > find murderer.
> 
> But you can access porgram counter from EL3 as well, so you could print all the 
> info you need in TF-A.

For least privilege principle, there is no reason we should print all
the violation information in TF-A.

> 
> > 2. If interrupt has to be handled in ATF, GIC intr would be set as G0S
> > (Group 0 Secure). For our interrupt routing, G0S intr would be fiq. When
> > we handle it in EL3, it would mask all EL1 irq temporarily. We do not
> > treat devapc interrupt as such critical.
> 
> But you said "violation scenario is unexpected" so actually you don't expect it 
> to happen.
> 
> > 
> > Doe it make sense? Or do you have any reason that it should be handled
> > in ATF?
> > 
> 
> My reasoning is that you bring a security mechanism into the kernel, which is in 
> none-secure state. That's a contradiction.

There are two functionality for Mediatek devapc hardware.
1. permission control/protection
2. violation info

You can see that 1. is security mechanism for sure, and we put it in
TF-A.
But 2. is not "security" at all. it's for debugging purpose, we think
it's no any security concern to put it in NS-EL1.

> 
> Regards,
> Matthias
> 
[snip]