linux-kernel - RE: [PATCH 1/1] drivers: misc: Add support for nCipher HSM devices

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: <f5a9bf8828194315b4a9444480f52e11@exukdagfar01.INTERNAL.ROOT.TES>
Date:   Fri, 20 Dec 2019 15:27:51 +0000
From:   "Kim, David" <david.kim@...pher.com>
To:     Greg KH <gregkh@...uxfoundation.org>
CC:     "arnd@...db.de" <arnd@...db.de>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "Magee, Tim" <tim.magee@...pher.com>
Subject: RE: [PATCH 1/1] drivers: misc: Add support for nCipher HSM devices

Hi Greg,

I'm just about to push v2 of our patch but wanted to address your comments in the emails you raised them in.

> > +
> > +/* Device ioctl struct definitions */
> > +
> > +/* Result of the ENQUIRY ioctl. */
> > +struct nfdev_enquiry_str {
> > +	__u32 busno; /**< Which bus is the PCI device on. */
> > +	__u8 slotno; /**< Which slot is the PCI device in. */
> > +	__u8 reserved[3]; /**< for consistent struct alignment */
> 
> What is this crazy /**< text?
> 
> Please use correct kerneldoc formatting to help document things.

Done. I've also double checked all the other files. Checkpatch didn't seem to complain about these so we missed them, sorry.

> > + */
> > +#define NFDEV_CONTROL_HARMLESS(c) ((c) <= 1)
> 
> Why does userspace need this?
> 

It doesn't. We've removed that and a couple other leftovers.

> > +enum {
> > +	/** Enquiry ioctl.
> > +	 *  \return nfdev_enquiry_str describing the attached device.
> > +	 */
> > +	NFDEV_IOCTL_NUM_ENQUIRY = 1,
> > +
> > +	/** Channel Update ioctl.
> > +	 *  \deprecated
> > +	 */
> > +	NFDEV_IOCTL_NUM_CHUPDATE,
> 
> You have to explicitly set your enums if you want them to work properly in an
> ioctl.  As it is, this will fail in nasty ways you can never debug :(
> 

Thanks, we've set them explicitly and also removed the unnecessary ones.

> > +
> > +#define NFDEV_IOCTL_DEBUG                                                      \
> > +	_IOW(NFDEV_IOCTL_TYPE, NFDEV_IOCTL_NUM_DEBUG, int)
> 
> Why do you care about debugging to userspace through an ioctl?  Just use
> debugfs and be done with it, that's what it is there for.  Also you can use
> dynamic debugging (hopefully you already are) and use that kernel-wide
> api/interface.
> 
> Individual drivers should NEVER have custom debugging controls and macros.

Yes, this was accidentally left in. It's been removed now.

> 
> > +
> > +#define NFDEV_IOCTL_PCI_IFVERS                                                 \
> > +	_IOW(NFDEV_IOCTL_TYPE, NFDEV_IOCTL_NUM_PCI_IFVERS, int)
> 
> Can't you get this from the pci device information?
> 

Unfortunately not. Each time the device is brought up or has its state changed, the IFVERS is re-negotiated. This call is our means of discovering what IFVER is supported by the currently running firmware.

Thanks,
Dave