linux-kernel - Re: [RFC PATCH 2/4] usb: core: Introduce usb authentication feature

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <2025063051-yelp-brim-d2d6@gregkh>
Date: Mon, 30 Jun 2025 13:43:05 +0200
From: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To: Nicolas Bouchinet <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

On Mon, Jun 30, 2025 at 01:07:29PM +0200, Nicolas Bouchinet wrote:
> Hi Greg,
> 
> Thank you very much for your review. We will take every style remarks into
> account in the next patch version. Other responses are inline (there is only
> one):
> 
> On 6/20/25 16:54, Greg Kroah-Hartman wrote:
> > 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?
> 
> Since this structure is part of the usb authentication protocol, we need to
> be
> sure it is sent as is on the usb bus.

Great, then properly document it as such please.  Ideally in a header
file as we have tried to document all the USB messages in ch9.h where
defined.

thanks,

greg k-h