| Message ID | 20200526120557.26240-1-p.yadav@ti.com |
|---|---|
| State | Accepted |
| Commit | b59889bf344dd261b50d69a5eacfa2874573c7cd |
| Delegated to: | Simon Glass |
| Headers | show |
| Series | regmap: Check for out-of-range offsets before mapping them | expand |
Hi Pratyush, On Tue, 26 May 2020 at 06:06, Pratyush Yadav <p.yadav@ti.com> wrote: > > In regmap_raw_{read,write}_range(), offsets are checked to make sure > they aren't out of range. But this check happens _after_ the address is > mapped from physical memory. Input should be sanity-checked before using > it. Mapping the address before validating it leaves the door open to > passing an invalid address to map_physmem(). So check for out of range > offsets _before_ mapping them. > > This fixes a segmentation fault in sandbox when -1 is used as an offset > to regmap_{read,write}(). > > Signed-off-by: Pratyush Yadav <p.yadav@ti.com> > --- > drivers/core/regmap.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > Reviewed-by: Simon Glass <sjg@chromium.org> Please add a sandbox test to catch this problem. Regards, Simon
Hi Simon, On 07/06/20 08:43PM, Simon Glass wrote: > Hi Pratyush, > > On Tue, 26 May 2020 at 06:06, Pratyush Yadav <p.yadav@ti.com> wrote: > > > > In regmap_raw_{read,write}_range(), offsets are checked to make sure > > they aren't out of range. But this check happens _after_ the address is > > mapped from physical memory. Input should be sanity-checked before using > > it. Mapping the address before validating it leaves the door open to > > passing an invalid address to map_physmem(). So check for out of range > > offsets _before_ mapping them. > > > > This fixes a segmentation fault in sandbox when -1 is used as an offset > > to regmap_{read,write}(). > > > > Signed-off-by: Pratyush Yadav <p.yadav@ti.com> > > --- > > drivers/core/regmap.c | 8 ++++---- > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > Reviewed-by: Simon Glass <sjg@chromium.org> Thanks. > Please add a sandbox test to catch this problem. The test "dm_test_devm_regmap" proposed in [0] should catch this: ut_asserteq(-ERANGE, regmap_write(priv->cfg_regmap, -1, val)); ut_asserteq(-ERANGE, regmap_read(priv->cfg_regmap, -1, &val)); [0] https://patchwork.ozlabs.org/project/uboot/patch/20200605203025.15466-9-p.yadav@ti.com/
Hi Simon, On 07/06/20 08:43PM, Simon Glass wrote: > Hi Pratyush, > > On Tue, 26 May 2020 at 06:06, Pratyush Yadav <p.yadav@ti.com> wrote: > > > > In regmap_raw_{read,write}_range(), offsets are checked to make sure > > they aren't out of range. But this check happens _after_ the address is > > mapped from physical memory. Input should be sanity-checked before using > > it. Mapping the address before validating it leaves the door open to > > passing an invalid address to map_physmem(). So check for out of range > > offsets _before_ mapping them. > > > > This fixes a segmentation fault in sandbox when -1 is used as an offset > > to regmap_{read,write}(). > > > > Signed-off-by: Pratyush Yadav <p.yadav@ti.com> > > --- > > drivers/core/regmap.c | 8 ++++---- > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > Reviewed-by: Simon Glass <sjg@chromium.org> Thanks. > Please add a sandbox test to catch this problem. The test "dm_test_devm_regmap" proposed in [0] should catch this: ut_asserteq(-ERANGE, regmap_write(priv->cfg_regmap, -1, val)); ut_asserteq(-ERANGE, regmap_read(priv->cfg_regmap, -1, &val)); [0] https://patchwork.ozlabs.org/project/uboot/patch/20200605203025.15466-9-p.yadav@ti.com/
On 13/06/20 03:11AM, sjg@google.com wrote: > Hi Simon, > > On 07/06/20 08:43PM, Simon Glass wrote: > > Hi Pratyush, > > > > On Tue, 26 May 2020 at 06:06, Pratyush Yadav <p.yadav@ti.com> wrote: > > > > > > In regmap_raw_{read,write}_range(), offsets are checked to make sure > > > they aren't out of range. But this check happens _after_ the address is > > > mapped from physical memory. Input should be sanity-checked before using > > > it. Mapping the address before validating it leaves the door open to > > > passing an invalid address to map_physmem(). So check for out of range > > > offsets _before_ mapping them. > > > > > > This fixes a segmentation fault in sandbox when -1 is used as an offset > > > to regmap_{read,write}(). > > > > > > Signed-off-by: Pratyush Yadav <p.yadav@ti.com> > > > --- > > > drivers/core/regmap.c | 8 ++++---- > > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > > > > Reviewed-by: Simon Glass <sjg@chromium.org> > > Thanks. > > > Please add a sandbox test to catch this problem. > > The test "dm_test_devm_regmap" proposed in [0] should catch this: > > ut_asserteq(-ERANGE, regmap_write(priv->cfg_regmap, -1, val)); > ut_asserteq(-ERANGE, regmap_read(priv->cfg_regmap, -1, &val)); > > [0] https://patchwork.ozlabs.org/project/uboot/patch/20200605203025.15466-9-p.yadav@ti.com/ > > -- > Regards, > Pratyush Yadav > Texas Instruments India > > Applied to u-boot-dm, thanks! Thanks. BTW, your script seems to be misbehaving. It didn't quote the text in my original email with "> ".
Hi Pratyush, On Mon, 15 Jun 2020 at 09:11, Pratyush Yadav <p.yadav@ti.com> wrote: > > On 13/06/20 03:11AM, sjg@google.com wrote: > > Hi Simon, > > > > On 07/06/20 08:43PM, Simon Glass wrote: > > > Hi Pratyush, > > > > > > On Tue, 26 May 2020 at 06:06, Pratyush Yadav <p.yadav@ti.com> wrote: > > > > > > > > In regmap_raw_{read,write}_range(), offsets are checked to make sure > > > > they aren't out of range. But this check happens _after_ the address is > > > > mapped from physical memory. Input should be sanity-checked before using > > > > it. Mapping the address before validating it leaves the door open to > > > > passing an invalid address to map_physmem(). So check for out of range > > > > offsets _before_ mapping them. > > > > > > > > This fixes a segmentation fault in sandbox when -1 is used as an offset > > > > to regmap_{read,write}(). > > > > > > > > Signed-off-by: Pratyush Yadav <p.yadav@ti.com> > > > > --- > > > > drivers/core/regmap.c | 8 ++++---- > > > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > > > > > > > Reviewed-by: Simon Glass <sjg@chromium.org> > > > > Thanks. > > > > > Please add a sandbox test to catch this problem. > > > > The test "dm_test_devm_regmap" proposed in [0] should catch this: > > > > ut_asserteq(-ERANGE, regmap_write(priv->cfg_regmap, -1, val)); > > ut_asserteq(-ERANGE, regmap_read(priv->cfg_regmap, -1, &val)); > > > > [0] https://patchwork.ozlabs.org/project/uboot/patch/20200605203025.15466-9-p.yadav@ti.com/ > > > > -- > > Regards, > > Pratyush Yadav > > Texas Instruments India > > > > Applied to u-boot-dm, thanks! > > Thanks. BTW, your script seems to be misbehaving. It didn't quote the > text in my original email with "> ". Oh dear, and it didn't parse it correctly either. I suppose at some point I'll take another look at it. Thanks for letting me know. Regards, Simon
diff --git a/drivers/core/regmap.c b/drivers/core/regmap.c index 4a214eff7c..a67a237b88 100644 --- a/drivers/core/regmap.c +++ b/drivers/core/regmap.c @@ -310,13 +310,13 @@ int regmap_raw_read_range(struct regmap *map, uint range_num, uint offset, } range = &map->ranges[range_num]; - ptr = map_physmem(range->start + offset, val_len, MAP_NOCACHE); - if (offset + val_len > range->size) { debug("%s: offset/size combination invalid\n", __func__); return -ERANGE; } + ptr = map_physmem(range->start + offset, val_len, MAP_NOCACHE); + switch (val_len) { case REGMAP_SIZE_8: *((u8 *)valp) = __read_8(ptr, map->endianness); @@ -419,13 +419,13 @@ int regmap_raw_write_range(struct regmap *map, uint range_num, uint offset, } range = &map->ranges[range_num]; - ptr = map_physmem(range->start + offset, val_len, MAP_NOCACHE); - if (offset + val_len > range->size) { debug("%s: offset/size combination invalid\n", __func__); return -ERANGE; } + ptr = map_physmem(range->start + offset, val_len, MAP_NOCACHE); + switch (val_len) { case REGMAP_SIZE_8: __write_8(ptr, val, map->endianness);
In regmap_raw_{read,write}_range(), offsets are checked to make sure they aren't out of range. But this check happens _after_ the address is mapped from physical memory. Input should be sanity-checked before using it. Mapping the address before validating it leaves the door open to passing an invalid address to map_physmem(). So check for out of range offsets _before_ mapping them. This fixes a segmentation fault in sandbox when -1 is used as an offset to regmap_{read,write}(). Signed-off-by: Pratyush Yadav <p.yadav@ti.com> --- drivers/core/regmap.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)