| Message ID | 20230206020716.2036-1-zhongjinghua@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [PATCH-next,v2] loop: loop_set_status_from_info() check before assignment | expand |
On 2/6/2023 10:07 AM, Zhong Jinghua wrote: > In loop_set_status_from_info(), lo->lo_offset and lo->lo_sizelimit should > be checked before reassignment, because if an overflow error occurs, the > original correct value will be changed to the wrong value, and it will not > be changed back. > > Modifying to the wrong value logic is always not quiet right, we hope to > optimize this. > > Signed-off-by: Zhong Jinghua <zhongjinghua@huawei.com> LGTM > --- > v1->v2: Modify note: overflowing -> overflow > drivers/block/loop.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/block/loop.c b/drivers/block/loop.c > index 1518a6423279..1b35cbd029c7 100644 > --- a/drivers/block/loop.c > +++ b/drivers/block/loop.c > @@ -977,13 +977,13 @@ loop_set_status_from_info(struct loop_device *lo, > return -EINVAL; > } > > + /* Avoid assigning overflow values */ > + if (info->lo_offset > LLONG_MAX || info->lo_sizelimit > LLONG_MAX) > + return -EOVERFLOW; > + > lo->lo_offset = info->lo_offset; > lo->lo_sizelimit = info->lo_sizelimit; > > - /* loff_t vars have been assigned __u64 */ > - if (lo->lo_offset < 0 || lo->lo_sizelimit < 0) > - return -EOVERFLOW; > - > memcpy(lo->lo_file_name, info->lo_file_name, LO_NAME_SIZE); > lo->lo_file_name[LO_NAME_SIZE-1] = 0; > lo->lo_flags = info->lo_flags;
Please disregard my reply. I did not notice it is not an internal email between our team members. On 2/6/2023 10:55 AM, Hou Tao wrote: > > On 2/6/2023 10:07 AM, Zhong Jinghua wrote: >> In loop_set_status_from_info(), lo->lo_offset and lo->lo_sizelimit should >> be checked before reassignment, because if an overflow error occurs, the >> original correct value will be changed to the wrong value, and it will not >> be changed back. >> >> Modifying to the wrong value logic is always not quiet right, we hope to >> optimize this. >> >> Signed-off-by: Zhong Jinghua <zhongjinghua@huawei.com> > LGTM >> --- >> v1->v2: Modify note: overflowing -> overflow >> drivers/block/loop.c | 8 ++++---- >> 1 file changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/block/loop.c b/drivers/block/loop.c >> index 1518a6423279..1b35cbd029c7 100644 >> --- a/drivers/block/loop.c >> +++ b/drivers/block/loop.c >> @@ -977,13 +977,13 @@ loop_set_status_from_info(struct loop_device *lo, >> return -EINVAL; >> } >> >> + /* Avoid assigning overflow values */ >> + if (info->lo_offset > LLONG_MAX || info->lo_sizelimit > LLONG_MAX) >> + return -EOVERFLOW; >> + >> lo->lo_offset = info->lo_offset; >> lo->lo_sizelimit = info->lo_sizelimit; >> >> - /* loff_t vars have been assigned __u64 */ >> - if (lo->lo_offset < 0 || lo->lo_sizelimit < 0) >> - return -EOVERFLOW; >> - >> memcpy(lo->lo_file_name, info->lo_file_name, LO_NAME_SIZE); >> lo->lo_file_name[LO_NAME_SIZE-1] = 0; >> lo->lo_flags = info->lo_flags;
Hi, 在 2023/02/06 10:07, Zhong Jinghua 写道: > In loop_set_status_from_info(), lo->lo_offset and lo->lo_sizelimit should > be checked before reassignment, because if an overflow error occurs, the > original correct value will be changed to the wrong value, and it will not > be changed back. > > Modifying to the wrong value logic is always not quiet right, we hope to > optimize this. > Please add a fix tag and cc stable: Fixes: c490a0b5a4f3 ("loop: Check for overflow while configuring loop") This commit doesn't fix the problem it described in commit message. Thanks, Kuai > Signed-off-by: Zhong Jinghua <zhongjinghua@huawei.com> > --- > v1->v2: Modify note: overflowing -> overflow > drivers/block/loop.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/block/loop.c b/drivers/block/loop.c > index 1518a6423279..1b35cbd029c7 100644 > --- a/drivers/block/loop.c > +++ b/drivers/block/loop.c > @@ -977,13 +977,13 @@ loop_set_status_from_info(struct loop_device *lo, > return -EINVAL; > } > > + /* Avoid assigning overflow values */ > + if (info->lo_offset > LLONG_MAX || info->lo_sizelimit > LLONG_MAX) > + return -EOVERFLOW; > + > lo->lo_offset = info->lo_offset; > lo->lo_sizelimit = info->lo_sizelimit; > > - /* loff_t vars have been assigned __u64 */ > - if (lo->lo_offset < 0 || lo->lo_sizelimit < 0) > - return -EOVERFLOW; > - > memcpy(lo->lo_file_name, info->lo_file_name, LO_NAME_SIZE); > lo->lo_file_name[LO_NAME_SIZE-1] = 0; > lo->lo_flags = info->lo_flags; >
diff --git a/drivers/block/loop.c b/drivers/block/loop.c index 1518a6423279..1b35cbd029c7 100644 --- a/drivers/block/loop.c +++ b/drivers/block/loop.c @@ -977,13 +977,13 @@ loop_set_status_from_info(struct loop_device *lo, return -EINVAL; } + /* Avoid assigning overflow values */ + if (info->lo_offset > LLONG_MAX || info->lo_sizelimit > LLONG_MAX) + return -EOVERFLOW; + lo->lo_offset = info->lo_offset; lo->lo_sizelimit = info->lo_sizelimit; - /* loff_t vars have been assigned __u64 */ - if (lo->lo_offset < 0 || lo->lo_sizelimit < 0) - return -EOVERFLOW; - memcpy(lo->lo_file_name, info->lo_file_name, LO_NAME_SIZE); lo->lo_file_name[LO_NAME_SIZE-1] = 0; lo->lo_flags = info->lo_flags;
In loop_set_status_from_info(), lo->lo_offset and lo->lo_sizelimit should be checked before reassignment, because if an overflow error occurs, the original correct value will be changed to the wrong value, and it will not be changed back. Modifying to the wrong value logic is always not quiet right, we hope to optimize this. Signed-off-by: Zhong Jinghua <zhongjinghua@huawei.com> --- v1->v2: Modify note: overflowing -> overflow drivers/block/loop.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)