Re: [PATCH] nvmem: core: add sanity check in nvmem_device_read()
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 > >>> --- > >>> 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
Re: [PATCH] nvmem: core: add sanity check in nvmem_device_read()
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 >>> --- >>> 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
Re: [PATCH] nvmem: core: add sanity check in nvmem_device_read()
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 --- 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)
Re: [PATCH] nvmem: core: add sanity check in nvmem_device_read()
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 > --- > 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
[PATCH] nvmem: core: add sanity check in nvmem_device_read()
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 --- 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
