| 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: <CAHp75Vfj_2WJdcn4TC2SesUrWFmyhXHujS9P8O3JXdFcAb=WeQ@mail.gmail.com>
Date: Mon, 22 May 2023 13:58:23 +0300
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Artur Rojek <contact@...ur-rojek.eu>
Cc: Paul Cercueil <paul@...pouillou.net>,
Jonathan Cameron <jic23@...nel.org>,
Dmitry Torokhov <dmitry.torokhov@...il.com>,
Chris Morgan <macromorgan@...mail.com>,
linux-mips@...r.kernel.org, linux-iio@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-input@...r.kernel.org
Subject: Re: [PATCH v2 2/2] input: joystick: Fix buffer data parsing
On Mon, May 22, 2023 at 1:59 AM Artur Rojek <contact@...ur-rojek.eu> wrote:
>
> Don't try to access buffer data of a channel by its scan index. Instead,
> calculate its offset in the buffer.
>
> This is necessary, as the scan index of a channel does not represent its
> position in a buffer - the buffer will contain data for enabled channels
> only, affecting data offsets and alignment.
>
> While at it, also fix minor style issue in probe.
a minor
the probe
> Reported-by: Chris Morgan <macromorgan@...mail.com>
> Closes: https://lore.kernel.org/linux-input/20220408212857.9583-1-macroalpha82@gmail.com/
What is this tag? Anything documented? Otherwise use BugLink or Link.
...
> struct adc_joystick_axis *axes;
> struct iio_channel *chans;
> + int *offsets;
Why not unsigned? I.o.w. is there any meaning for negative values?
> int num_chans;
> bool polled;
...
> + off = joy->offsets[i];
> +
Move this blank line to be before the previous line.
> + if (off < 0)
> + return -EINVAL;
...
> case 1:
> - val = ((const u8 *)data)[idx];
> + val = *chan_data;
Might be also
get_unaligned((const u8 *)chan_data);
And with all this (see below) the chan_data actually can be declared
as const void *.
> break;
> case 2:
> - data_u16 = (const u16 *)data + idx;
> -
> /*
> * Data is aligned to the sample size by IIO core.
> * Call `get_unaligned_xe16` to hide type casting.
> */
> if (endianness == IIO_BE)
> - val = get_unaligned_be16(data_u16);
> + val = get_unaligned_be16(chan_data);
> else if (endianness == IIO_LE)
> - val = get_unaligned_le16(data_u16);
> + val = get_unaligned_le16(chan_data);
> else /* IIO_CPU */
> - val = *data_u16;
> + val = *(const u16 *)chan_data;
This probably needs to be
get_unaligned((const u16 *)chan_data);
for the sake of consistency with the above.
> break;
> default:
> return -EINVAL;
...
> +static int adc_joystick_si_cmp(const void *a, const void *b, const void *priv)
> +{
> + const struct iio_channel *chans = priv;
> +
> + return chans[*(int *)a].channel->scan_index -
> + chans[*(int *)b].channel->scan_index;
Discarding const?
> +}
...
> + offsets = kmalloc_array(count, sizeof(int), GFP_KERNEL);
sizeof(*offsets)
> + if (!offsets)
> + return ERR_PTR(-ENOMEM);
...
> + si_order = kmalloc_array(count, sizeof(int), GFP_KERNEL);
sizeof(*si_order)
> + if (!si_order) {
> + kfree(offsets);
> + return ERR_PTR(-ENOMEM);
> + }
...
> + /* Channels in buffer are ordered by scan index. Sort to match that. */
the buffer
> + sort_r(si_order, count, sizeof(int), adc_joystick_si_cmp, NULL, chans);
sizeof(*si_order) ?
sizeof(int) is a bit odd, the above will tell better without even
knowing the sort_r() parameters what it is about.
...
> + for (i = 0; i < count; ++i) {
> + idx = si_order[i];
> + ch = chans[idx].channel;
> + si = ch->scan_index;
> +
> + if (si < 0 || !test_bit(si, indio_dev->active_scan_mask)) {
> + offsets[idx] = -1;
> + continue;
> + }
> +
> + /* Channels sharing scan indices also share the samples. */
> + if (idx > 0 && si == chans[idx - 1].channel->scan_index) {
> + offsets[idx] = offsets[idx - 1];
> + continue;
> + }
> +
> + offsets[idx] = offset;
> + length = ch->scan_type.storagebits / 8;
BITS_PER_BYTE ?
> + if (ch->scan_type.repeat > 1)
> + length *= ch->scan_type.repeat;
> + /* Account for channel alignment. */
> + if (offset % length)
> + offset += length - (offset % length);
Would one of ALIGN() / rounddown / etc work here?
> + offset += length;
> + }
...
> + joy->offsets = adc_joystick_get_chan_offsets(joy->chans,
> + joy->num_chans);
> + if (IS_ERR(joy->offsets)) {
> + dev_err(devp, "Unable to allocate channel offsets\n");
> + return PTR_ERR(joy->offsets);
> + }
> +
> + return 0;
return PTR_ERR_OR_ZERO() ?
...
> error = devm_add_action_or_reset(dev, adc_joystick_cleanup,
> joy->buffer);
> - if (error) {
> + if (error) {
> dev_err(dev, "Unable to add action\n");
> return error;
> }
Unrelated change.
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists