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: <e8426444-0463-426a-892c-9493ba0cb543@app.fastmail.com>
Date:   Wed, 18 Oct 2023 13:02:49 +0200
From:   "Arnd Bergmann" <arnd@...db.de>
To:     "Leon Romanovsky" <leonro@...dia.com>
Cc:     "Saeed Mahameed" <saeed@...nel.org>,
        "Greg Kroah-Hartman" <gregkh@...uxfoundation.org>,
        linux-kernel@...r.kernel.org, "Jason Gunthorpe" <jgg@...dia.com>,
        "Jiri Pirko" <jiri@...dia.com>,
        "Saeed Mahameed" <saeedm@...dia.com>
Subject: Re: [PATCH 3/5] misc: mlx5ctl: Add info ioctl

On Wed, Oct 18, 2023, at 12:08, Leon Romanovsky wrote:
> On Wed, Oct 18, 2023 at 11:02:00AM +0200, Arnd Bergmann wrote:
>> On Wed, Oct 18, 2023, at 10:19, Saeed Mahameed wrote:
>> 
>> > Implement INFO ioctl to return the allocated UID and the capability flags
>> > and some other useful device information such as the underlying ConnectX
>> > device.
>> 
>> I'm commenting on the ABI here, ignoring everything for the moment.
>> 
>> >  static const struct file_operations mlx5ctl_fops = {
>> >  	.owner = THIS_MODULE,
>> >  	.open = mlx5ctl_open,
>> >  	.release = mlx5ctl_release,
>> > +	.unlocked_ioctl = mlx5ctl_ioctl,
>> >  };
>> 
>> There should be a .compat_ioctl entry as well, to allow 32-bit
>> tasks to use the same interface.
>
> Even for new code as well?
>
> Both kernel and userspace code is new, not released yet.

Yes, the main thing is that both x86 and arm support 32-bit
user space, and there are lots of users that use those in
containers and embedded systems. Even if it's unlikely to be
used in combination with your driver, there really isn't
much reason to be different from other drivers here.


>> I don't know what a UCTX UID is, but if this is related to
>> uid_t, it has to be 32 bit wide.
>
> UCTX UID is mlx5 hardware index, it is not uid_t.

Ok

>> 
>> > +	__u16 reserved1;
>> > +	__u32 uctx_cap; /* current process effective UCTX cap */
>> > +	__u32 dev_uctx_cap; /* device's UCTX capabilities */
>> > +	__u32 ucap; /* process user capability */
>> > +	__u32 reserved2[4];
>> > +};
>> 
>> If I'm counting right, this structure has a size of
>> 108 bytes but an alignment of 8 bytes, so you end up with
>> a 4-byte hole at the end, which introduces a risk of
>> leaking kernel data. Maybe give it an extra reserved word?
>
> I think that he needs to delete __u32 reserved2[4]; completely.

Removing the extra reserved words and the 'flags' is probably
best here, but you still need one 32-bit word to get to
a multiple of eight bytes to avoid the hole.

    Arnd

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ