| 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: <20250104144923.47933eb8@jic23-huawei>
Date: Sat, 4 Jan 2025 14:49:23 +0000
From: Jonathan Cameron <jic23@...nel.org>
To: Matteo Martelli <matteomartelli3@...il.com>
Cc: Lars-Peter Clausen <lars@...afoo.de>, Peter Rosin <peda@...ntia.se>,
linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] iio: iio-mux: kzalloc instead of devm_kzalloc to
ensure page alignment
On Wed, 11 Dec 2024 18:17:54 +0000
Jonathan Cameron <jic23@...nel.org> wrote:
> On Mon, 09 Dec 2024 11:39:55 +0100
> Matteo Martelli <matteomartelli3@...il.com> wrote:
>
> > On Sun, 8 Dec 2024 18:15:31 +0000, Jonathan Cameron <jic23@...nel.org> wrote:
> > > On Mon, 02 Dec 2024 16:11:08 +0100
> > > Matteo Martelli <matteomartelli3@...il.com> wrote:
> > >
> > > > During channel configuration, the iio-mux driver allocates a page with
> > > > devm_kzalloc(PAGE_SIZE) to read channel ext_info. However, the resulting
> > > > buffer points to an offset of the page due to the devres header sitting
> > > > at the beginning of the allocated area. This leads to failure in the
> > > > provider driver when sysfs_emit* helpers are used to format the ext_info
> > > > attributes.
> > > >
> > > > Switch to plain kzalloc version. The devres version is not strictly
> > > > necessary as the buffer is only accessed during the channel
> > > > configuration phase. Rely on __free cleanup to deallocate the buffer.
> > > > Also, move the ext_info handling into a new function to have the page
> > > > buffer definition and assignment in one statement as suggested by
> > > > cleanup documentation.
> > > >
> > > > Signed-off-by: Matteo Martelli <matteomartelli3@...il.com>
> > > This seems fine to me, but the diff ended up a bit complex, so I'd like
> > > Peter to take a look as well before I apply it.
> >
> > For a simpler diff I could go for devm_get_free_pages()+devm_free_pages(),
> > but since devres doesn't seem necessary in this case, I think this patch
> > provides a cleaner solution at the end.
>
> The approach is fine I think, I'd just like a second opinion so will
> give Peter some time to get to it before applying.
I want to get some build coverage at least on this, so I've applied it
for now to the togreg branch of iio.git and pushed out as testing.
Still a few more days for Peter to shout if he wants more time to looks at it!
Jonathan
>
> >
> > >
> > > Do you have a board that is hitting this? If so, a fixes tag is definitely
> > > appropriate. I think it is probably appropriate even it not.
> >
> > I am not sure if any existing board is affected as I encountered this
> > issue while experimenting with consumer drivers, thus using a custom DT
> > on top of sun50i-a64-pine64.dts just for testing. The following DT files
> > might be affected but only if the iio channel controlled by the iio_mux
> > multiplexer owns an ext_info attribute which is also exposed on sysfs.
> >
> > $ grep -Rl 'io-channel-mux' arch
> > arch/arm/boot/dts/aspeed/aspeed-bmc-ampere-mtmitchell.dts
> > arch/arm/boot/dts/aspeed/aspeed-bmc-ampere-mtjade.dts
> > arch/arm/boot/dts/microchip/at91-tse850-3.dts
> > arch/arm/boot/dts/microchip/at91-natte.dtsi
> > arch/arm64/boot/dts/rockchip/rk3566-powkiddy-rk2023.dtsi
> > arch/arm64/boot/dts/rockchip/rk3566-anbernic-rg353x.dtsi
> > arch/arm64/boot/dts/rockchip/rk3566-anbernic-rg503.dts
> > arch/arm64/boot/dts/rockchip/rk3326-odroid-go3.dts
> > arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-h.dtb
> > arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-h.dts
> >
> > I am also not sure what would be the reference commit for the Fixes tag.
> > The related ext_info attributes handling was introduced in the first
> > commit of the iio_mux implementation. If that applies, following the
> > corresponding Fixes tag.
> >
> > Fixes: 7ba9df54b091 ("iio: multiplexer: new iio category and iio-mux driver")
> That works I think.
>
> Thanks,
>
> >
> > >
> > > Jonathan
> > >
> >
> > Best regards,
> > Matteo
> > > > ---
> > > > drivers/iio/multiplexer/iio-mux.c | 84 +++++++++++++++++++++------------------
> > > > 1 file changed, 46 insertions(+), 38 deletions(-)
> > > >
> > > > diff --git a/drivers/iio/multiplexer/iio-mux.c b/drivers/iio/multiplexer/iio-mux.c
> > > > index 2953403bef53bbe47a97a8ab1c475ed88d7f86d2..c309d991490c63ba4299f1cda7102f10dcf54982 100644
> > > > --- a/drivers/iio/multiplexer/iio-mux.c
> > > > +++ b/drivers/iio/multiplexer/iio-mux.c
> > > > @@ -7,6 +7,7 @@
> > > > * Author: Peter Rosin <peda@...ntia.se>
> > > > */
> > > >
> > > > +#include <linux/cleanup.h>
> > > > #include <linux/err.h>
> > > > #include <linux/iio/consumer.h>
> > > > #include <linux/iio/iio.h>
> > > > @@ -237,49 +238,18 @@ static ssize_t mux_write_ext_info(struct iio_dev *indio_dev, uintptr_t private,
> > > > return ret;
> > > > }
> > > >
> > > > -static int mux_configure_channel(struct device *dev, struct mux *mux,
> > > > - u32 state, const char *label, int idx)
> > > > +static int mux_configure_chan_ext_info(struct device *dev, struct mux *mux,
> > > > + int idx, int num_ext_info)
> > > > {
> > > > struct mux_child *child = &mux->child[idx];
> > > > - struct iio_chan_spec *chan = &mux->chan[idx];
> > > > struct iio_chan_spec const *pchan = mux->parent->channel;
> > > > - char *page = NULL;
> > > > - int num_ext_info;
> > > > int i;
> > > > int ret;
> > > >
> > > > - chan->indexed = 1;
> > > > - chan->output = pchan->output;
> > > > - chan->datasheet_name = label;
> > > > - chan->ext_info = mux->ext_info;
> > > > -
> > > > - ret = iio_get_channel_type(mux->parent, &chan->type);
> > > > - if (ret < 0) {
> > > > - dev_err(dev, "failed to get parent channel type\n");
> > > > - return ret;
> > > > - }
> > > > -
> > > > - if (iio_channel_has_info(pchan, IIO_CHAN_INFO_RAW))
> > > > - chan->info_mask_separate |= BIT(IIO_CHAN_INFO_RAW);
> > > > - if (iio_channel_has_info(pchan, IIO_CHAN_INFO_SCALE))
> > > > - chan->info_mask_separate |= BIT(IIO_CHAN_INFO_SCALE);
> > > > -
> > > > - if (iio_channel_has_available(pchan, IIO_CHAN_INFO_RAW))
> > > > - chan->info_mask_separate_available |= BIT(IIO_CHAN_INFO_RAW);
> > > > -
> > > > - if (state >= mux_control_states(mux->control)) {
> > > > - dev_err(dev, "too many channels\n");
> > > > - return -EINVAL;
> > > > - }
> > > > -
> > > > - chan->channel = state;
> > > > + char *page __free(kfree) = kzalloc(PAGE_SIZE, GFP_KERNEL);
> > > > + if (!page)
> > > > + return -ENOMEM;
> > > >
> > > > - num_ext_info = iio_get_channel_ext_info_count(mux->parent);
> > > > - if (num_ext_info) {
> > > > - page = devm_kzalloc(dev, PAGE_SIZE, GFP_KERNEL);
> > > > - if (!page)
> > > > - return -ENOMEM;
> > > > - }
> > > > child->ext_info_cache = devm_kcalloc(dev,
> > > > num_ext_info,
> > > > sizeof(*child->ext_info_cache),
> > > > @@ -318,8 +288,46 @@ static int mux_configure_channel(struct device *dev, struct mux *mux,
> > > > child->ext_info_cache[i].size = ret;
> > > > }
> > > >
> > > > - if (page)
> > > > - devm_kfree(dev, page);
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static int mux_configure_channel(struct device *dev, struct mux *mux, u32 state,
> > > > + const char *label, int idx)
> > > > +{
> > > > + struct iio_chan_spec *chan = &mux->chan[idx];
> > > > + struct iio_chan_spec const *pchan = mux->parent->channel;
> > > > + int num_ext_info;
> > > > + int ret;
> > > > +
> > > > + chan->indexed = 1;
> > > > + chan->output = pchan->output;
> > > > + chan->datasheet_name = label;
> > > > + chan->ext_info = mux->ext_info;
> > > > +
> > > > + ret = iio_get_channel_type(mux->parent, &chan->type);
> > > > + if (ret < 0) {
> > > > + dev_err(dev, "failed to get parent channel type\n");
> > > > + return ret;
> > > > + }
> > > > +
> > > > + if (iio_channel_has_info(pchan, IIO_CHAN_INFO_RAW))
> > > > + chan->info_mask_separate |= BIT(IIO_CHAN_INFO_RAW);
> > > > + if (iio_channel_has_info(pchan, IIO_CHAN_INFO_SCALE))
> > > > + chan->info_mask_separate |= BIT(IIO_CHAN_INFO_SCALE);
> > > > +
> > > > + if (iio_channel_has_available(pchan, IIO_CHAN_INFO_RAW))
> > > > + chan->info_mask_separate_available |= BIT(IIO_CHAN_INFO_RAW);
> > > > +
> > > > + if (state >= mux_control_states(mux->control)) {
> > > > + dev_err(dev, "too many channels\n");
> > > > + return -EINVAL;
> > > > + }
> > > > +
> > > > + chan->channel = state;
> > > > +
> > > > + num_ext_info = iio_get_channel_ext_info_count(mux->parent);
> > > > + if (num_ext_info)
> > > > + return mux_configure_chan_ext_info(dev, mux, idx, num_ext_info);
> > > >
> > > > return 0;
> > > > }
> > > >
> > >
> >
>
>
Powered by blists - more mailing lists