| 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
| ||
|
Message-ID: <e5f542b253d0a3f5ef9db2ab21f0e25aef0feebf.camel@analog.com>
Date: Sun, 22 Mar 2020 16:11:58 +0000
From: "Ardelean, Alexandru" <alexandru.Ardelean@...log.com>
To: "andy.shevchenko@...il.com" <andy.shevchenko@...il.com>,
"keescook@...omium.org" <keescook@...omium.org>
CC: "linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
"Grozav, Andrei" <Andrei.Grozav@...log.com>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
"jic23@...nel.org" <jic23@...nel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"Hennerich, Michael" <Michael.Hennerich@...log.com>,
"lars@...afoo.de" <lars@...afoo.de>,
"Csomortani, Istvan" <Istvan.Csomortani@...log.com>,
"robh+dt@...nel.org" <robh+dt@...nel.org>,
"Bogdan, Dragos" <Dragos.Bogdan@...log.com>,
"Nagy, Laszlo" <Laszlo.Nagy@...log.com>,
"Costina, Adrian" <Adrian.Costina@...log.com>
Subject: Re: [PATCH v11 5/8] iio: adc: adi-axi-adc: add support for AXI ADC IP
core
On Sun, 2020-03-22 at 12:45 +0200, Andy Shevchenko wrote:
> +Cc Kees (see below about allocation size checks)
>
> On Sun, Mar 22, 2020 at 11:36 AM Ardelean, Alexandru
> <alexandru.Ardelean@...log.com> wrote:
> > On Sat, 2020-03-21 at 23:38 +0200, Andy Shevchenko wrote:
> > > On Sat, Mar 21, 2020 at 10:55 AM Alexandru Ardelean
> > > <alexandru.ardelean@...log.com> wrote:
>
> ...
>
> > > > Link: https://wiki.analog.com/resources/fpga/docs/axi_adc_ip
> >
> > i can send a v12 for this in a few days;
> >
> > > Is it tag or simple link? I would suggest not to use Link: if it's not a
> > > tag.
> >
> > simple link
> > any suggestions/alternatives?
> > i wasn't aware of conventions about this;
>
> Use like [1] ...
> ...
>
> [1]: https://...
>
> Or maybe introduce is as a tag DocLink:, for example?
> Or Datasheet: ?
>
> ...
>
> > > > +static struct adi_axi_adc_client *conv_to_client(struct
> > > > adi_axi_adc_conv
> > > > *conv)
> > > > +{
> > > > + if (!conv)
> > > > + return NULL;
> > >
> > > This is so unusual. Why do you need it?
> >
> > see [1]
> >
> > > > + return container_of(conv, struct adi_axi_adc_client, conv);
> > > > +}
> > > > +
> > > > +void *adi_axi_adc_conv_priv(struct adi_axi_adc_conv *conv)
> > > > +{
> > > > + struct adi_axi_adc_client *cl = conv_to_client(conv);
> > > > +
> > > > + if (!cl)
> > > > + return NULL;
> > >
> > > So about this.
> >
> > [1]
> > because 'adi_axi_adc_conv_priv()' (and implicitly conv_to_client()) gets
> > called
> > from other drivers; we can't expect to be sure that conv & cl aren't NULL;
>
> In both cases it's pointer arithmetic, right? Even look at the example
> of netdev you gave below, they haven't done these (redundant) checks.
> The outcome that crashes if any will be more distinct.
>
> > > > + return (char *)cl + ALIGN(sizeof(struct adi_axi_adc_client),
> > > > IIO_ALIGN);
> > >
> > > This all looks a bit confusing. Is it invention of offsetof() ?
> >
> > umm; tbh, it's more of a copy/clone of iio_priv()
> >
> > it's not un-common though;
> > see [and this one has more exposure]:
> > --------------------------------------------------------
> > static inline void *netdev_priv(const struct net_device *dev)
> > {
> > return (char *)dev + ALIGN(sizeof(struct net_device), NETDEV_ALIGN);
> > }
> > --------------------------------------------------------
>
> Good point.
>
> > > > +}
>
> ...
>
> > > > +static struct adi_axi_adc_conv *adi_axi_adc_conv_register(struct device
> > > > *dev,
> > > > + int
> > > > sizeof_priv)
> > > > +{
> > > > + struct adi_axi_adc_client *cl;
> > > > + size_t alloc_size;
> > > > +
> > > > + alloc_size = sizeof(struct adi_axi_adc_client);
> > > > + if (sizeof_priv) {
> > > > + alloc_size = ALIGN(alloc_size, IIO_ALIGN);
> > > > + alloc_size += sizeof_priv;
> > > > + }
> > > > + alloc_size += IIO_ALIGN - 1;
> > >
> > > Have you looked at linux/overflow.h?
> >
> > i did now;
> > any hints where i should look closer?
>
> It seems it lacks of this kind of allocation size checks... Perhaps add one?
> Kees, what do you think?
i borrowed this allocation logic from IIO core [iio_device_alloc()];
i may be stupid, but i still don't understand how to use overflow.h or what to
get from it;
the checks in there seem to be a bit too much for what's needed here;
or maybe there is something else in some newer linux-tree?
or maybe an example of how it's used?
>
> > > > + cl = kzalloc(alloc_size, GFP_KERNEL);
> > > > + if (!cl)
> > > > + return ERR_PTR(-ENOMEM);
>
> ...
>
> > > > +static void adi_axi_adc_conv_unregister(struct adi_axi_adc_conv *conv)
> > > > +{
> > > > + struct adi_axi_adc_client *cl = conv_to_client(conv);
> > > > +
> > > > + if (!cl)
> > > > + return;
> > >
> > > When is this possible?
> >
> > good point; it isn't;
> > it's a left-over from when adi_axi_adc_conv_unregister() was exported
> > still, i wouldn't mind leaving it [for paranoia], if there isn't a strong
> > opinion to remove it;
>
> I think it makes code dirty (too much protective programming). We have
> a lot places where we can shoot our feet, but at least not hiding the
> issue is a benefit in my opinion.
>
> ...
>
>
>
> > > > +static struct attribute *adi_axi_adc_attributes[] = {
> > > > + ADI_AXI_ATTR(SCALE_AVAIL, in_voltage_scale_available),
> > > > + NULL,
> > >
> > > Terminators good w/o comma.
> >
> > i don't feel strongly pro/against
> > sure
>
> There is a rationale behind this. If there is a weird case of adding
> entry behind the terminator, you will see it immediately at compile
> time (consider automatic rebase).
>
> > > > +};
> > >
> > > ...
> > >
> > > > +/* Match table for of_platform binding */
> > > > +static const struct of_device_id adi_axi_adc_of_match[] = {
> > > > + { .compatible = "adi,axi-adc-10.0.a", .data =
> > > > &adi_axi_adc_10_0_a_info },
> > > > + { /* end of list */ },
> > >
> > > Ditto.
>
> Ditto.
>
> > > > +};
>
> ...
>
> > > > + if (!dev->of_node) {
> > > > + dev_err(dev, "DT node is null\n");
> > > > + return ERR_PTR(-ENODEV);
> > > > + }
>
> I guess this check is redundant since following OF calls will fail anyway.
>
> > > > +
> > > > + id = of_match_node(adi_axi_adc_of_match, dev->of_node);
> > >
> > > You may use this from struct driver and move the table after this
> > > function.
> >
> > right; it didn't occur to me, since i was already using
> > of_device_get_match_data() in ad9467
>
> Even better. But see above note.
>
> > > > + if (!id)
> > > > + return ERR_PTR(-ENODEV);
Powered by blists - more mailing lists