| Message ID | 20230206054326.89323-1-k1rh4.lee@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | fpga: dfl-afu-region: Add overflow checks for region size and offset | expand |
On 2/5/23 21:43, k1rh4.lee@gmail.com wrote: > From: Sangsup Lee <k1rh4.lee@gmail.com> > > The size + offset is able to be integer overflow value and it lead to mis-allocate region. Reviewed-by: Russ Weight <russell.h.weight@intel.com> > Signed-off-by: Sangsup Lee <k1rh4.lee@gmail.com> > --- > drivers/fpga/dfl-afu-region.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/drivers/fpga/dfl-afu-region.c b/drivers/fpga/dfl-afu-region.c > index 2e7b41629406..82b530111601 100644 > --- a/drivers/fpga/dfl-afu-region.c > +++ b/drivers/fpga/dfl-afu-region.c > @@ -151,12 +151,17 @@ int afu_mmio_region_get_by_offset(struct dfl_feature_platform_data *pdata, > struct dfl_afu_mmio_region *region; > struct dfl_afu *afu; > int ret = 0; > + u64 region_size = 0; > > mutex_lock(&pdata->lock); > + if (check_add_overflow(offset, size, ®ion_size)) { > + ret = -EINVAL; > + goto exit; > + } > afu = dfl_fpga_pdata_get_private(pdata); > for_each_region(region, afu) > if (region->offset <= offset && > - region->offset + region->size >= offset + size) { > + region->offset + region->size >= region_size) { > *pregion = *region; > goto exit; > }
On 2023-02-05 at 21:43:26 -0800, k1rh4.lee@gmail.com wrote: > From: Sangsup Lee <k1rh4.lee@gmail.com> > > The size + offset is able to be integer overflow value and it lead to mis-allocate region. But I didn't see the memory allocation. > > Signed-off-by: Sangsup Lee <k1rh4.lee@gmail.com> > --- > drivers/fpga/dfl-afu-region.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/drivers/fpga/dfl-afu-region.c b/drivers/fpga/dfl-afu-region.c > index 2e7b41629406..82b530111601 100644 > --- a/drivers/fpga/dfl-afu-region.c > +++ b/drivers/fpga/dfl-afu-region.c > @@ -151,12 +151,17 @@ int afu_mmio_region_get_by_offset(struct dfl_feature_platform_data *pdata, > struct dfl_afu_mmio_region *region; > struct dfl_afu *afu; > int ret = 0; > + u64 region_size = 0; Reverse Xmax tree please. > > mutex_lock(&pdata->lock); > + if (check_add_overflow(offset, size, ®ion_size)) { I'm not sure if the check is necessary. The offset comes from: offset = vma->vm_pgoff << PAGE_SHIFT The size comes from: size = vma->vm_end - vma->vm_start Is it possible the upper mm layer passes invalid vma? Thanks, Yilun > + ret = -EINVAL; > + goto exit; > + } > afu = dfl_fpga_pdata_get_private(pdata); > for_each_region(region, afu) > if (region->offset <= offset && > - region->offset + region->size >= offset + size) { > + region->offset + region->size >= region_size) { > *pregion = *region; > goto exit; > } > -- > 2.25.1 >
Hi, On Fri, Feb 10, 2023 at 04:21:37PM +0800, Xu Yilun wrote: > On 2023-02-05 at 21:43:26 -0800, k1rh4.lee@gmail.com wrote: > > From: Sangsup Lee <k1rh4.lee@gmail.com> > > > > The size + offset is able to be integer overflow value and it lead to mis-allocate region. > > But I didn't see the memory allocation. > > > > > Signed-off-by: Sangsup Lee <k1rh4.lee@gmail.com> > > --- > > drivers/fpga/dfl-afu-region.c | 7 ++++++- > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/fpga/dfl-afu-region.c b/drivers/fpga/dfl-afu-region.c > > index 2e7b41629406..82b530111601 100644 > > --- a/drivers/fpga/dfl-afu-region.c > > +++ b/drivers/fpga/dfl-afu-region.c > > @@ -151,12 +151,17 @@ int afu_mmio_region_get_by_offset(struct dfl_feature_platform_data *pdata, > > struct dfl_afu_mmio_region *region; > > struct dfl_afu *afu; > > int ret = 0; > > + u64 region_size = 0; > > Reverse Xmax tree please. > > > > > mutex_lock(&pdata->lock); > > + if (check_add_overflow(offset, size, ®ion_size)) { > > I'm not sure if the check is necessary. > > The offset comes from: offset = vma->vm_pgoff << PAGE_SHIFT > The size comes from: size = vma->vm_end - vma->vm_start > Is it possible the upper mm layer passes invalid vma? > > Thanks, > Yilun Did you saw the comments on your patch by Yilun? Did it felt trough the cracks? Regards, Salvatore
Hi, In my opinion the code has an insecure code pattern. The size may have integer overflow condition i think. But, I did not do dynamic analysis but I did static audit fpga code(I don't have an fpga device). because of this. I don't make sure about Yilun's comment. I think the code must have defensive coding rules. best regards.
diff --git a/drivers/fpga/dfl-afu-region.c b/drivers/fpga/dfl-afu-region.c index 2e7b41629406..82b530111601 100644 --- a/drivers/fpga/dfl-afu-region.c +++ b/drivers/fpga/dfl-afu-region.c @@ -151,12 +151,17 @@ int afu_mmio_region_get_by_offset(struct dfl_feature_platform_data *pdata, struct dfl_afu_mmio_region *region; struct dfl_afu *afu; int ret = 0; + u64 region_size = 0; mutex_lock(&pdata->lock); + if (check_add_overflow(offset, size, ®ion_size)) { + ret = -EINVAL; + goto exit; + } afu = dfl_fpga_pdata_get_private(pdata); for_each_region(region, afu) if (region->offset <= offset && - region->offset + region->size >= offset + size) { + region->offset + region->size >= region_size) { *pregion = *region; goto exit; }