| 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: <2025062059-dangling-coping-1e17@gregkh>
Date: Fri, 20 Jun 2025 16:54:28 +0200
From: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To: nicolas.bouchinet@....cyber.gouv.fr
Cc: Alan Stern <stern@...land.harvard.edu>,
Kannappan R <r.kannappan@...el.com>,
Sabyrzhan Tasbolatov <snovitoll@...il.com>,
Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
Stefan Eichenberger <stefan.eichenberger@...adex.com>,
Thomas Gleixner <tglx@...utronix.de>,
Pawel Laszczak <pawell@...ence.com>, Ma Ke <make_ruc2021@....com>,
Jeff Johnson <jeff.johnson@....qualcomm.com>,
Luc Bonnafoux <luc.bonnafoux@....gouv.fr>,
Luc Bonnafoux <luc.bonnafoux@....cyber.gouv.fr>,
Nicolas Bouchinet <nicolas.bouchinet@....gouv.fr>,
linux-kernel@...r.kernel.org, linux-usb@...r.kernel.org
Subject: Re: [RFC PATCH 2/4] usb: core: Introduce usb authentication feature
First off, thanks so much for doing this work, I've been wondering if
anyone would ever do it :)
Just a few quick review comments that you might want to do for the next
round of your patches for some basic style things:
On Fri, Jun 20, 2025 at 04:27:17PM +0200, nicolas.bouchinet@....cyber.gouv.fr wrote:
> +#include <linux/types.h>
> +#include <linux/usb.h>
> +#include <linux/usb/ch9.h>
> +#include <linux/usb/hcd.h>
> +#include <linux/usb/quirks.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/device.h>
> +#include <linux/delay.h>
> +#include <asm/byteorder.h>
> +
> +#include "authent_netlink.h"
> +
> +#include "authent.h"
No need for the blank lines there.
> +static int usb_authent_req_digest(struct usb_device *dev, uint8_t *const buffer,
> + uint8_t digest[256], uint8_t *mask)
> +{
> + int ret = 0;
> + struct usb_authent_digest_resp *digest_resp = NULL;
> +
> + if (unlikely((buffer == NULL || mask == NULL))) {
> + pr_err("invalid arguments\n");
> + return -EINVAL;
> + }
> + ret = usb_control_msg(dev, usb_rcvctrlpipe(dev, 0), AUTH_IN, USB_DIR_IN,
> + (USB_SECURITY_PROTOCOL_VERSION << 8) +
> + USB_AUTHENT_DIGEST_REQ_TYPE,
> + 0, buffer, 260, USB_CTRL_GET_TIMEOUT);
> + if (ret < 0) {
> + pr_err("Failed to get digest: %d\n", ret);
> + ret = -ECOMM;
> + goto exit;
> + }
> +
> + // Parse received digest response
> + digest_resp = (struct usb_authent_digest_resp *)buffer;
> + pr_notice("received digest response:\n");
> + pr_notice(" protocolVersion: %x\n", digest_resp->protocolVersion);
> + pr_notice(" messageType: %x\n", digest_resp->messageType);
> + pr_notice(" capability: %x\n", digest_resp->capability);
> + pr_notice(" slotMask: %x\n", digest_resp->slotMask);
Always use the dev_*() macros instead of pr_*() ones as that way you
know what device is making the message please.
> +
> + *mask = digest_resp->slotMask;
> + memcpy(digest, digest_resp->digests, 256);
> +
> + ret = 0;
> +
> +exit:
> +
> + return ret;
> +}
> +
> +struct usb_auth_cert_req {
> + uint16_t offset;
> + uint16_t length;
> +} __packed;
Kernel types are u16, not uint16_t. The uint*_t types are from
userspace C code, not kernel code. Yes, they are slowly sliding in in
places, but let's not do that unless really required for some specific
reason.
And why packed?
And what about endian issues, the data from the devices should be in a
specific format, right?
> +
> +/**
> + * @brief Request a specific part of a certificate chain from the device
Are you sure this is proper kerneldoc style? Does this build properly?
> + *
> + * Context: task context, might sleep
> + *
> + * Possible errors:
> + * - ECOMM : failed to send or receive a message to the device
> + * - EINVAL : if buffer or cert_part is NULL
> + *
> + * @param [in] dev : handle to the USB device
> + * @param [in,out] buffer : buffer used for communication, caller allocated
> + * @param [in] slot : slot in which to read the certificate
> + * @param [in] offset : at which the certificate fragment must be read
> + * @param [in] length : of the certificate fragment to read
> + * @param [out] cert_part : buffer to hold the fragment, caller allocated
Again, I don't think this is kerneldoc format. Please build the kernel
documentation output and see what this results in.
> + *
> + * @return 0 on SUCCESS else an error code
> + */
> +static int usb_auth_read_cert_part(struct usb_device *dev, uint8_t *const buffer,
> + const uint8_t slot, const uint16_t offset,
> + const uint16_t length, uint8_t *cert_part)
> +{
> + struct usb_auth_cert_req cert_req = { 0 };
> + int ret = -1;
Use real error values, not random integers :)
> +
> + if (unlikely(buffer == NULL || cert_part == NULL)) {
Only use likely/unlikely if you can measure the speed difference. For
USB, and probe sequences, that will never be the casae.
> + pr_err("invalid argument\n");
Again, dev_err()?
But how can these parameters not be set properly? You control how they
are called, no need to always verify that you wrote the code properly :)
> + return -EINVAL;
> + }
> +
> + cert_req.offset = offset;
> + cert_req.length = length;
> +
> + // AUTH OUT request transfer
> + memcpy(buffer, &cert_req, sizeof(struct usb_auth_cert_req));
> + ret = usb_control_msg(dev, usb_sndctrlpipe(dev, 0), AUTH_OUT,
> + USB_DIR_OUT,
> + (USB_SECURITY_PROTOCOL_VERSION << 8) +
> + USB_AUTHENT_CERTIFICATE_REQ_TYPE,
> + (slot << 8), buffer,
> + sizeof(struct usb_auth_cert_req),
> + USB_CTRL_GET_TIMEOUT);
> + if (ret < 0) {
> + pr_err("Failed to send certificate request: %d\n", ret);
> + ret = -ECOMM;
> + goto cleanup;
> + }
> +
> + // AUTH IN certificate read
> + ret = usb_control_msg(dev, usb_rcvctrlpipe(dev, 0), AUTH_IN, USB_DIR_IN,
> + (USB_SECURITY_PROTOCOL_VERSION << 8) +
> + USB_AUTHENT_CERTIFICATE_RESP_TYPE,
> + (slot << 8), buffer, length + 4,
> + USB_CTRL_GET_TIMEOUT);
> + if (ret < 0) {
> + pr_notice("Failed to get certificate from peripheral: %d\n", ret);
> + ret = -ECOMM;
> + goto cleanup;
> + }
> +
> + // TODO: parse received header
> + memcpy(cert_part, buffer + 4, length);
> +
> + ret = 0;
> +
> +cleanup:
> +
> + return ret;
As "cleanup:" does nothing, no need for it, just return the error value
above when you were doing a goto line.
> +}
> +
> +/**
> + * usb_authent_read_certificate - Read a device certificate
> + * @dev: [in] pointer to the usb device to query
> + * @buffer: [inout] buffer to hold request data, caller allocated
> + * @slot: [in] certificate chain to be read
> + * @cert_der: [out] buffer to hold received certificate chain
> + * @cert_len: [out] length of received certificate
> + *
> + * Context: task context, might sleep.
> + *
> + * Possible errors:
> + * - EINVAL : NULL pointer or invalid slot value
> + * - ECOMM : failed to send request to device
> + * - ENOMEM : failed to allocate memory for certificate
> + *
> + * Return: If successful, zero. Otherwise, a negative error number.
> + */
> +static int usb_authent_read_certificate(struct usb_device *dev, uint8_t *const buffer,
> + uint8_t slot, uint8_t **cert_der, size_t *cert_len)
> +{
> + uint16_t read_offset = 0;
> + uint16_t read_length = 0;
> + uint8_t chain_part[64] = { 0 };
Again, u16 and u8 please.
thanks,
greg k-h
Powered by blists - more mailing lists