linux-kernel - Re: [PATCH v2 2/2] soc: mediatek: add mtk-devapc 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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAAOTY_9MPYi=FAisE50UzT=eceSykN+Z8HnfFLLg_uRDhPAkpg@mail.gmail.com>
Date:   Fri, 10 Jul 2020 22:21:08 +0800
From:   Chun-Kuang Hu <chunkuang.hu@...nel.org>
To:     Neal Liu <neal.liu@...iatek.com>
Cc:     Chun-Kuang Hu <chunkuang.hu@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Matthias Brugger <matthias.bgg@...il.com>,
        devicetree@...r.kernel.org,
        wsd_upstream <wsd_upstream@...iatek.com>,
        lkml <linux-kernel@...r.kernel.org>,
        "moderated list:ARM/Mediatek SoC support" 
        <linux-mediatek@...ts.infradead.org>,
        Linux ARM <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH v2 2/2] soc: mediatek: add mtk-devapc driver

Hi, Neal:

Neal Liu <neal.liu@...iatek.com> 於 2020年7月10日 週五 上午11:23寫道：
>
> Hi Chun-Kuang,
>
> Thanks for your review.
>
> On Thu, 2020-07-09 at 21:01 +0800, Chun-Kuang Hu wrote:
> > Hi, Neal:
> >
> > Neal Liu <neal.liu@...iatek.com> 於 2020年7月9日 週四 下午5:13寫道：
> > >
> > > MediaTek bus fabric provides TrustZone security support and data
> > > protection to prevent slaves from being accessed by unexpected
> > > masters.
> > > The security violation is logged and sent to the processor for
> > > further analysis or countermeasures.
> > >
> > > Any occurrence of security violation would raise an interrupt, and
> > > it will be handled by mtk-devapc driver. The violation
> > > information is printed in order to find the murderer.
> > >
> > > Signed-off-by: Neal Liu <neal.liu@...iatek.com>
> >
> > [snip]
> >
> > > +
> > > +static u32 get_shift_group(struct mtk_devapc_context *devapc_ctx,
> > > +                          int slave_type, int vio_idx)
> >
> > vio_idx  is useless, so remove it.
> >
>
> yes, my mistake. I'll remove it on next patch.
>
> > > +{
> > > +       u32 vio_shift_sta;
> > > +       void __iomem *reg;
> > > +       int bit;
> > > +
> > > +       reg = mtk_devapc_pd_get(devapc_ctx, slave_type, VIO_SHIFT_STA, 0);
> > > +       vio_shift_sta = readl(reg);
> > > +
> > > +       for (bit = 0; bit < 32; bit++) {
> > > +               if ((vio_shift_sta >> bit) & 0x1)
> > > +                       break;
> > > +       }
> > > +
> > > +       return bit;
> > > +}
> > > +
> >
> > [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;
> > > +       int slave_type_num;
> > > +       int vio_idx = -1;
> > > +       int slave_type;
> > > +
> > > +       slave_type_num = devapc_ctx->slave_type_num;
> > > +       device_info = devapc_ctx->device_info;
> > > +
> > > +       for (slave_type = 0; slave_type < slave_type_num; slave_type++) {
> >
> > If slave_type_num is 1, I think the code should be simpler.
>
> slave_type_num is depends on DT data, it's not always 1.

Please change commit title to "add mt6779 mtk-devapc driver". This
patch is just for mt6779. If slave_type_num = 1 in mt6779, there is
only one slave and we don't need a slave_type variable. Add
slave_type_num in the patch of adding one SoC which has multiple
slaves.

>
> >
> > > +               if (!mtk_devapc_dump_vio_dbg(devapc_ctx, slave_type, &vio_idx))
> > > +                       continue;
> > > +
> > > +               /* Ensure that violation info are written before
> > > +                * further operations
> > > +                */
> > > +               smp_mb();
> > > +
> > > +               mask_module_irq(devapc_ctx, slave_type, vio_idx, true);
> >
> > Why do you mask irq?
>
> It has to mask slave's irq before clear violation status.
> It's one of hardware design.

If don't do this before clear_vio_status, what would happen? The clear
would fail?

>
> >
> > > +
> > > +               clear_vio_status(devapc_ctx, slave_type, vio_idx);
> > > +
> > > +               mask_module_irq(devapc_ctx, slave_type, vio_idx, false);
> > > +       }
> > > +
> > > +       return IRQ_HANDLED;
> > > +}
> > > +
> > > +/*
> > > + * start_devapc - initialize devapc status and start receiving interrupt
> > > + *               while devapc violation is triggered.
> > > + */
> >
> > [snip]
> >
> > > +
> > > +struct mtk_device_info {
> > > +       int sys_index;
> >
> > Useless, so remove it.
>
> We need to print it as our debug information.
> But I did not apply it on this patch, I'll add it on next patch.

I think vio address is enough to find out the murder, so remove it in
this patch. If it provide another information, add it in another patch
and describe clear about what is this and how to use this information.

>
> >
> > > +       int ctrl_index;
> >
> > Ditto.
> >
> > Regards,
> > Chun-Kuang.
> >
> > > +       int vio_index;
> > > +};
> > > +
>