linux-kernel - Re: [PATCH 1/2] irqchip: irq-mst: Add MStar interrupt controller support

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]

Date:   Wed, 19 Aug 2020 22:55:26 +0800
From:   Mark-PK Tsai <mark-pk.tsai@...iatek.com>
To:     <maz@...nel.org>, Mark-PK Tsai <mark-pk.tsai@...iatek.com>
CC:     <alix.wu@...iatek.com>, <daniel@...f.com>,
        <devicetree@...r.kernel.org>, <jason@...edaemon.net>,
        <linux-arm-kernel@...ts.infradead.org>,
        <linux-kernel@...r.kernel.org>,
        <linux-mediatek@...ts.infradead.org>, <matthias.bgg@...il.com>,
        <robh+dt@...nel.org>, <tglx@...utronix.de>,
        <yj.chiang@...iatek.com>
Subject: Re: [PATCH 1/2] irqchip: irq-mst: Add MStar interrupt controller support

From: Marc Zyngier <maz@...nel.org>

>On 2020-08-19 14:31, Mark-PK Tsai wrote:
>> From: Marc Zyngier <maz@...nel.org>
>> 
>>> > +
>>> > +static int mst_intc_domain_alloc(struct irq_domain *domain, unsigned
>>> > int virq,
>>> > +				 unsigned int nr_irqs, void *data)
>>> > +{
>>> > +	int i;
>>> > +	irq_hw_number_t hwirq;
>>> > +	struct irq_fwspec parent_fwspec, *fwspec = data;
>>> > +	struct mst_intc_chip_data *cd = (struct mst_intc_chip_data
>>> > *)domain->host_data;
>>> 
>>> No cast necessary here.
>>> 
>>> > +
>>> > +	/* Not GIC compliant */
>>> > +	if (fwspec->param_count != 3)
>>> > +		return -EINVAL;
>>> > +
>>> > +	/* No PPI should point to this domain */
>>> > +	if (fwspec->param[0])
>>> > +		return -EINVAL;
>>> > +
>>> > +	if (fwspec->param[1] >= cd->nr_irqs)
>>> 
>>> This condition is bogus, as it doesn't take into account the nr_irqs
>>> parameter.
>>> 
>> 
>> 
>> The hwirq number need to be in the irq map range. (property:
>> mstar,irqs-map-range)
>> If it's not, it must be incorrect configuration.
>
>I agree. And since you are checking whether the configuration is 
>correct,
>it'd better be completely correct.
>
>> So how about use the condition as following?
>> 
>> if (hwirq >= cd->nr_irqs)
>> 	return -EINVAL;
>
>Again, this says nothing of the validity of (hwirq + nr_irqs - 1)...
>

How about move this to mst_intc_domain_translate? Then all the irq_fwspec
point to domain mst_intc should be valid.

The mst_intc_domain_translate will be as following:

static int mst_intc_domain_translate(struct irq_domain *d,
				     struct irq_fwspec *fwspec,
				     unsigned long *hwirq,
				     unsigned int *type)
{
	struct mst_intc_chip_data *cd = d->host_data;

	if (is_of_node(fwspec->fwnode)) {
		if (fwspec->param_count != 3)
			return -EINVAL;

		/* No PPI should point to this domain */
		if (fwspec->param[0] != 0)
			return -EINVAL;

		if (fwspec->param[1] >= cd->nr_irqs)
			return -EINVAL;

		*hwirq = fwspec->param[1];
		*type = fwspec->param[2] & IRQ_TYPE_SENSE_MASK;
		return 0;
	}

	return -EINVAL;
}


>> 
>>> > +		return -EINVAL;
>>> > +
>>> > +	hwirq = fwspec->param[1];
>>> > +	for (i = 0; i < nr_irqs; i++)
>>> > +		irq_domain_set_hwirq_and_chip(domain, virq + i, hwirq + i,
>>> > +					      &mst_intc_chip,
>>> > +					      domain->host_data);