[PATCH] nvmem: Enforce nvmem stride in the sysfs interface

[PATCH] nvmem: Enforce nvmem stride in the sysfs interface

[Douglas Anderson]

The 'struct nvmem_config' has a stride attribute that specifies the
needed alignment for accesses into the nvmem.  This is used in
nvmem_cell_info_to_nvmem_cell() but not in the sysfs read/write
functions.  If the alignment is important in one place it's important
everywhere, so let's add enforcement.

For now we'll consider it totally invalid to access with the wrong
alignment.  We could relax this in the read case where we could just
read some extra bytes and throw them away.  Relaxing it in the write
case seems harder (and less safe?) since we'd have to read some data
first and then write it back.  To keep it symmetric we'll just
disallow it in both cases.

Reported-by: Ravi Kumar Bokka <rbokka@codeaurora.org>
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 drivers/nvmem/core.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index 05c6ae4b0b97..1c0e7953f90d 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -111,6 +111,9 @@ static ssize_t bin_attr_nvmem_read(struct file *filp, struct kobject *kobj,
 	if (pos >= nvmem->size)
 		return 0;
 
+	if (!IS_ALIGNED(pos, nvmem->stride))
+		return -EINVAL;
+
 	if (count < nvmem->word_size)
 		return -EINVAL;
 
@@ -148,6 +151,9 @@ static ssize_t bin_attr_nvmem_write(struct file *filp, struct kobject *kobj,
 	if (pos >= nvmem->size)
 		return -EFBIG;
 
+	if (!IS_ALIGNED(pos, nvmem->stride))
+		return -EINVAL;
+
 	if (count < nvmem->word_size)
 		return -EINVAL;
 
-- 
2.27.0.rc0.183.gde8f92d652-goog

Re: [PATCH] nvmem: Enforce nvmem stride in the sysfs interface

[Ravi Kumar Bokka (Temp)]

Hi,

On 5/29/2020 5:23 AM, Douglas Anderson wrote:
> The 'struct nvmem_config' has a stride attribute that specifies the
> needed alignment for accesses into the nvmem.  This is used in
> nvmem_cell_info_to_nvmem_cell() but not in the sysfs read/write
> functions.  If the alignment is important in one place it's important
> everywhere, so let's add enforcement.
> 
> For now we'll consider it totally invalid to access with the wrong
> alignment.  We could relax this in the read case where we could just
> read some extra bytes and throw them away.  Relaxing it in the write
> case seems harder (and less safe?) since we'd have to read some data
> first and then write it back.  To keep it symmetric we'll just
> disallow it in both cases.
> 
> Reported-by: Ravi Kumar Bokka <rbokka@codeaurora.org>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> 
>   drivers/nvmem/core.c | 6 ++++++
>   1 file changed, 6 insertions(+)
> 

I have reviewed and tested this patch.
Result: kernel crash resolved with unaligned offset.

Reviewed-by: Ravi Kumar Bokka <rbokka@codeaurora.org>
Tested-by: Ravi Kumar Bokka <rbokka@codeaurora.org>

> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> index 05c6ae4b0b97..1c0e7953f90d 100644
> --- a/drivers/nvmem/core.c
> +++ b/drivers/nvmem/core.c
> @@ -111,6 +111,9 @@ static ssize_t bin_attr_nvmem_read(struct file *filp, struct kobject *kobj,
>   	if (pos >= nvmem->size)
>   		return 0;
>   
> +	if (!IS_ALIGNED(pos, nvmem->stride))
> +		return -EINVAL;
> +
>   	if (count < nvmem->word_size)
>   		return -EINVAL;
>   
> @@ -148,6 +151,9 @@ static ssize_t bin_attr_nvmem_write(struct file *filp, struct kobject *kobj,
>   	if (pos >= nvmem->size)
>   		return -EFBIG;
>   
> +	if (!IS_ALIGNED(pos, nvmem->stride))
> +		return -EINVAL;
> +
>   	if (count < nvmem->word_size)
>   		return -EINVAL;
>   
> 

Regards,
Ravi Kumar.B
-- 
Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc. is a 
member of the Code Aurora Forum, hosted by the Linux Foundation.

Re: [PATCH] nvmem: Enforce nvmem stride in the sysfs interface

[Srinivas Kandagatla]

On 29/05/2020 00:53, Douglas Anderson wrote:
> The 'struct nvmem_config' has a stride attribute that specifies the
> needed alignment for accesses into the nvmem.  This is used in
> nvmem_cell_info_to_nvmem_cell() but not in the sysfs read/write
> functions.  If the alignment is important in one place it's important
> everywhere, so let's add enforcement.
> 
> For now we'll consider it totally invalid to access with the wrong
> alignment.  We could relax this in the read case where we could just
> read some extra bytes and throw them away.  Relaxing it in the write
> case seems harder (and less safe?) since we'd have to read some data
> first and then write it back.  To keep it symmetric we'll just
> disallow it in both cases.
> 
> Reported-by: Ravi Kumar Bokka <rbokka@codeaurora.org>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> 
>   drivers/nvmem/core.c | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> index 05c6ae4b0b97..1c0e7953f90d 100644
> --- a/drivers/nvmem/core.c
> +++ b/drivers/nvmem/core.c


Thanks Doug, This was something I wanted to streamline on all the code 
paths, but never got to it!

Applied Thanks,
srini

> @@ -111,6 +111,9 @@ static ssize_t bin_attr_nvmem_read(struct file *filp, struct kobject *kobj,
>   	if (pos >= nvmem->size)
>   		return 0;
>   
> +	if (!IS_ALIGNED(pos, nvmem->stride))
> +		return -EINVAL;
> +
>   	if (count < nvmem->word_size)
>   		return -EINVAL;
>   
> @@ -148,6 +151,9 @@ static ssize_t bin_attr_nvmem_write(struct file *filp, struct kobject *kobj,
>   	if (pos >= nvmem->size)
>   		return -EFBIG;
>   
> +	if (!IS_ALIGNED(pos, nvmem->stride))
> +		return -EINVAL;
> +
>   	if (count < nvmem->word_size)
>   		return -EINVAL;
>   
>