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:
 <DM6PR12MB45163D7B0DB9B1A334652425D8072@DM6PR12MB4516.namprd12.prod.outlook.com>
Date: Tue, 9 Apr 2024 10:58:06 +0000
From: Danielle Ratson <danieller@...dia.com>
To: Matthew Wilcox <willy@...radead.org>, "Russell King (Oracle)"
	<linux@...linux.org.uk>
CC: "netdev@...r.kernel.org" <netdev@...r.kernel.org>, "davem@...emloft.net"
	<davem@...emloft.net>, "edumazet@...gle.com" <edumazet@...gle.com>,
	"kuba@...nel.org" <kuba@...nel.org>, "pabeni@...hat.com" <pabeni@...hat.com>,
	"corbet@....net" <corbet@....net>, "sdf@...gle.com" <sdf@...gle.com>,
	"kory.maincent@...tlin.com" <kory.maincent@...tlin.com>,
	"maxime.chevallier@...tlin.com" <maxime.chevallier@...tlin.com>,
	"vladimir.oltean@....com" <vladimir.oltean@....com>,
	"przemyslaw.kitszel@...el.com" <przemyslaw.kitszel@...el.com>,
	"ahmed.zaki@...el.com" <ahmed.zaki@...el.com>, "richardcochran@...il.com"
	<richardcochran@...il.com>, "shayagr@...zon.com" <shayagr@...zon.com>,
	"paul.greenwalt@...el.com" <paul.greenwalt@...el.com>, "jiri@...nulli.us"
	<jiri@...nulli.us>, "linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, mlxsw
	<mlxsw@...dia.com>, Petr Machata <petrm@...dia.com>, Ido Schimmel
	<idosch@...dia.com>
Subject: RE: [PATCH net-next 07/10] ethtool: cmis_cdb: Add a layer for
 supporting CDB commands

> -----Original Message-----
> From: Matthew Wilcox <willy@...radead.org>
> Sent: Monday, 8 April 2024 19:10
> To: Russell King (Oracle) <linux@...linux.org.uk>
> Cc: Danielle Ratson <danieller@...dia.com>; netdev@...r.kernel.org;
> davem@...emloft.net; edumazet@...gle.com; kuba@...nel.org;
> pabeni@...hat.com; corbet@....net; sdf@...gle.com;
> kory.maincent@...tlin.com; maxime.chevallier@...tlin.com;
> vladimir.oltean@....com; przemyslaw.kitszel@...el.com;
> ahmed.zaki@...el.com; richardcochran@...il.com; shayagr@...zon.com;
> paul.greenwalt@...el.com; jiri@...nulli.us; linux-doc@...r.kernel.org; linux-
> kernel@...r.kernel.org; mlxsw <mlxsw@...dia.com>; Petr Machata
> <petrm@...dia.com>; Ido Schimmel <idosch@...dia.com>
> Subject: Re: [PATCH net-next 07/10] ethtool: cmis_cdb: Add a layer for
> supporting CDB commands
> 
> On Mon, Apr 08, 2024 at 03:55:19PM +0100, Russell King (Oracle) wrote:
> > On Mon, Apr 08, 2024 at 03:53:37PM +0300, Danielle Ratson wrote:
> > > +/**
> > > + * struct ethtool_cmis_cdb_request - CDB commands request fields as
> decribed in
> > > + *				the CMIS standard
> > > + * @id: Command ID.
> > > + * @epl_len: EPL memory length.
> > > + * @lpl_len: LPL memory length.
> > > + * @chk_code: Check code for the previous field and the payload.
> > > + * @resv1: Added to match the CMIS standard request continuity.
> > > + * @resv2: Added to match the CMIS standard request continuity.
> > > + * @payload: Payload for the CDB commands.
> > > + */
> > > +struct ethtool_cmis_cdb_request {
> > > +	__be16 id;
> > > +	struct_group(body,
> > > +		u16 epl_len;
> >
> > u16 with a struct that also uses __be16 looks suspicious.
> 
> I'd understand if it were the other way around.  The command ID isn't a
> _number_, it's an ID.  You can't add 1 to it and get a meaningful requilt; all you
> can do is check it for equality, so a u16 makes sense for an ID.
> That's what I did in NVMe; command_id is typed as a __u16.
> 
> But a 'length' field is obviously a number.  You can meaningfully add and
> subtract numbers to it.  If there's an endian consideration, that's where you'd
> expect to see it.
> 
> So I concur, this is supicious.
> 
> (Oh, you might wonder why namespace ID (nsid) in the NVMe command is an
> le32 rather than a u32, since it is also an ID.  And that's because it's an ID
> which is exposed to userspace.  You wouldn't want to have big endian systems
> call the namespace 16777216 and little endian systems call it 1)

Hi,

Thank you two for the comment.
Yes, you are right. Does it make sense to you to define both as __be16? 

Thanks,
Danielle 

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ