* Re: [PATCH] KEYS: encrypted: Add check for strsep [not found] <20231108073627.1063464-1-nichen@iscas.ac.cn> @ 2024-01-24 18:21 ` Verma, Vishal L 2024-01-24 19:15 ` Mimi Zohar 2024-01-30 17:19 ` Jarkko Sakkinen 0 siblings, 2 replies; 13+ messages in thread From: Verma, Vishal L @ 2024-01-24 18:21 UTC (permalink / raw) To: zohar@linux.ibm.com, paul@paul-moore.com, jarkko@kernel.org, dhowells@redhat.com, yaelt@google.com, serge@hallyn.com, nichen@iscas.ac.cn, sumit.garg@linaro.org, jmorris@namei.org Cc: Jiang, Dave, linux-integrity@vger.kernel.org, linux-cxl@vger.kernel.org, linux-kernel@vger.kernel.org, Williams, Dan J, keyrings@vger.kernel.org, linux-security-module@vger.kernel.org, nvdimm@lists.linux.dev On Wed, 2023-11-08 at 07:36 +0000, Chen Ni wrote: > Add check for strsep() in order to transfer the error. > > Fixes: cd3bc044af48 ("KEYS: encrypted: Instantiate key with user- > provided decrypted data") > Signed-off-by: Chen Ni <nichen@iscas.ac.cn> > --- > security/keys/encrypted-keys/encrypted.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/security/keys/encrypted-keys/encrypted.c > b/security/keys/encrypted-keys/encrypted.c > index 8af2136069d2..76f55dd13cb8 100644 > --- a/security/keys/encrypted-keys/encrypted.c > +++ b/security/keys/encrypted-keys/encrypted.c > @@ -237,6 +237,10 @@ static int datablob_parse(char *datablob, const > char **format, > break; > } > *decrypted_data = strsep(&datablob, " \t"); > + if (!*decrypted_data) { > + pr_info("encrypted_key: decrypted_data is > missing\n"); > + break; > + } Hello, This patch seems to break keyring usage in CXL and NVDIMM, with the "decrypted_data is missing" error path being hit. Reverting this commit fixes the tests. I'm not sure if there are valid scenarios where this is expected to be empty? Here's an strace snippet of where the error occurs: keyctl(KEYCTL_SEARCH, KEY_SPEC_USER_KEYRING, "user", "nvdimm-master", 0) = 76300785 openat(AT_FDCWD, "/sys/devices/platform/cxl_acpi.0/root0/nvdimm-bridge0/ndbus0/nmem0/state", O_RDONLY|O_CLOEXEC) = 3 read(3, "idle\n", 1024) = 5 close(3) = 0 keyctl(KEYCTL_SEARCH, KEY_SPEC_USER_KEYRING, "encrypted", "nvdimm:0", 0) = -1 ENOKEY (Required key not available) uname({sysname="Linux", nodename="fedora", ...}) = 0 newfstatat(AT_FDCWD, "/etc/ndctl/keys/nvdimm_0_fedora.blob", 0x7fff23fbc210, 0) = -1 ENOENT (No such file or directory) add_key("encrypted", "nvdimm:0", "new enc32 user:nvdimm-master 32", 31, KEY_SPEC_USER_KEYRING) = -1 EINVAL (Invalid argument) ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] KEYS: encrypted: Add check for strsep 2024-01-24 18:21 ` [PATCH] KEYS: encrypted: Add check for strsep Verma, Vishal L @ 2024-01-24 19:15 ` Mimi Zohar 2024-01-24 20:10 ` Verma, Vishal L 2024-01-30 17:19 ` Jarkko Sakkinen 1 sibling, 1 reply; 13+ messages in thread From: Mimi Zohar @ 2024-01-24 19:15 UTC (permalink / raw) To: Verma, Vishal L, paul@paul-moore.com, jarkko@kernel.org, dhowells@redhat.com, yaelt@google.com, serge@hallyn.com, nichen@iscas.ac.cn, sumit.garg@linaro.org, jmorris@namei.org Cc: Jiang, Dave, linux-integrity@vger.kernel.org, linux-cxl@vger.kernel.org, linux-kernel@vger.kernel.org, Williams, Dan J, keyrings@vger.kernel.org, linux-security-module@vger.kernel.org, nvdimm@lists.linux.dev On Wed, 2024-01-24 at 18:21 +0000, Verma, Vishal L wrote: > On Wed, 2023-11-08 at 07:36 +0000, Chen Ni wrote: > > Add check for strsep() in order to transfer the error. > > > > Fixes: cd3bc044af48 ("KEYS: encrypted: Instantiate key with user- > > provided decrypted data") > > Signed-off-by: Chen Ni <nichen@iscas.ac.cn> > > --- > > security/keys/encrypted-keys/encrypted.c | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/security/keys/encrypted-keys/encrypted.c > > b/security/keys/encrypted-keys/encrypted.c > > index 8af2136069d2..76f55dd13cb8 100644 > > --- a/security/keys/encrypted-keys/encrypted.c > > +++ b/security/keys/encrypted-keys/encrypted.c > > @@ -237,6 +237,10 @@ static int datablob_parse(char *datablob, const > > char **format, > > break; > > } > > *decrypted_data = strsep(&datablob, " \t"); > > + if (!*decrypted_data) { > > + pr_info("encrypted_key: decrypted_data is > > missing\n"); > > + break; > > + } > > Hello, > > This patch seems to break keyring usage in CXL and NVDIMM, with the > "decrypted_data is missing" error path being hit. Reverting this commit > fixes the tests. I'm not sure if there are valid scenarios where this is > expected to be empty? > > Here's an strace snippet of where the error occurs: > > keyctl(KEYCTL_SEARCH, KEY_SPEC_USER_KEYRING, "user", "nvdimm-master", 0) = 76300785 > openat(AT_FDCWD, "/sys/devices/platform/cxl_acpi.0/root0/nvdimm-bridge0/ndbus0/nmem0/state", O_RDONLY|O_CLOEXEC) = 3 > read(3, "idle\n", 1024) = 5 > close(3) = 0 > keyctl(KEYCTL_SEARCH, KEY_SPEC_USER_KEYRING, "encrypted", "nvdimm:0", 0) = -1 ENOKEY (Required key not available) > uname({sysname="Linux", nodename="fedora", ...}) = 0 > newfstatat(AT_FDCWD, "/etc/ndctl/keys/nvdimm_0_fedora.blob", 0x7fff23fbc210, 0) = -1 ENOENT (No such file or directory) > add_key("encrypted", "nvdimm:0", "new enc32 user:nvdimm-master 32", 31, KEY_SPEC_USER_KEYRING) = -1 EINVAL (Invalid argument) Indeed! The user-provided decrypted data should be optional. The change needs to be reverted. thanks, Mimi ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] KEYS: encrypted: Add check for strsep 2024-01-24 19:15 ` Mimi Zohar @ 2024-01-24 20:10 ` Verma, Vishal L 2024-01-24 20:40 ` Mimi Zohar 0 siblings, 1 reply; 13+ messages in thread From: Verma, Vishal L @ 2024-01-24 20:10 UTC (permalink / raw) To: zohar@linux.ibm.com, paul@paul-moore.com, jarkko@kernel.org, dhowells@redhat.com, yaelt@google.com, serge@hallyn.com, nichen@iscas.ac.cn, sumit.garg@linaro.org, jmorris@namei.org Cc: Jiang, Dave, linux-integrity@vger.kernel.org, linux-cxl@vger.kernel.org, linux-kernel@vger.kernel.org, Williams, Dan J, keyrings@vger.kernel.org, linux-security-module@vger.kernel.org, nvdimm@lists.linux.dev On Wed, 2024-01-24 at 14:15 -0500, Mimi Zohar wrote: > On Wed, 2024-01-24 at 18:21 +0000, Verma, Vishal L wrote: > > On Wed, 2023-11-08 at 07:36 +0000, Chen Ni wrote: > > > Add check for strsep() in order to transfer the error. > > > > > > Fixes: cd3bc044af48 ("KEYS: encrypted: Instantiate key with user- > > > provided decrypted data") > > > Signed-off-by: Chen Ni <nichen@iscas.ac.cn> > > > --- > > > security/keys/encrypted-keys/encrypted.c | 4 ++++ > > > 1 file changed, 4 insertions(+) > > > > > > diff --git a/security/keys/encrypted-keys/encrypted.c > > > b/security/keys/encrypted-keys/encrypted.c > > > index 8af2136069d2..76f55dd13cb8 100644 > > > --- a/security/keys/encrypted-keys/encrypted.c > > > +++ b/security/keys/encrypted-keys/encrypted.c > > > @@ -237,6 +237,10 @@ static int datablob_parse(char *datablob, const > > > char **format, > > > break; > > > } > > > *decrypted_data = strsep(&datablob, " \t"); > > > + if (!*decrypted_data) { > > > + pr_info("encrypted_key: decrypted_data is > > > missing\n"); > > > + break; > > > + } > > > > Hello, > > > > This patch seems to break keyring usage in CXL and NVDIMM, with the > > "decrypted_data is missing" error path being hit. Reverting this commit > > fixes the tests. I'm not sure if there are valid scenarios where this is > > expected to be empty? > > > > Here's an strace snippet of where the error occurs: > > > > keyctl(KEYCTL_SEARCH, KEY_SPEC_USER_KEYRING, "user", "nvdimm-master", 0) = 76300785 > > openat(AT_FDCWD, "/sys/devices/platform/cxl_acpi.0/root0/nvdimm-bridge0/ndbus0/nmem0/state", O_RDONLY|O_CLOEXEC) = 3 > > read(3, "idle\n", 1024) = 5 > > close(3) = 0 > > keyctl(KEYCTL_SEARCH, KEY_SPEC_USER_KEYRING, "encrypted", "nvdimm:0", 0) = -1 ENOKEY (Required key not available) > > uname({sysname="Linux", nodename="fedora", ...}) = 0 > > newfstatat(AT_FDCWD, "/etc/ndctl/keys/nvdimm_0_fedora.blob", 0x7fff23fbc210, 0) = -1 ENOENT (No such file or directory) > > add_key("encrypted", "nvdimm:0", "new enc32 user:nvdimm-master 32", 31, KEY_SPEC_USER_KEYRING) = -1 EINVAL (Invalid argument) > > > Indeed! The user-provided decrypted data should be optional. The change needs > to be reverted. > Ah, thanks for confirming! Would you like me to send a revert patch or will you do it? Thanks, Vishal ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] KEYS: encrypted: Add check for strsep 2024-01-24 20:10 ` Verma, Vishal L @ 2024-01-24 20:40 ` Mimi Zohar 2024-01-24 21:10 ` Verma, Vishal L 0 siblings, 1 reply; 13+ messages in thread From: Mimi Zohar @ 2024-01-24 20:40 UTC (permalink / raw) To: Verma, Vishal L, paul@paul-moore.com, jarkko@kernel.org, dhowells@redhat.com, yaelt@google.com, serge@hallyn.com, nichen@iscas.ac.cn, sumit.garg@linaro.org, jmorris@namei.org Cc: Jiang, Dave, linux-integrity@vger.kernel.org, linux-cxl@vger.kernel.org, linux-kernel@vger.kernel.org, Williams, Dan J, keyrings@vger.kernel.org, linux-security-module@vger.kernel.org, nvdimm@lists.linux.dev On Wed, 2024-01-24 at 20:10 +0000, Verma, Vishal L wrote: > On Wed, 2024-01-24 at 14:15 -0500, Mimi Zohar wrote: > > On Wed, 2024-01-24 at 18:21 +0000, Verma, Vishal L wrote: > > > On Wed, 2023-11-08 at 07:36 +0000, Chen Ni wrote: > > > > Add check for strsep() in order to transfer the error. > > > > > > > > Fixes: cd3bc044af48 ("KEYS: encrypted: Instantiate key with user- > > > > provided decrypted data") > > > > Signed-off-by: Chen Ni <nichen@iscas.ac.cn> > > > > --- > > > > security/keys/encrypted-keys/encrypted.c | 4 ++++ > > > > 1 file changed, 4 insertions(+) > > > > > > > > diff --git a/security/keys/encrypted-keys/encrypted.c > > > > b/security/keys/encrypted-keys/encrypted.c > > > > index 8af2136069d2..76f55dd13cb8 100644 > > > > --- a/security/keys/encrypted-keys/encrypted.c > > > > +++ b/security/keys/encrypted-keys/encrypted.c > > > > @@ -237,6 +237,10 @@ static int datablob_parse(char *datablob, const > > > > char **format, > > > > break; > > > > } > > > > *decrypted_data = strsep(&datablob, " \t"); > > > > + if (!*decrypted_data) { > > > > + pr_info("encrypted_key: decrypted_data is > > > > missing\n"); > > > > + break; > > > > + } > > > > > > Hello, > > > > > > This patch seems to break keyring usage in CXL and NVDIMM, with the > > > "decrypted_data is missing" error path being hit. Reverting this commit > > > fixes the tests. I'm not sure if there are valid scenarios where this is > > > expected to be empty? > > > > > > Here's an strace snippet of where the error occurs: > > > > > > keyctl(KEYCTL_SEARCH, KEY_SPEC_USER_KEYRING, "user", "nvdimm-master", 0) = 76300785 > > > openat(AT_FDCWD, "/sys/devices/platform/cxl_acpi.0/root0/nvdimm-bridge0/ndbus0/nmem0/state", O_RDONLY|O_CLOEXEC) = 3 > > > read(3, "idle\n", 1024) = 5 > > > close(3) = 0 > > > keyctl(KEYCTL_SEARCH, KEY_SPEC_USER_KEYRING, "encrypted", "nvdimm:0", 0) = -1 ENOKEY (Required key not available) > > > uname({sysname="Linux", nodename="fedora", ...}) = 0 > > > newfstatat(AT_FDCWD, "/etc/ndctl/keys/nvdimm_0_fedora.blob", 0x7fff23fbc210, 0) = -1 ENOENT (No such file or directory) > > > add_key("encrypted", "nvdimm:0", "new enc32 user:nvdimm-master 32", 31, KEY_SPEC_USER_KEYRING) = -1 EINVAL (Invalid argument) > > > > > > Indeed! The user-provided decrypted data should be optional. The change needs > > to be reverted. > > > Ah, thanks for confirming! Would you like me to send a revert patch or > will you do it? Revert "KEYS: encrypted: Add check for strsep" This reverts commit b4af096b5df5dd131ab796c79cedc7069d8f4882. New encrypted keys are created either from kernel-generated random numbers or user-provided decrypted data. Revert the change requiring user-provided decrypted data. Can I add your Reported-by? Mimi ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] KEYS: encrypted: Add check for strsep 2024-01-24 20:40 ` Mimi Zohar @ 2024-01-24 21:10 ` Verma, Vishal L 2024-01-30 17:22 ` Jarkko Sakkinen 0 siblings, 1 reply; 13+ messages in thread From: Verma, Vishal L @ 2024-01-24 21:10 UTC (permalink / raw) To: zohar@linux.ibm.com, paul@paul-moore.com, jarkko@kernel.org, dhowells@redhat.com, yaelt@google.com, serge@hallyn.com, nichen@iscas.ac.cn, sumit.garg@linaro.org, jmorris@namei.org Cc: Jiang, Dave, linux-integrity@vger.kernel.org, linux-cxl@vger.kernel.org, linux-kernel@vger.kernel.org, Williams, Dan J, keyrings@vger.kernel.org, linux-security-module@vger.kernel.org, nvdimm@lists.linux.dev On Wed, 2024-01-24 at 15:40 -0500, Mimi Zohar wrote: > On Wed, 2024-01-24 at 20:10 +0000, Verma, Vishal L wrote: > > > > > Ah, thanks for confirming! Would you like me to send a revert patch or > > will you do it? > > Revert "KEYS: encrypted: Add check for strsep" > > This reverts commit b4af096b5df5dd131ab796c79cedc7069d8f4882. > > New encrypted keys are created either from kernel-generated random > numbers or user-provided decrypted data. Revert the change requiring > user-provided decrypted data. > > > Can I add your Reported-by? Yes that works, Thank you. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] KEYS: encrypted: Add check for strsep 2024-01-24 21:10 ` Verma, Vishal L @ 2024-01-30 17:22 ` Jarkko Sakkinen 2024-01-30 17:30 ` Jarkko Sakkinen 0 siblings, 1 reply; 13+ messages in thread From: Jarkko Sakkinen @ 2024-01-30 17:22 UTC (permalink / raw) To: Verma, Vishal L, zohar@linux.ibm.com, paul@paul-moore.com, dhowells@redhat.com, yaelt@google.com, serge@hallyn.com, nichen@iscas.ac.cn, sumit.garg@linaro.org, jmorris@namei.org Cc: Jiang, Dave, linux-integrity@vger.kernel.org, linux-cxl@vger.kernel.org, linux-kernel@vger.kernel.org, Williams, Dan J, keyrings@vger.kernel.org, linux-security-module@vger.kernel.org, nvdimm@lists.linux.dev On Wed Jan 24, 2024 at 11:10 PM EET, Verma, Vishal L wrote: > On Wed, 2024-01-24 at 15:40 -0500, Mimi Zohar wrote: > > On Wed, 2024-01-24 at 20:10 +0000, Verma, Vishal L wrote: > > > > > > > Ah, thanks for confirming! Would you like me to send a revert patch or > > > will you do it? > > > > Revert "KEYS: encrypted: Add check for strsep" > > > > This reverts commit b4af096b5df5dd131ab796c79cedc7069d8f4882. > > > > New encrypted keys are created either from kernel-generated random > > numbers or user-provided decrypted data. Revert the change requiring > > user-provided decrypted data. > > > > > > Can I add your Reported-by? > > Yes that works, Thank you. This went totally wrong IMHO. Priority should be to locate and fix the bug not revert useful stuff when a bug is found that has limited scope. BR, Jarkko ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] KEYS: encrypted: Add check for strsep 2024-01-30 17:22 ` Jarkko Sakkinen @ 2024-01-30 17:30 ` Jarkko Sakkinen 2024-01-30 18:25 ` Dan Williams 0 siblings, 1 reply; 13+ messages in thread From: Jarkko Sakkinen @ 2024-01-30 17:30 UTC (permalink / raw) To: Jarkko Sakkinen, Verma, Vishal L, zohar@linux.ibm.com, paul@paul-moore.com, dhowells@redhat.com, yaelt@google.com, serge@hallyn.com, nichen@iscas.ac.cn, sumit.garg@linaro.org, jmorris@namei.org Cc: Jiang, Dave, linux-integrity@vger.kernel.org, linux-cxl@vger.kernel.org, linux-kernel@vger.kernel.org, Williams, Dan J, keyrings@vger.kernel.org, linux-security-module@vger.kernel.org, nvdimm@lists.linux.dev On Tue Jan 30, 2024 at 7:22 PM EET, Jarkko Sakkinen wrote: > On Wed Jan 24, 2024 at 11:10 PM EET, Verma, Vishal L wrote: > > On Wed, 2024-01-24 at 15:40 -0500, Mimi Zohar wrote: > > > On Wed, 2024-01-24 at 20:10 +0000, Verma, Vishal L wrote: > > > > > > > > > Ah, thanks for confirming! Would you like me to send a revert patch or > > > > will you do it? > > > > > > Revert "KEYS: encrypted: Add check for strsep" > > > > > > This reverts commit b4af096b5df5dd131ab796c79cedc7069d8f4882. > > > > > > New encrypted keys are created either from kernel-generated random > > > numbers or user-provided decrypted data. Revert the change requiring > > > user-provided decrypted data. > > > > > > > > > Can I add your Reported-by? > > > > Yes that works, Thank you. > > This went totally wrong IMHO. > > Priority should be to locate and fix the bug not revert useful stuff > when a bug is found that has limited scope. By guidelines here the commit is also a bug fix and reverting such commit means seeding a bug to the mainline. Also the klog message alone is a bug fix here. So also by book it really has to come back as it was already commit because we cannot knowingly mount bugs to the mainline, right? BR, Jarkko ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] KEYS: encrypted: Add check for strsep 2024-01-30 17:30 ` Jarkko Sakkinen @ 2024-01-30 18:25 ` Dan Williams 2024-02-01 21:43 ` Jarkko Sakkinen 0 siblings, 1 reply; 13+ messages in thread From: Dan Williams @ 2024-01-30 18:25 UTC (permalink / raw) To: Jarkko Sakkinen, Verma, Vishal L, zohar@linux.ibm.com, paul@paul-moore.com, dhowells@redhat.com, yaelt@google.com, serge@hallyn.com, nichen@iscas.ac.cn, sumit.garg@linaro.org, jmorris@namei.org Cc: Jiang, Dave, linux-integrity@vger.kernel.org, linux-cxl@vger.kernel.org, linux-kernel@vger.kernel.org, Williams, Dan J, keyrings@vger.kernel.org, linux-security-module@vger.kernel.org, nvdimm@lists.linux.dev Jarkko Sakkinen wrote: > On Tue Jan 30, 2024 at 7:22 PM EET, Jarkko Sakkinen wrote: > > On Wed Jan 24, 2024 at 11:10 PM EET, Verma, Vishal L wrote: > > > On Wed, 2024-01-24 at 15:40 -0500, Mimi Zohar wrote: > > > > On Wed, 2024-01-24 at 20:10 +0000, Verma, Vishal L wrote: > > > > > > > > > > > Ah, thanks for confirming! Would you like me to send a revert patch or > > > > > will you do it? > > > > > > > > Revert "KEYS: encrypted: Add check for strsep" > > > > > > > > This reverts commit b4af096b5df5dd131ab796c79cedc7069d8f4882. > > > > > > > > New encrypted keys are created either from kernel-generated random > > > > numbers or user-provided decrypted data. Revert the change requiring > > > > user-provided decrypted data. > > > > > > > > > > > > Can I add your Reported-by? > > > > > > Yes that works, Thank you. > > > > This went totally wrong IMHO. > > > > Priority should be to locate and fix the bug not revert useful stuff > > when a bug is found that has limited scope. > > By guidelines here the commit is also a bug fix and reverting > such commit means seeding a bug to the mainline. Also the klog > message alone is a bug fix here. So also by book it really has > to come back as it was already commit because we cannot > knowingly mount bugs to the mainline, right? No, the commit broke userspace. The rule is do not cause regressions even if userspace is abusing the ABI in an undesirable way. Even the new pr_info() is a log spamming behavior change, a pr_debug() might be suitable, but otherwise a logic change here needs a clear description about what is broken about the old userspace behavior and why the kernel can not possibly safely handle it. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] KEYS: encrypted: Add check for strsep 2024-01-30 18:25 ` Dan Williams @ 2024-02-01 21:43 ` Jarkko Sakkinen 2024-02-02 0:05 ` Mimi Zohar 0 siblings, 1 reply; 13+ messages in thread From: Jarkko Sakkinen @ 2024-02-01 21:43 UTC (permalink / raw) To: Dan Williams, Verma, Vishal L, zohar@linux.ibm.com, paul@paul-moore.com, dhowells@redhat.com, yaelt@google.com, serge@hallyn.com, nichen@iscas.ac.cn, sumit.garg@linaro.org, jmorris@namei.org Cc: Jiang, Dave, linux-integrity@vger.kernel.org, linux-cxl@vger.kernel.org, linux-kernel@vger.kernel.org, keyrings@vger.kernel.org, linux-security-module@vger.kernel.org, nvdimm@lists.linux.dev On Tue Jan 30, 2024 at 8:25 PM EET, Dan Williams wrote: > Jarkko Sakkinen wrote: > > On Tue Jan 30, 2024 at 7:22 PM EET, Jarkko Sakkinen wrote: > > > On Wed Jan 24, 2024 at 11:10 PM EET, Verma, Vishal L wrote: > > > > On Wed, 2024-01-24 at 15:40 -0500, Mimi Zohar wrote: > > > > > On Wed, 2024-01-24 at 20:10 +0000, Verma, Vishal L wrote: > > > > > > > > > > > > > Ah, thanks for confirming! Would you like me to send a revert patch or > > > > > > will you do it? > > > > > > > > > > Revert "KEYS: encrypted: Add check for strsep" > > > > > > > > > > This reverts commit b4af096b5df5dd131ab796c79cedc7069d8f4882. > > > > > > > > > > New encrypted keys are created either from kernel-generated random > > > > > numbers or user-provided decrypted data. Revert the change requiring > > > > > user-provided decrypted data. > > > > > > > > > > > > > > > Can I add your Reported-by? > > > > > > > > Yes that works, Thank you. > > > > > > This went totally wrong IMHO. > > > > > > Priority should be to locate and fix the bug not revert useful stuff > > > when a bug is found that has limited scope. > > > > By guidelines here the commit is also a bug fix and reverting > > such commit means seeding a bug to the mainline. Also the klog > > message alone is a bug fix here. So also by book it really has > > to come back as it was already commit because we cannot > > knowingly mount bugs to the mainline, right? > > No, the commit broke userspace. The rule is do not cause regressions > even if userspace is abusing the ABI in an undesirable way. Even the > new pr_info() is a log spamming behavior change, a pr_debug() might be > suitable, but otherwise a logic change here needs a clear description > about what is broken about the old userspace behavior and why the kernel > can not possibly safely handle it. The rationale literally gives empirical proof that the log message is useful by measure. It would be useless if log level is decreased to debug, as then sysadmin's won't take notice. I don't really know what is the definition of "spam" here but at least for me actually useful log message are not in that category. Issue was legit but git revert is objectively an incorrect way to address the bug. BR, Jarkko ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] KEYS: encrypted: Add check for strsep 2024-02-01 21:43 ` Jarkko Sakkinen @ 2024-02-02 0:05 ` Mimi Zohar 2024-02-12 5:11 ` Jarkko Sakkinen 0 siblings, 1 reply; 13+ messages in thread From: Mimi Zohar @ 2024-02-02 0:05 UTC (permalink / raw) To: Jarkko Sakkinen, Dan Williams, Verma, Vishal L, paul@paul-moore.com, dhowells@redhat.com, yaelt@google.com, serge@hallyn.com, nichen@iscas.ac.cn, sumit.garg@linaro.org, jmorris@namei.org Cc: Jiang, Dave, linux-integrity@vger.kernel.org, linux-cxl@vger.kernel.org, linux-kernel@vger.kernel.org, keyrings@vger.kernel.org, linux-security-module@vger.kernel.org, nvdimm@lists.linux.dev On Thu, 2024-02-01 at 23:43 +0200, Jarkko Sakkinen wrote: > On Tue Jan 30, 2024 at 8:25 PM EET, Dan Williams wrote: > > Jarkko Sakkinen wrote: > > > On Tue Jan 30, 2024 at 7:22 PM EET, Jarkko Sakkinen wrote: > > > > On Wed Jan 24, 2024 at 11:10 PM EET, Verma, Vishal L wrote: > > > > > On Wed, 2024-01-24 at 15:40 -0500, Mimi Zohar wrote: > > > > > > On Wed, 2024-01-24 at 20:10 +0000, Verma, Vishal L wrote: > > > > > > > Ah, thanks for confirming! Would you like me to send a > > > > > > > revert patch or > > > > > > > will you do it? > > > > > > > > > > > > Revert "KEYS: encrypted: Add check for strsep" > > > > > > > > > > > > This reverts commit > > > > > > b4af096b5df5dd131ab796c79cedc7069d8f4882. > > > > > > > > > > > > New encrypted keys are created either from kernel-generated > > > > > > random > > > > > > numbers or user-provided decrypted data. Revert the change > > > > > > requiring > > > > > > user-provided decrypted data. > > > > > > > > > > > > > > > > > > Can I add your Reported-by? > > > > > > > > > > Yes that works, Thank you. > > > > > > > > This went totally wrong IMHO. > > > > > > > > Priority should be to locate and fix the bug not revert useful > > > > stuff > > > > when a bug is found that has limited scope. > > > > > > By guidelines here the commit is also a bug fix and reverting > > > such commit means seeding a bug to the mainline. Also the klog > > > message alone is a bug fix here. So also by book it really has > > > to come back as it was already commit because we cannot > > > knowingly mount bugs to the mainline, right? > > > > No, the commit broke userspace. The rule is do not cause > > regressions > > even if userspace is abusing the ABI in an undesirable way. Even > > the > > new pr_info() is a log spamming behavior change, a pr_debug() might > > be > > suitable, but otherwise a logic change here needs a clear > > description > > about what is broken about the old userspace behavior and why the > > kernel > > can not possibly safely handle it. > > The rationale literally gives empirical proof that the log message > is useful by measure. It would be useless if log level is decreased > to debug, as then sysadmin's won't take notice. I don't really know > what is the definition of "spam" here but at least for me actually > useful log message are not in that category. > > Issue was legit but git revert is objectively an incorrect way to > address the bug. No, I made a mistake in upstreaming the patch in the first place. It broke the original "encrypted" keys usage. Reverting it was the correct solution. Mimi ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] KEYS: encrypted: Add check for strsep 2024-02-02 0:05 ` Mimi Zohar @ 2024-02-12 5:11 ` Jarkko Sakkinen 0 siblings, 0 replies; 13+ messages in thread From: Jarkko Sakkinen @ 2024-02-12 5:11 UTC (permalink / raw) To: Mimi Zohar, Dan Williams, Verma, Vishal L, paul@paul-moore.com, dhowells@redhat.com, yaelt@google.com, serge@hallyn.com, nichen@iscas.ac.cn, sumit.garg@linaro.org, jmorris@namei.org Cc: Jiang, Dave, linux-integrity@vger.kernel.org, linux-cxl@vger.kernel.org, linux-kernel@vger.kernel.org, keyrings@vger.kernel.org, linux-security-module@vger.kernel.org, nvdimm@lists.linux.dev On Fri Feb 2, 2024 at 12:05 AM UTC, Mimi Zohar wrote: > On Thu, 2024-02-01 at 23:43 +0200, Jarkko Sakkinen wrote: > > On Tue Jan 30, 2024 at 8:25 PM EET, Dan Williams wrote: > > > Jarkko Sakkinen wrote: > > > > On Tue Jan 30, 2024 at 7:22 PM EET, Jarkko Sakkinen wrote: > > > > > On Wed Jan 24, 2024 at 11:10 PM EET, Verma, Vishal L wrote: > > > > > > On Wed, 2024-01-24 at 15:40 -0500, Mimi Zohar wrote: > > > > > > > On Wed, 2024-01-24 at 20:10 +0000, Verma, Vishal L wrote: > > > > > > > > Ah, thanks for confirming! Would you like me to send a > > > > > > > > revert patch or > > > > > > > > will you do it? > > > > > > > > > > > > > > Revert "KEYS: encrypted: Add check for strsep" > > > > > > > > > > > > > > This reverts commit > > > > > > > b4af096b5df5dd131ab796c79cedc7069d8f4882. > > > > > > > > > > > > > > New encrypted keys are created either from kernel-generated > > > > > > > random > > > > > > > numbers or user-provided decrypted data. Revert the change > > > > > > > requiring > > > > > > > user-provided decrypted data. > > > > > > > > > > > > > > > > > > > > > Can I add your Reported-by? > > > > > > > > > > > > Yes that works, Thank you. > > > > > > > > > > This went totally wrong IMHO. > > > > > > > > > > Priority should be to locate and fix the bug not revert useful > > > > > stuff > > > > > when a bug is found that has limited scope. > > > > > > > > By guidelines here the commit is also a bug fix and reverting > > > > such commit means seeding a bug to the mainline. Also the klog > > > > message alone is a bug fix here. So also by book it really has > > > > to come back as it was already commit because we cannot > > > > knowingly mount bugs to the mainline, right? > > > > > > No, the commit broke userspace. The rule is do not cause > > > regressions > > > even if userspace is abusing the ABI in an undesirable way. Even > > > the > > > new pr_info() is a log spamming behavior change, a pr_debug() might > > > be > > > suitable, but otherwise a logic change here needs a clear > > > description > > > about what is broken about the old userspace behavior and why the > > > kernel > > > can not possibly safely handle it. > > > > The rationale literally gives empirical proof that the log message > > is useful by measure. It would be useless if log level is decreased > > to debug, as then sysadmin's won't take notice. I don't really know > > what is the definition of "spam" here but at least for me actually > > useful log message are not in that category. > > > > Issue was legit but git revert is objectively an incorrect way to > > address the bug. > > No, I made a mistake in upstreaming the patch in the first place. It > broke the original "encrypted" keys usage. Reverting it was the > correct solution. > > Mimi The way I see it the semantic change caused the bug because it was not backwards compatible. That does not make the log message less useful. BR, Jarkko ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] KEYS: encrypted: Add check for strsep 2024-01-24 18:21 ` [PATCH] KEYS: encrypted: Add check for strsep Verma, Vishal L 2024-01-24 19:15 ` Mimi Zohar @ 2024-01-30 17:19 ` Jarkko Sakkinen 2024-01-30 17:20 ` Jarkko Sakkinen 1 sibling, 1 reply; 13+ messages in thread From: Jarkko Sakkinen @ 2024-01-30 17:19 UTC (permalink / raw) To: Verma, Vishal L, zohar@linux.ibm.com, paul@paul-moore.com, dhowells@redhat.com, yaelt@google.com, serge@hallyn.com, nichen@iscas.ac.cn, sumit.garg@linaro.org, jmorris@namei.org Cc: Jiang, Dave, linux-integrity@vger.kernel.org, linux-cxl@vger.kernel.org, linux-kernel@vger.kernel.org, Williams, Dan J, keyrings@vger.kernel.org, linux-security-module@vger.kernel.org, nvdimm@lists.linux.dev On Wed Jan 24, 2024 at 8:21 PM EET, Verma, Vishal L wrote: > On Wed, 2023-11-08 at 07:36 +0000, Chen Ni wrote: > > Add check for strsep() in order to transfer the error. > > > > Fixes: cd3bc044af48 ("KEYS: encrypted: Instantiate key with user- > > provided decrypted data") > > Signed-off-by: Chen Ni <nichen@iscas.ac.cn> > > --- > > security/keys/encrypted-keys/encrypted.c | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/security/keys/encrypted-keys/encrypted.c > > b/security/keys/encrypted-keys/encrypted.c > > index 8af2136069d2..76f55dd13cb8 100644 > > --- a/security/keys/encrypted-keys/encrypted.c > > +++ b/security/keys/encrypted-keys/encrypted.c > > @@ -237,6 +237,10 @@ static int datablob_parse(char *datablob, const > > char **format, > > break; > > } > > *decrypted_data = strsep(&datablob, " \t"); > > + if (!*decrypted_data) { > > + pr_info("encrypted_key: decrypted_data is > > missing\n"); > > + break; > > + } > > Hello, > > This patch seems to break keyring usage in CXL and NVDIMM, with the > "decrypted_data is missing" error path being hit. Reverting this commit > fixes the tests. I'm not sure if there are valid scenarios where this is > expected to be empty? > > Here's an strace snippet of where the error occurs: > > keyctl(KEYCTL_SEARCH, KEY_SPEC_USER_KEYRING, "user", "nvdimm-master", 0) = 76300785 > openat(AT_FDCWD, "/sys/devices/platform/cxl_acpi.0/root0/nvdimm-bridge0/ndbus0/nmem0/state", O_RDONLY|O_CLOEXEC) = 3 > read(3, "idle\n", 1024) = 5 > close(3) = 0 > keyctl(KEYCTL_SEARCH, KEY_SPEC_USER_KEYRING, "encrypted", "nvdimm:0", 0) = -1 ENOKEY (Required key not available) > uname({sysname="Linux", nodename="fedora", ...}) = 0 > newfstatat(AT_FDCWD, "/etc/ndctl/keys/nvdimm_0_fedora.blob", 0x7fff23fbc210, 0) = -1 ENOENT (No such file or directory) > add_key("encrypted", "nvdimm:0", "new enc32 user:nvdimm-master 32", 31, KEY_SPEC_USER_KEYRING) = -1 EINVAL (Invalid argument) > I think removing the klog message does not make sense meaning that the recent revert was wrong action taken. Instead necessary actions to retain backwards compatibility must be taken, meaning that the branch should set "ret = 0;". Motivation to keep it is dead obvious: your examples show that it can reveal potentially incorrect behaviour in user space software packages. It is info-level to mark that it can be also false positive. I.e. the revert commit takes away functionality that previously caused kernel masking a potential bug. Please revert the revert. BR, Jarkko ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] KEYS: encrypted: Add check for strsep 2024-01-30 17:19 ` Jarkko Sakkinen @ 2024-01-30 17:20 ` Jarkko Sakkinen 0 siblings, 0 replies; 13+ messages in thread From: Jarkko Sakkinen @ 2024-01-30 17:20 UTC (permalink / raw) To: Jarkko Sakkinen, Verma, Vishal L, zohar@linux.ibm.com, paul@paul-moore.com, dhowells@redhat.com, yaelt@google.com, serge@hallyn.com, nichen@iscas.ac.cn, sumit.garg@linaro.org, jmorris@namei.org Cc: Jiang, Dave, linux-integrity@vger.kernel.org, linux-cxl@vger.kernel.org, linux-kernel@vger.kernel.org, Williams, Dan J, keyrings@vger.kernel.org, linux-security-module@vger.kernel.org, nvdimm@lists.linux.dev [-- Attachment #1.1: Type: text/plain, Size: 2896 bytes --] On Tue Jan 30, 2024 at 7:19 PM EET, Jarkko Sakkinen wrote: > On Wed Jan 24, 2024 at 8:21 PM EET, Verma, Vishal L wrote: > > On Wed, 2023-11-08 at 07:36 +0000, Chen Ni wrote: > > > Add check for strsep() in order to transfer the error. > > > > > > Fixes: cd3bc044af48 ("KEYS: encrypted: Instantiate key with user- > > > provided decrypted data") > > > Signed-off-by: Chen Ni <nichen@iscas.ac.cn> > > > --- > > > security/keys/encrypted-keys/encrypted.c | 4 ++++ > > > 1 file changed, 4 insertions(+) > > > > > > diff --git a/security/keys/encrypted-keys/encrypted.c > > > b/security/keys/encrypted-keys/encrypted.c > > > index 8af2136069d2..76f55dd13cb8 100644 > > > --- a/security/keys/encrypted-keys/encrypted.c > > > +++ b/security/keys/encrypted-keys/encrypted.c > > > @@ -237,6 +237,10 @@ static int datablob_parse(char *datablob, const > > > char **format, > > > break; > > > } > > > *decrypted_data = strsep(&datablob, " \t"); > > > + if (!*decrypted_data) { > > > + pr_info("encrypted_key: decrypted_data is > > > missing\n"); > > > + break; > > > + } > > > > Hello, > > > > This patch seems to break keyring usage in CXL and NVDIMM, with the > > "decrypted_data is missing" error path being hit. Reverting this commit > > fixes the tests. I'm not sure if there are valid scenarios where this is > > expected to be empty? > > > > Here's an strace snippet of where the error occurs: > > > > keyctl(KEYCTL_SEARCH, KEY_SPEC_USER_KEYRING, "user", "nvdimm-master", 0) = 76300785 > > openat(AT_FDCWD, "/sys/devices/platform/cxl_acpi.0/root0/nvdimm-bridge0/ndbus0/nmem0/state", O_RDONLY|O_CLOEXEC) = 3 > > read(3, "idle\n", 1024) = 5 > > close(3) = 0 > > keyctl(KEYCTL_SEARCH, KEY_SPEC_USER_KEYRING, "encrypted", "nvdimm:0", 0) = -1 ENOKEY (Required key not available) > > uname({sysname="Linux", nodename="fedora", ...}) = 0 > > newfstatat(AT_FDCWD, "/etc/ndctl/keys/nvdimm_0_fedora.blob", 0x7fff23fbc210, 0) = -1 ENOENT (No such file or directory) > > add_key("encrypted", "nvdimm:0", "new enc32 user:nvdimm-master 32", 31, KEY_SPEC_USER_KEYRING) = -1 EINVAL (Invalid argument) > > > > I think removing the klog message does not make sense meaning > that the recent revert was wrong action taken. > > Instead necessary actions to retain backwards compatibility > must be taken, meaning that the branch should set "ret = 0;". > > Motivation to keep it is dead obvious: your examples show that > it can reveal potentially incorrect behaviour in user space > software packages. It is info-level to mark that it can be > also false positive. I.e. the revert commit takes away > functionality that previously caused kernel masking a > potential bug. > > Please revert the revert. > > BR, Jarkko See the attached patch. BR, Jarkko [-- Attachment #2: 0001-KEYS-encrypted-Return-0-for-empty-decrypted-data.patch --] [-- Type: text/x-patch, Size: 1110 bytes --] From cdfa4203c86412af39b232eaa7401591451659de Mon Sep 17 00:00:00 2001 From: Jarkko Sakkinen <jarkko@kernel.org> Date: Tue, 30 Jan 2024 19:11:50 +0200 Subject: [PATCH] KEYS: encrypted: Return 0 for empty decrypted data Return 0 to return backwards comptibility but *do keep* the klog entry for foresincs sake. Fixes: b4af096b5df5 ("KEYS: encrypted: Add check for strsep") Link: https://lore.kernel.org/keyrings/4d3465b48b9c5a87deb385b15bf5125fc1704019.camel@intel.com/ Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org> --- security/keys/encrypted-keys/encrypted.c | 1 + 1 file changed, 1 insertion(+) diff --git a/security/keys/encrypted-keys/encrypted.c b/security/keys/encrypted-keys/encrypted.c index 76f55dd13cb8..e3173141027e 100644 --- a/security/keys/encrypted-keys/encrypted.c +++ b/security/keys/encrypted-keys/encrypted.c @@ -225,6 +225,7 @@ static int datablob_parse(char *datablob, const char **format, *decrypted_datalen = strsep(&datablob, " \t"); if (!*decrypted_datalen) { pr_info("encrypted_key: keylen parameter is missing\n"); + ret = 0; goto out; } } -- 2.40.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
end of thread, other threads:[~2024-02-12 5:12 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20231108073627.1063464-1-nichen@iscas.ac.cn>
2024-01-24 18:21 ` [PATCH] KEYS: encrypted: Add check for strsep Verma, Vishal L
2024-01-24 19:15 ` Mimi Zohar
2024-01-24 20:10 ` Verma, Vishal L
2024-01-24 20:40 ` Mimi Zohar
2024-01-24 21:10 ` Verma, Vishal L
2024-01-30 17:22 ` Jarkko Sakkinen
2024-01-30 17:30 ` Jarkko Sakkinen
2024-01-30 18:25 ` Dan Williams
2024-02-01 21:43 ` Jarkko Sakkinen
2024-02-02 0:05 ` Mimi Zohar
2024-02-12 5:11 ` Jarkko Sakkinen
2024-01-30 17:19 ` Jarkko Sakkinen
2024-01-30 17:20 ` Jarkko Sakkinen
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox