Re: [PATCH for-next 2/2] nvme: Make CAP_SYS_ADMIN fine-granular

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

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

On Mon, Sep 26, 2022 at 10:30:14PM +0000, Chaitanya Kulkarni wrote:
>On 9/26/22 07:54, Kanchan Joshi wrote:
>> Change all the callers of CAP_SYS_ADMIN to go through nvme_cmd_allowed
>> for any decision making.
>> Since file open mode is taken into consideration for any
>> approval/denial, change at various places to keep file-mode information
>> handy.
>>
>> Signed-off-by: Kanchan Joshi <joshi.k@samsung.com>
>> ---
>>   drivers/nvme/host/ioctl.c | 70 +++++++++++++++++++++------------------
>>   1 file changed, 38 insertions(+), 32 deletions(-)
>>
>> diff --git a/drivers/nvme/host/ioctl.c b/drivers/nvme/host/ioctl.c
>> index 6ca6477dd899..4e53a01e702d 100644
>> --- a/drivers/nvme/host/ioctl.c
>> +++ b/drivers/nvme/host/ioctl.c
>> @@ -259,7 +259,7 @@ static bool nvme_validate_passthru_nsid(struct nvme_ctrl *ctrl,
>>   }
>>
>>   static int nvme_user_cmd(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
>> -			struct nvme_passthru_cmd __user *ucmd)
>> +			struct nvme_passthru_cmd __user *ucmd, fmode_t mode)
>>   {
>>   	struct nvme_passthru_cmd cmd;
>>   	struct nvme_command c;
>> @@ -267,10 +267,10 @@ static int nvme_user_cmd(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
>>   	u64 result;
>>   	int status;
>>
>> -	if (!capable(CAP_SYS_ADMIN))
>> -		return -EACCES;
>>   	if (copy_from_user(&cmd, ucmd, sizeof(cmd)))
>>   		return -EFAULT;
>> +	if (!nvme_cmd_allowed(ns, cmd.opcode, mode))
>> +		return -EACCES;
>
>you are chaning the order of the check CAP_SYS_ADMIN, unless there is a
>specific reason for it (that is not listed in the commit log) move
>nvme_cmd_allowed() where CAP_SYS_ADMIN is to retain the original
>behaviour which seems right since you are avoiding kernel copy in case
>cmds are not allowed.

cmd.opcode is required to make the decision making. So it cannot be
moved any up. 
User-space does not come to know whether error comes before/after
kernel-copy, so that part does not fall into behavior-change category.

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