From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: MIME-Version: 1.0 In-Reply-To: <20160112013259.GC79247@tevye.fc.hp.com> References: <20160112013259.GC79247@tevye.fc.hp.com> Date: Tue, 12 Jan 2016 10:55:16 -0800 Message-ID: Subject: Re: [PATCH v5 0/6] nvdimm: Add an IOCTL pass thru for DSM calls From: Dan Williams Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org To: Jerry Hoemann Cc: Ross Zwisler , "Rafael J. Wysocki" , Len Brown , "Elliott, Robert (Persistent Memory)" , jmoyer , Dmitry Krivenok , Linda Knippers , Robert Moore , Lv Zheng , Rafael J Wysocki , "linux-nvdimm@lists.01.org" , Linux ACPI , "linux-kernel@vger.kernel.org" List-ID: On Mon, Jan 11, 2016 at 5:32 PM, Jerry Hoemann wrote: > On Sun, Jan 10, 2016 at 04:03:18PM -0800, Dan Williams wrote: >> On Wed, Jan 6, 2016 at 3:58 PM, Dan Williams wrote: [..] >> 2/ Disallow potentially invalid calls to reach firmware. At a minimum >> the kernel needs to know the uuid in advance for any dsm it wants to >> send. I.e. check the 'dsm_fun_idx' against the dsm_mask. This is >> also important for making sure the kernel can manage exclusive access >> to the configuration data area if present >> (ND_CMD_{GET|SET}_CONFIG_DATA). > > Technically, the kernel doesn't need to know the uuid in advance > as that is part of the bundle passed into the passthru. True, but the set of uuids the kernel ever needs to know about is likely small, and this policy mandates publication/notification of new command sets to the kernel community. Later on it gives the kernel a touch point to implement dsm function number blacklisting which I think is a useful security feature. I'll leave the UUID parameter in the command in case a device ever implements multiple command sets and we need to select between two function number spaces. > > > Are you concerned about firmware mis-behaving when presented > with a (UUID, Function_Index) that is not supported? > (and really we should add Revision ID to that tuple.) > > In a prior version of the patch not sent upstream, I did "discover" the > uuid and set up the dsm_mask. However, this created a need to modify > kernel each time uuid changes. Also, i don't think this is necessary > as FW should be gracefully validating its input arguments. By > not setting up/using dsm_mask in pass thru case, this can be tested. ACPICA will throw parse errors on mis-formatted DSMs. We can't prevent all malformed calls, but this is basic input validation that the kernel can perform. > I don't understand the exclusive access concern w/ config data. > Could you please elaborate? See nd_cmd_clear_to_send()... when a dimm is active the kernel mandates that updates to the namespace labels go through sysfs. This is a safety measure to prevent userspace from inadvertently clobbering in use labels. Once the dimm goes idle (all 'region' devices related to the dimm are disabled) userspace can manually update the configuration data area. From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1763103AbcALSzT (ORCPT ); Tue, 12 Jan 2016 13:55:19 -0500 Received: from mail-yk0-f172.google.com ([209.85.160.172]:34104 "EHLO mail-yk0-f172.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752978AbcALSzR (ORCPT ); Tue, 12 Jan 2016 13:55:17 -0500 MIME-Version: 1.0 In-Reply-To: <20160112013259.GC79247@tevye.fc.hp.com> References: <20160112013259.GC79247@tevye.fc.hp.com> Date: Tue, 12 Jan 2016 10:55:16 -0800 Message-ID: Subject: Re: [PATCH v5 0/6] nvdimm: Add an IOCTL pass thru for DSM calls From: Dan Williams To: Jerry Hoemann Cc: Ross Zwisler , "Rafael J. Wysocki" , Len Brown , "Elliott, Robert (Persistent Memory)" , jmoyer , Dmitry Krivenok , Linda Knippers , Robert Moore , Lv Zheng , Rafael J Wysocki , "linux-nvdimm@lists.01.org" , Linux ACPI , "linux-kernel@vger.kernel.org" Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Jan 11, 2016 at 5:32 PM, Jerry Hoemann wrote: > On Sun, Jan 10, 2016 at 04:03:18PM -0800, Dan Williams wrote: >> On Wed, Jan 6, 2016 at 3:58 PM, Dan Williams wrote: [..] >> 2/ Disallow potentially invalid calls to reach firmware. At a minimum >> the kernel needs to know the uuid in advance for any dsm it wants to >> send. I.e. check the 'dsm_fun_idx' against the dsm_mask. This is >> also important for making sure the kernel can manage exclusive access >> to the configuration data area if present >> (ND_CMD_{GET|SET}_CONFIG_DATA). > > Technically, the kernel doesn't need to know the uuid in advance > as that is part of the bundle passed into the passthru. True, but the set of uuids the kernel ever needs to know about is likely small, and this policy mandates publication/notification of new command sets to the kernel community. Later on it gives the kernel a touch point to implement dsm function number blacklisting which I think is a useful security feature. I'll leave the UUID parameter in the command in case a device ever implements multiple command sets and we need to select between two function number spaces. > > > Are you concerned about firmware mis-behaving when presented > with a (UUID, Function_Index) that is not supported? > (and really we should add Revision ID to that tuple.) > > In a prior version of the patch not sent upstream, I did "discover" the > uuid and set up the dsm_mask. However, this created a need to modify > kernel each time uuid changes. Also, i don't think this is necessary > as FW should be gracefully validating its input arguments. By > not setting up/using dsm_mask in pass thru case, this can be tested. ACPICA will throw parse errors on mis-formatted DSMs. We can't prevent all malformed calls, but this is basic input validation that the kernel can perform. > I don't understand the exclusive access concern w/ config data. > Could you please elaborate? See nd_cmd_clear_to_send()... when a dimm is active the kernel mandates that updates to the namespace labels go through sysfs. This is a safety measure to prevent userspace from inadvertently clobbering in use labels. Once the dimm goes idle (all 'region' devices related to the dimm are disabled) userspace can manually update the configuration data area.