Re: [RFC 1/2] nvme: add whitelisting infrastructure

[Kanchan Joshi <joshi.k@samsung.com>]

[-- Attachment #1: Type: text/plain, Size: 2008 bytes --]

On Sat, Sep 10, 2022 at 07:34:03AM +0200, Christoph Hellwig wrote:
>On Fri, Sep 09, 2022 at 10:57:44AM -0600, Keith Busch wrote:
>> On Fri, Sep 09, 2022 at 10:03:06PM +0530, Kanchan Joshi wrote:
>> > +bool nvme_admin_cmd_allowed(u8 opcode, fmode_t mode)
>> > +{
>> > +	/* allowed few read-only commands post the mode check */
>> > +	switch (opcode) {
>> > +	case nvme_admin_identify:
>> > +	case nvme_admin_get_log_page:
>> > +	case nvme_admin_get_features:
>> > +		return (mode & FMODE_READ);
>>
>> Some log pages have read side effects, like Namespace Changed List or anything
>> latched to RAE. That opcode seems a little more dangerous than the others in
>> the whitelist.
>
>Yes.  Some of the log pages (e.g. the persistent error log, or the LBA
>status log) are also getting really close to covert channels.  Can we
>please have really good justifications for why we'd whitelist anything
>on the admin side?

some of the information (namespace size, lba format etc.) is essential
to form io-command, and that information requires issuing admin-cmd.

But it seems we have another way to look at this.
Since we are talking about kernel (nvme driver) deciding what admin-cmd
should go (and what should not), onus in on nvme-driver to be right with
the choice. With pure static (driver-defined) whitelisting this is
what we get into.
Would it be better to consider dynamic (or hybrid) whitelisting for
admin-cmd? In that nvme-driver decides nothing (or little) but allows
admin to decide which admin-cmds are sane on a particular system. This
will still be on the line that 'root can do anything'.

Code wise, this could be a bitmap of 256 bits, one bit for each admin-cmd.
This can have few bits set by default (that driver trusts). 
While other bits (admin cmds) can be set only by admin-only ioctl.

Perhaps discussing this with code will be clearer. And we can do that in
a different RFC. And we sepearate the io-cmd whitelisting series from
all this as we seem to have more consensus on that already.

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]