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

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

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@oss.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