nvmem_device_read() could be called directly once nvmem device
registered, the sanity check should be done before call
nvmem_reg_read() as cell and sysfs read did now.
Signed-off-by: Bingbu Cao <[email protected]>
---
drivers/nvmem/core.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index 927eb5f6003f..c9a77993f008 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -1491,6 +1491,13 @@ int nvmem_device_read(struct nvmem_device *nvmem,
if (!nvmem)
return -EINVAL;
+ if (offset >= nvmem->size || bytes < nvmem->word_size)
+ return 0;
+
+ if (bytes + offset > nvmem->size)
+ bytes = nvmem->size - offset;
+
+ bytes = round_down(bytes, nvmem->word_size);
rc = nvmem_reg_read(nvmem, offset, buf, bytes);
if (rc)
--
2.7.4
Hi Bingbu,
Thank you for the patch.
On Tue, Aug 04, 2020 at 05:13:56PM +0800, Bingbu Cao wrote:
> nvmem_device_read() could be called directly once nvmem device
> registered, the sanity check should be done before call
> nvmem_reg_read() as cell and sysfs read did now.
>
> Signed-off-by: Bingbu Cao <[email protected]>
> ---
> drivers/nvmem/core.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> index 927eb5f6003f..c9a77993f008 100644
> --- a/drivers/nvmem/core.c
> +++ b/drivers/nvmem/core.c
> @@ -1491,6 +1491,13 @@ int nvmem_device_read(struct nvmem_device *nvmem,
> if (!nvmem)
> return -EINVAL;
>
> + if (offset >= nvmem->size || bytes < nvmem->word_size)
> + return 0;
> +
> + if (bytes + offset > nvmem->size)
> + bytes = nvmem->size - offset;
The check is relevant for nvmem_device_write(), too.
There are also other ways to access nvmem devices such as nvmem_cell_read
and others alike. Should they be considered as well?
> +
> + bytes = round_down(bytes, nvmem->word_size);
> rc = nvmem_reg_read(nvmem, offset, buf, bytes);
>
> if (rc)
--
Kind regards,
Sakari Ailus
On 04/08/2020 10:58, Sakari Ailus wrote:
> Hi Bingbu,
>
> Thank you for the patch.
>
> On Tue, Aug 04, 2020 at 05:13:56PM +0800, Bingbu Cao wrote:
>> nvmem_device_read() could be called directly once nvmem device
>> registered, the sanity check should be done before call
>> nvmem_reg_read() as cell and sysfs read did now.
>>
>> Signed-off-by: Bingbu Cao <[email protected]>
>> ---
>> drivers/nvmem/core.c | 7 +++++++
>> 1 file changed, 7 insertions(+)
>>
>> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
>> index 927eb5f6003f..c9a77993f008 100644
>> --- a/drivers/nvmem/core.c
>> +++ b/drivers/nvmem/core.c
>> @@ -1491,6 +1491,13 @@ int nvmem_device_read(struct nvmem_device *nvmem,
>> if (!nvmem)
>> return -EINVAL;
>>
>> + if (offset >= nvmem->size || bytes < nvmem->word_size)
>> + return 0;
>> +
>> + if (bytes + offset > nvmem->size)
>> + bytes = nvmem->size - offset;
>
> The check is relevant for nvmem_device_write(), too.
>
> There are also other ways to access nvmem devices such as nvmem_cell_read
> and others alike. Should they be considered as well?
We should probably move these sanity checks to a common place like
nvmem_reg_read() and nvmem_reg_write(), so the callers need not
duplicate the same!
--srini
>
>> +
>> + bytes = round_down(bytes, nvmem->word_size);
>> rc = nvmem_reg_read(nvmem, offset, buf, bytes);
>>
>> if (rc)
>
On 8/4/20 6:03 PM, Srinivas Kandagatla wrote:
>
>
> On 04/08/2020 10:58, Sakari Ailus wrote:
>> Hi Bingbu,
>>
>> Thank you for the patch.
>>
>> On Tue, Aug 04, 2020 at 05:13:56PM +0800, Bingbu Cao wrote:
>>> nvmem_device_read() could be called directly once nvmem device
>>> registered, the sanity check should be done before call
>>> nvmem_reg_read() as cell and sysfs read did now.
>>>
>>> Signed-off-by: Bingbu Cao <[email protected]>
>>> ---
>>> ? drivers/nvmem/core.c | 7 +++++++
>>> ? 1 file changed, 7 insertions(+)
>>>
>>> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
>>> index 927eb5f6003f..c9a77993f008 100644
>>> --- a/drivers/nvmem/core.c
>>> +++ b/drivers/nvmem/core.c
>>> @@ -1491,6 +1491,13 @@ int nvmem_device_read(struct nvmem_device *nvmem,
>>> ????? if (!nvmem)
>>> ????????? return -EINVAL;
>>> ? +??? if (offset >= nvmem->size || bytes < nvmem->word_size)
>>> +??????? return 0;
>>> +
>>> +??? if (bytes + offset > nvmem->size)
>>> +??????? bytes = nvmem->size - offset;
>>
>> The check is relevant for nvmem_device_write(), too.
>>
>> There are also other ways to access nvmem devices such as nvmem_cell_read
>> and others alike. Should they be considered as well?
>
> We should probably move these sanity checks to a common place like
> nvmem_reg_read() and nvmem_reg_write(), so the callers need not duplicate the same!
>
Srini and Sakari, thanks for your review.
Is it OK just return INVAL with simple check like below?
if (bytes + offset > nvmem->size ||
bytes != round_down(bytes, nvmem->word_size))
return -EINVAL;
> --srini
>
>>
>>> +
>>> +??? bytes = round_down(bytes, nvmem->word_size);
>>> ????? rc = nvmem_reg_read(nvmem, offset, buf, bytes);
>>> ? ????? if (rc)
>>
--
Best regards,
Bingbu Cao
On Tue, Aug 04, 2020 at 06:44:27PM +0800, Bingbu Cao wrote:
>
> On 8/4/20 6:03 PM, Srinivas Kandagatla wrote:
> >
> >
> > On 04/08/2020 10:58, Sakari Ailus wrote:
> >> Hi Bingbu,
> >>
> >> Thank you for the patch.
> >>
> >> On Tue, Aug 04, 2020 at 05:13:56PM +0800, Bingbu Cao wrote:
> >>> nvmem_device_read() could be called directly once nvmem device
> >>> registered, the sanity check should be done before call
> >>> nvmem_reg_read() as cell and sysfs read did now.
> >>>
> >>> Signed-off-by: Bingbu Cao <[email protected]>
> >>> ---
> >>> ? drivers/nvmem/core.c | 7 +++++++
> >>> ? 1 file changed, 7 insertions(+)
> >>>
> >>> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> >>> index 927eb5f6003f..c9a77993f008 100644
> >>> --- a/drivers/nvmem/core.c
> >>> +++ b/drivers/nvmem/core.c
> >>> @@ -1491,6 +1491,13 @@ int nvmem_device_read(struct nvmem_device *nvmem,
> >>> ????? if (!nvmem)
> >>> ????????? return -EINVAL;
> >>> ? +??? if (offset >= nvmem->size || bytes < nvmem->word_size)
> >>> +??????? return 0;
> >>> +
> >>> +??? if (bytes + offset > nvmem->size)
> >>> +??????? bytes = nvmem->size - offset;
> >>
> >> The check is relevant for nvmem_device_write(), too.
> >>
> >> There are also other ways to access nvmem devices such as nvmem_cell_read
> >> and others alike. Should they be considered as well?
> >
> > We should probably move these sanity checks to a common place like
> > nvmem_reg_read() and nvmem_reg_write(), so the callers need not duplicate the same!
> >
> Srini and Sakari, thanks for your review.
>
> Is it OK just return INVAL with simple check like below?
>
> if (bytes + offset > nvmem->size ||
> bytes != round_down(bytes, nvmem->word_size))
> return -EINVAL;
This changes what is currently supported so I'd say no.
--
Sakari Ailus