| 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: <Y/+AoJiGL8PwJZJC@smile.fi.intel.com>
Date: Wed, 1 Mar 2023 18:43:12 +0200
From: Andy Shevchenko <andriy.shevchenko@...el.com>
To: Martin Zaťovič <m.zatovic1@...il.com>
Cc: linux-kernel@...r.kernel.org, robh+dt@...nel.org,
krzysztof.kozlowski+dt@...aro.org, gregkh@...uxfoundation.org,
airlied@...hat.com, dipenp@...dia.com, treding@...dia.com,
mwen@...lia.com, fmdefrancesco@...il.com, arnd@...db.de,
bvanassche@....org, ogabbay@...nel.org, axboe@...nel.dk,
mathieu.poirier@...aro.org, linux@...y.sk, masahiroy@...nel.org,
yangyicong@...ilicon.com, dan.j.williams@...el.com,
jacek.lawrynowicz@...ux.intel.com, benjamin.tissoires@...hat.com,
devicetree@...r.kernel.org, furong.zhou@...ux.intel.com,
linus.walleij@...aro.org
Subject: Re: [PATCHv3 4/4] wiegand: add Wiegand GPIO bitbanged controller
driver
On Wed, Mar 01, 2023 at 03:28:35PM +0100, Martin Zaťovič wrote:
> This controller formats the data to a Wiegand format and bit-bangs
> the message on devicetree defined GPIO lines.
>
> Several attributes need to be defined in the devicetree in order
> for this driver to work, namely the data-hi-gpios, data-lo-gpios,
> pulse-len, frame-gap and interval-len. These attributes are
> documented in the devicetree bindings documentation files.
>
> The driver creates a dev file for writing messages on the bus.
> It also creates a sysfs file to control the payload length of
> messages(in bits). If a message is shorter than the set payload
> length, it will be discarded. On the other hand, if a message is
> longer, the additional bits will be stripped off.
...
> +Date: January 2023
Blast from the past?
...
> +config WIEGAND_GPIO
> + tristate "GPIO-based wiegand master (write only)"
> + depends on WIEGAND
> + help
> + This GPIO bitbanging Wiegand controller uses the libgpiod library to
> + utilize GPIO lines for sending Wiegand data. Userspace may access
> + the Wiegand GPIO interface via a dev entry.
What will be the name of the module if M?
...
> +#include <linux/of.h>
No way.
...
> +struct wiegand_gpio {
> + struct device *dev;
> + struct wiegand_controller *ctlr;
> + struct miscdevice misc_dev;
Make it first, same idea as per previous patch comments.
> + struct mutex mutex;
> + struct gpio_desc *gpio_data_hi;
> + struct gpio_desc *gpio_data_lo;
> + struct file_operations fops;
> + u8 data[WIEGAND_MAX_PAYLEN_BYTES];
Have you considered DMA alignment? Is it a problem or not here?
> +};
...
> +static ssize_t store_ulong(u32 *val, const char *buf, size_t size, unsigned long max)
> +{
> + int rc;
> + u32 new;
> + if (max != 0 && new > max)
First part of the conditional is redundant. When you have such a user, you may
add the restriction back.
> + return -EINVAL;
> +
> + *val = new;
> + return size;
> +}
...
> +static struct attribute *wiegand_gpio_attrs[] = {
> + &dev_attr_payload_len.attr,
> + NULL,
No comma for the terminator entry.
> +};
> +
Redundant blank line.
> +ATTRIBUTE_GROUPS(wiegand_gpio);
...
> +void wiegand_gpio_send_bit(struct wiegand_gpio *wiegand_gpio, bool value, bool last)
> +{
> + u32 pulse_len = wiegand_gpio->ctlr->pulse_len;
> + u32 interval_len = wiegand_gpio->ctlr->interval_len;
> + u32 frame_gap = wiegand_gpio->ctlr->frame_gap;
> + struct gpio_desc *gpio = value ? wiegand_gpio->gpio_data_hi : wiegand_gpio->gpio_data_lo;
> +
> + gpiod_set_value_cansleep(gpio, 0);
> + udelay(pulse_len);
> + gpiod_set_value_cansleep(gpio, 1);
> +
> + if (last)
> + udelay(frame_gap - pulse_len);
> + else
> + udelay(interval_len - pulse_len);
This is quite dangerous. You may end up with CPU 100% load for a long time
without any way out. What is the range and why udelay() can't be replaced
with usleep_range() for longer waits?
> +}
...
> +/* This function is used for writing from file in dev directory */
> +static int wiegand_gpio_write_by_bits(struct wiegand_gpio *wiegand_gpio, u16 bitlen)
> +{
> + size_t i;
> + bool bit_value, is_last_bit;
> +
> + for (i = 0; i < bitlen; i++) {
> + bit_value = ((wiegand_gpio->data[i / 8] >> (7 - (i % 8))) & 0x01);
Ah, your buffer should probably be a bitmap.
Also consider bitmap_get_value8().
> + is_last_bit = (i + 1) == bitlen;
> + wiegand_gpio_send_bit(wiegand_gpio, bit_value, is_last_bit);
> + }
> +
> + return 0;
> +}
> +
> +static ssize_t wiegand_gpio_get_user_data(struct wiegand_gpio *wiegand_gpio, char __user const *buf,
> + size_t len)
> +{
> + size_t rc;
> +
> + if (len > WIEGAND_MAX_PAYLEN_BYTES)
> + return -EBADMSG;
> + rc = copy_from_user(&wiegand_gpio->data[0], buf, WIEGAND_MAX_PAYLEN_BYTES);
> + if (rc < 0)
> + return rc;
This is wrong. Homework: read the documentation and existing code to see why
and how to fix.
> + return len;
> +}
...
> +static ssize_t wiegand_gpio_fwrite(struct file *filp, char __user const *buf, size_t len,
> + loff_t *offset)
> +{
> + struct wiegand_gpio_instance *info = filp->private_data;
> + struct wiegand_gpio *wiegand_gpio = info->dev;
> + u32 msg_length = wiegand_gpio->ctlr->payload_len;
> + int rc;
> +
> + if (buf == NULL || len == 0 || len * 8 < msg_length)
!buf
DIV_ROUND_UP(msg_length / 8) > len
less overflow prone.
> + return -EINVAL;
> +
> + rc = wiegand_gpio_get_user_data(wiegand_gpio, buf, len);
> + if (rc < 0)
> + return rc;
> +
> + wiegand_gpio_write_by_bits(wiegand_gpio, msg_length);
> +
> + return len;
> +}
> +static int wiegand_gpio_fopen(struct inode *ino, struct file *filp)
> +{
> + int rc;
> + struct wiegand_gpio_instance *info;
> + struct wiegand_gpio *wiegand_gpio = container_of(filp->f_op, struct wiegand_gpio, fops);
> +
> + mutex_lock(&wiegand_gpio->mutex);
Can it be interrupted by a signal?
> + if ((filp->f_flags & O_ACCMODE) == O_RDONLY || (filp->f_flags & O_ACCMODE) == O_RDWR) {
> + dev_err(wiegand_gpio->dev, "Device is write only\n");
> + rc = -EIO;
> + goto err;
> + }
> +
> + info = kzalloc(sizeof(*info), GFP_KERNEL);
> + if (!info) {
> + rc = -ENOMEM;
> + goto err;
> + }
> +
> + info->dev = wiegand_gpio;
> + info->flags = filp->f_flags;
> + mutex_unlock(&wiegand_gpio->mutex);
> +
> + filp->private_data = info;
> +
> + return 0;
> +err:
> + mutex_unlock(&wiegand_gpio->mutex);
> + return rc;
> +}
...
> + u8 msg_bytelength = (msg_bitlen % 8) ? (msg_bitlen / 8) + 1 : (msg_bitlen / 8);
DIV_ROUND_UP() (you will need math.h)
...
> + if (dev->of_node)
> + master->dev.of_node = device->dev.of_node;
No.
...
> + if (status)
> + return status;
What's this and why is it here?
I'm afraid you haven't compiled this code... :-(
...
> + master->transfer_message = &wiegand_gpio_transfer_message;
> + master->payload_len = 26; /* set standard 26-bit format */
Can you replace master with some of the suggested words?
Or is this a terminology from the specification of the bus?
...
> + status = wiegand_gpio_request(dev, wiegand_gpio);
> + if (status) {
> + dev_err(wiegand_gpio->dev, "failed at requesting GPIOs\n");
> + return status;
return dev_error_probe();
Ditto for the rest.
> + }
...
> + status = gpiod_direction_output(wiegand_gpio->gpio_data_hi, 1);
> + status |= gpiod_direction_output(wiegand_gpio->gpio_data_lo, 1);
Huh?!
...
> + wiegand_gpio->misc_dev.name = kasprintf(GFP_KERNEL, "wiegand-gpio%u", master->bus_num);
No checks?
...
> + dev->groups = wiegand_gpio_groups;
Feels like this can be moved to dev_groups member of the struct driver.
> +
> + return status;
> +}
...
> +static const struct of_device_id wiegand_gpio_dt_idtable[] = {
> + { .compatible = "wiegand-gpio", },
> + {},
No comma for the terminator entry.
> +};
...
> +
Redundant blank line.
> +module_platform_driver(wiegand_gpio_driver);
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists