| 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
| ||
|
Date: Thu, 24 Aug 2023 16:26:56 +0300
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, conor+dt@...nel.org,
gregkh@...uxfoundation.org, linus.walleij@...aro.org,
quic_jhugo@...cinc.com, nipun.gupta@....com, tzimmermann@...e.de,
ogabbay@...nel.org, mathieu.poirier@...aro.org, axboe@...nel.dk,
damien.lemoal@...nsource.wdc.com, linux@...y.sk, arnd@...db.de,
yangyicong@...ilicon.com, benjamin.tissoires@...hat.com,
masahiroy@...nel.org, jacek.lawrynowicz@...ux.intel.com,
geert+renesas@...der.be, devicetree@...r.kernel.org
Subject: Re: [PATCHv5 2/4] wiegand: add Wiegand bus driver
On Thu, Aug 24, 2023 at 01:10:13PM +0200, Martin Zaťovič wrote:
> The Wiegand protocol serves as a standardized interface protocol, extensively
> employed within electronic access control systems, to facilitate data exchange
> between credentials, readers, and door controllers. Its inception can be
> attributed to the widespread adoption of Wiegand card readers, leveraging the
> Wiegand effect - a physical phenomenon wherein a Wiegand wire (or card)
> generates small yet distinct magnetic fields. A Wiegand reader detects the
> magnetic pulses emitted by the internal wires of the card.
>
> Three wires are central to the Wiegand protocol: a common ground wire and two
> distinct data wires designated as DATA0 and DATA1. During periods of inactivity,
> both DATA0 and DATA1 lines remain pulled up. For transmitting a '0,' the DATA0
> line is pulled down while DATA1 remains pulled up; conversely, transmitting
> a '1' causes DATA1 to be pulled down while DATA0 remains pulled up. Notably,
> this protocol ensures that the two lines never simultaneously experience a low
> state.
>
> Timing characteristics within the Wiegand protocol lack a uniform
> standardization, introducing variability between devices. Generally, pulse
> durations hover between 50 to 100 microseconds, while inter-pulse gaps span 20
> to 100 milliseconds. There is no stop bit or similar delimiter to signal the
> conclusion of a message. Instead, the receiver either counts the bits within the
> message or enforces a timeout, often set at around ten times the inter-pulse gap
> duration.
>
> The 26-Bit Wiegand Interface Standard, or 26-Bit Wiegand Format outlines the
> message format most commonly used among Wiegand devices. This format allocates
> the initial and terminal bits for parity. The subsequent eight bits following
> the initial parity bit are reserved for the Facility Code designed for distinct
> location identification. The remaining bits house the unique ID number. As the
> technology evolved, new Wiegand formats emerged, including 36-bit and 37-bit
> formats. It was also common practice for manufacturers to engineer devices
> compatible with proprietary Wiegand formats tailored to their specifications.
>
> The Wiegand bus driver handles Wiegand controller and Wiegand device
> managemement and driver matching. The bus driver defines the structures for
> Wiegand controllers and Wiegand devices. Wiegand controller structure contains
> all attributes that define the communication such as the payload_len for
> configuring the size of a single Wiegand message in bits, thus choosing a
> format. Each Wiegand controller should be associated with one Wiegand device,
> as Wiegand is typically a point-to-point bus.
...
> +#include <linux/bitops.h>
Should be bitmap.h
> +#include <linux/cdev.h>
> +#include <linux/container_of.h>
> +#include <linux/device.h>
> +#include <linux/dmaengine.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/module.h>
mutex.h ?
> +#include <linux/of_device.h>
> +#include <linux/property.h>
> +#include <linux/slab.h>
> +#include <linux/types.h>
> +#include <linux/wiegand.h>
...
> +static ssize_t wiegand_fwrite(struct file *filp, char __user const *buf, size_t len,
> + loff_t *offset)
> +{
> + int ret;
> + struct wiegand_controller *ctlr = filp->private_data;
> +
> + mutex_lock(&ctlr->file_lock);
> +
> + if (!buf || len == 0)
Ouch?!
To avoid this, you can use cleanup.h and guard your functions with a lock,
moreover in this case the check is not even needed to be performed under
the lock.
> + return -EINVAL;
> +
> + ret = wiegand_get_user_data(ctlr, buf, len);
> + if (ret < 0)
> + return ret;
> +
> + ctlr->transfer_message(ctlr);
> +
> + mutex_unlock(&ctlr->file_lock);
> + return len;
> +}
...
> +static int wiegand_fopen(struct inode *ino, struct file *filp)
> +{
> + int ret;
> + struct wiegand_controller *ctlr = container_of(filp->f_op, struct wiegand_controller, fops);
> +
> + filp->private_data = ctlr;
> +
> + mutex_lock(&ctlr->file_lock);
Do you care about the call being interrupted by a signal?
Ditto for other system calls in the framework.
> + if ((filp->f_flags & O_ACCMODE) == O_RDONLY || (filp->f_flags & O_ACCMODE) == O_RDWR) {
> + dev_err(ctlr->miscdev.this_device, "device is write only\n");
> + ret = -EIO;
> + goto out_unlock;
> + }
> +
> + mutex_unlock(&ctlr->file_lock);
> + return 0;
> +
> +out_unlock:
> + mutex_unlock(&ctlr->file_lock);
> + return ret;
> +}
...
> + size_t ctlr_size = ALIGN(sizeof(*ctlr), dma_get_cache_alignment());
Yeah, yet another user for a new macro in overflow.h (not yet there, though).
> + size_t total_size;
> +
> + if (check_add_overflow(size, ctlr_size, &total_size))
> + return NULL;
> +
> + ctlr = kzalloc(total_size, GFP_KERNEL);
> + if (!ctlr)
> + return NULL;
...
> +struct wiegand_controller *devm_wiegand_alloc_controller(struct device *dev, unsigned int size,
> + bool secondary)
> +{
> + struct wiegand_controller *ctlr;
> +
> + ctlr = wiegand_alloc_controller(dev, size, secondary);
> + if (ctlr)
> + ctlr->devm_allocated = true;
> + else
> + return NULL;
if (!ctrl)
return NULL;
or maybe
return ERR_PTR(-ENOMEM);
> + if (devm_add_action_or_reset(dev, wiegand_controller_put, ctlr))
> + return NULL;
ret = ...
if (ret)
return ERR_PTR(ret);
?
> +
> + return ctlr;
...
> +/**
> + * register_wiegand_device - allocates and registers a new Wiegand device
> + *
> + * @ctlr: controller structure to attach device to
> + * @node: firmware node for the device
Run
scripts/kernel-doc -v -none -Wall ...
against this file and fix all warnings.
> + */
> + struct fwnode_handle *node)
Usually we call it fwnode.
...
> + device_set_node(&wiegand->dev, node);
device_set_node(&wiegand->dev, fwnode_handle_get(fwnode));
> +out_node_put:
> + fwnode_handle_put(node);
Hmm... Shouldn't we do this at the ->release()?
> + put_device(&wiegand->dev);
> + wiegand_cleanup(wiegand);
> + return ERR_PTR(ret);
> +}
...
> + struct fwnode_handle *node;
fwnode
here and everywhere else.
...
> + fwnode_for_each_available_child_node(ctlr->dev.fwnode, node) {
device_for_each_child_node()
> + wiegand = register_wiegand_device(ctlr, node);
> + if (IS_ERR(wiegand))
> + dev_warn(&ctlr->dev, "failed to create wiegand device for %pfwf\n", node);
> + }
...
> + struct device *ctlr_dev = &ctlr->dev;
Just name it "dev". You can use similar approach in another functions,
like the above, to make them look nicer.
...
> + ret = device_property_read_u32(ctlr_dev, "pulse-len-us", &pulse_len);
> + if (!ret && pulse_len > 0)
> + timing->pulse_len = pulse_len;
> + else
> + timing->pulse_len = WIEGAND_DEFAULT_PULSE_LEN;
What about
/* Use default if property is absent or can't be read */
pulse_len = WIEGAND_DEFAULT_PULSE_LEN;
device_property_read_u32(dev, "pulse-len-us", &pulse_len);
timing->pulse_len = pulse_len;
Wrong values should be caught up by DT schema linter, no? If the parameters are
0 it's not your problem, although you can warn.
I'm not sure about this, so your current variant is okay.
Ditto for the rest.
...
> +static int __unregister(struct device *dev, void *null)
It is recommended to use namespace even if it's static function.
...
> +void wiegand_unregister_controller(void *ptr)
> +{
Why the parameter is not properly typed?
Yes for devm you probably need a wrapper
static devm_..._unregister_...(void *ctrl)
{
wiegand_unregister_controller(ctrl)
}
For the exported _unregister, taking into account the possible optional support
for this, you may need to check the parameter to be valid.
if (IS_ERR_OR_NULL(...)) // _OR_NULL probably is not needed as per above
return;
> +}
> + status = misc_register(&ctlr->miscdev);
> + if (status) {
> + dev_err(&ctlr->dev, "couldn't register misc device\n");
> + goto out_free_ida_name;
> + }
> + mutex_init(&ctlr->file_lock);
Shouldn't it be needed to be initialized before device itself?
...
> + status = device_add(&ctlr->dev);
> + if (status < 0)
Don't we need to call something like device_del() or put_device() at this point?
OK, read doc for device_add() it clarifies this.
> + goto out_free_ida_name_misc;
...
> + status = __wiegand_add_device(wiegand);
> + if (!status) {
> + ctlr->device_count++;
> + wiegand->id = wiegand->controller->device_count;
> + }
> +
> + return status;
if (status)
return status;
...
return 0;
...
> +void wiegand_unregister_device(struct wiegand_device *wiegand)
> +{
> + if (!wiegand)
IS_ERR_OR_NULL() or alike (see above)
> + return;
> +
> + fwnode_handle_put(wiegand->dev.fwnode);
No, do not dereference fwnode in struct device, always use proper API
and/or dev_fwnode() accessor.
> + device_del(&wiegand->dev);
> + wiegand_cleanup(wiegand);
> + put_device(&wiegand->dev);
> +}
...
> +static int wiegand_match_device(struct device *dev, struct device_driver *drv)
> +{
> + struct wiegand_device *wiegand_dev = to_wiegand_device(dev);
> +
> + if (of_driver_match_device(dev, drv))
> + return 1;
I would add ACPI support as well as
if (acpi_driver_match_device(dev, drv))
return 1;
> + return strcmp(wiegand_dev->modalias, drv->name) == 0;
> +}
...
> +static int __init wiegand_init(void)
> +{
> + int ret;
> +
> + ret = bus_register(&wiegand_bus_type);
> + if (ret < 0)
> + pr_err("Wiegand bus registration failed: %d\n", ret);
pr_fmt() may help to have unified prefix for all messages printed with help of
pr_*() macros.
> + return ret;
> +}
...
> +#define WIEGAND_MAX_PAYLEN_BYTES 256
I don't see you use BYTES, but rather BITS. Can you define _BITS instead?
...
> +struct wiegand_device;
+ Blank line.
> +/**
> + * struct wiegand_timing - Wiegand timings in microseconds
Perhaps timings ?
> + * @pulse_len: length of the low pulse
> + * @interval_len: length of a whole bit (both the pulse and the high phase)
> + * @frame_gap: length of the last bit of a frame (both the pulse and the high phase)
> + */
...
> + struct wiegand_timing timing;
Perhaps timings ?
...
> +#endif /* H_LINUX_INCLUDE_LINUX_WIEGAND_H */
Leading H is something unusual?
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists