linux-kernel - Re: [PATCH v3 2/4] iio: core: Add opaque_struct

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: <ZMgTCzslTd2REumj@smile.fi.intel.com>
Date:   Mon, 31 Jul 2023 23:01:15 +0300
From:   Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To:     Jonathan Cameron <jic23@...nel.org>
Cc:     linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org,
        Lars-Peter Clausen <lars@...afoo.de>,
        Uwe Kleine-König 
        <u.kleine-koenig@...gutronix.de>,
        Kees Cook <keescook@...omium.org>, Nuno Sa <nuno.sa@...log.com>
Subject: Re: [PATCH v3 2/4] iio: core: Add opaque_struct_size() helper and
 use it

On Sat, Jul 29, 2023 at 12:46:18PM +0100, Jonathan Cameron wrote:
> On Mon, 24 Jul 2023 14:02:02 +0300
> Andy Shevchenko <andriy.shevchenko@...ux.intel.com> wrote:

...

> > + * Note, when @s is 0, the alignment @a is added to the sizeof(*(@p))
> > + * and the result, depending on the @a, may be way off the initial size.
> 
> How often is this true?  A quick and dirty grep suggests at least 2 so perhaps
> worth retaining the old behaviour.

You mean that the sizeof(_some_grepped_struct_) is much less than an alignment
in those uses?

> Can we take that into account?  Maybe something like
> 
> #define opaque_struct_size(p, a, s) ((s) ? size_add(ALIGN(sizeof(*(p)), (a)), (s)): sizeof(*p)) 

(s) will be evaluated twice, not good. So, not in this form.

> Or do it at the call site below.

Looks much better to me.

...

> 	if (sizeof_priv)
> 		alloc_size = opaque_struct_size(iio_dev_opaque, IIO_DMA_MINALIGN, sizeof_priv);
> 	else
> 		alloc_size = sizeof(struct iio_dev_opaque);

Right.

...

> > -	indio_dev->priv = (char *)iio_dev_opaque +
> > -		ALIGN(sizeof(struct iio_dev_opaque), IIO_DMA_MINALIGN);
> > +	indio_dev->priv = opaque_struct_data(iio_dev_opaque, IIO_DMA_MINALIGN);
> 
> Would have been safer if original code set this to NULL if
> sizeof_priv == 0

Yeah, original code and proposed change has no difference in this sense.

> A driver doing that should never have used iio_priv() but nicer if it was
> NULL rather than off the end of the allocation.

Agree.
But looking at the above, I would rather see that in a form of

	if (...)
		priv = opaque_struct_data(...);
	else
		priv = NULL;

-- 
With Best Regards,
Andy Shevchenko