| Message ID | 20200324153630.11882-3-vsementsov@virtuozzo.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Several error use-after-free | expand |
On 3/24/20 10:36 AM, Vladimir Sementsov-Ogievskiy wrote: > local_err is used again in mirror_exit_common() after > bdrv_set_backing_hd(), so we must zero it. Otherwise try to set > non-NULL local_err will crash. > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- > block/mirror.c | 1 + > 1 file changed, 1 insertion(+) Reviewed-by: Eric Blake <eblake@redhat.com> > > diff --git a/block/mirror.c b/block/mirror.c > index 447051dbc6..6203e5946e 100644 > --- a/block/mirror.c > +++ b/block/mirror.c > @@ -678,6 +678,7 @@ static int mirror_exit_common(Job *job) > bdrv_set_backing_hd(target_bs, backing, &local_err); > if (local_err) { > error_report_err(local_err); > + local_err = NULL; > ret = -EPERM; > } > } >
On 3/24/20 11:36 AM, Vladimir Sementsov-Ogievskiy wrote: > local_err is used again in mirror_exit_common() after > bdrv_set_backing_hd(), so we must zero it. Otherwise try to set > non-NULL local_err will crash. > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- > block/mirror.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/block/mirror.c b/block/mirror.c > index 447051dbc6..6203e5946e 100644 > --- a/block/mirror.c > +++ b/block/mirror.c > @@ -678,6 +678,7 @@ static int mirror_exit_common(Job *job) > bdrv_set_backing_hd(target_bs, backing, &local_err); > if (local_err) { > error_report_err(local_err); > + local_err = NULL; > ret = -EPERM; > } > } > Reviewed-by: John Snow <jsnow@redhat.com>
On 24.03.20 16:36, Vladimir Sementsov-Ogievskiy wrote: > local_err is used again in mirror_exit_common() after > bdrv_set_backing_hd(), so we must zero it. Otherwise try to set > non-NULL local_err will crash. OK, but wouldn’t it be better hygiene to set it to NULL every time it is freed? (There is a second instance of error_report_err() in this function. I’m a bit worried we might introduce another local_err use after that one at some point in the future, and forget to run the cocci script then.) Are the cocci scripts run regularly by someone? E.g. as part of a pull to master? Max > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- > block/mirror.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/block/mirror.c b/block/mirror.c > index 447051dbc6..6203e5946e 100644 > --- a/block/mirror.c > +++ b/block/mirror.c > @@ -678,6 +678,7 @@ static int mirror_exit_common(Job *job) > bdrv_set_backing_hd(target_bs, backing, &local_err); > if (local_err) { > error_report_err(local_err); > + local_err = NULL; > ret = -EPERM; > } > } >
On 25.03.20 12:11, Max Reitz wrote: > On 24.03.20 16:36, Vladimir Sementsov-Ogievskiy wrote: >> local_err is used again in mirror_exit_common() after >> bdrv_set_backing_hd(), so we must zero it. Otherwise try to set >> non-NULL local_err will crash. > > OK, but wouldn’t it be better hygiene to set it to NULL every time it is > freed? (There is a second instance of error_report_err() in this > function. I’m a bit worried we might introduce another local_err use > after that one at some point in the future, and forget to run the cocci > script then.) > > Are the cocci scripts run regularly by someone? E.g. as part of a pull > to master? Doesn’t look like it. I’m currently running everything, and there’s a lot of results so far. Max
25.03.2020 14:11, Max Reitz wrote: > On 24.03.20 16:36, Vladimir Sementsov-Ogievskiy wrote: >> local_err is used again in mirror_exit_common() after >> bdrv_set_backing_hd(), so we must zero it. Otherwise try to set >> non-NULL local_err will crash. > > OK, but wouldn’t it be better hygiene to set it to NULL every time it is > freed? (There is a second instance of error_report_err() in this > function. I’m a bit worried we might introduce another local_err use > after that one at some point in the future, and forget to run the cocci > script then.) Yes, it's better. But if we now decide to fix all such things, it would be huge series. May be too huge for 5.0.. So I decided to fix only real obvious problems now. Hmm huge or not? Ok, let's try with less restrictions: --- a/scripts/coccinelle/error-use-after-free.cocci +++ b/scripts/coccinelle/error-use-after-free.cocci @@ -28,7 +28,7 @@ expression err; fn(...) { - <... + ... when any ( error_free(err); + err = NULL; @@ -46,7 +46,5 @@ expression err; + err = NULL; ) ... when != err = NULL - when != exit(...) - fn2(..., err, ...) - ...> + when any } on block/ directory: spatch --sp-file scripts/coccinelle/error-use-after-free.cocci --macro-file scripts/cocci-macro-file.h --in-place --no-show-diff --use-gitgrep block git diff --stat scripts/coccinelle/error-use-after-free.cocci | 6 ++---- block/block-backend.c | 1 + block/commit.c | 4 ++++ block/crypto.c | 1 + block/file-posix.c | 5 +++++ block/mirror.c | 2 ++ block/monitor/block-hmp-cmds.c | 4 ++++ block/parallels.c | 3 +++ block/qapi-sysemu.c | 2 ++ block/qapi.c | 1 + block/qcow.c | 2 ++ block/qcow2-cluster.c | 1 + block/qcow2-refcount.c | 1 + block/qcow2-snapshot.c | 3 +++ block/qcow2.c | 4 ++++ block/replication.c | 1 + block/sheepdog.c | 13 +++++++++++++ block/stream.c | 1 + block/vdi.c | 2 ++ block/vhdx.c | 2 ++ block/vmdk.c | 2 ++ block/vpc.c | 2 ++ block/vvfat.c | 2 ++ 23 files changed, 61 insertions(+), 4 deletions(-) If you want, I'll send series. > > Are the cocci scripts run regularly by someone? E.g. as part of a pull > to master? I'm afraid not. I have a plan in my mind, to make python checkcode, which will work in pair with checkpatch somehow, and will work on workdir instead of patch. It will simplify significantly adding different code checks, including starting coccinelle scripts. > > Max > >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >> --- >> block/mirror.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/block/mirror.c b/block/mirror.c >> index 447051dbc6..6203e5946e 100644 >> --- a/block/mirror.c >> +++ b/block/mirror.c >> @@ -678,6 +678,7 @@ static int mirror_exit_common(Job *job) >> bdrv_set_backing_hd(target_bs, backing, &local_err); >> if (local_err) { >> error_report_err(local_err); >> + local_err = NULL; >> ret = -EPERM; >> } >> } >> > >
On 25.03.20 12:47, Vladimir Sementsov-Ogievskiy wrote: > 25.03.2020 14:11, Max Reitz wrote: >> On 24.03.20 16:36, Vladimir Sementsov-Ogievskiy wrote: >>> local_err is used again in mirror_exit_common() after >>> bdrv_set_backing_hd(), so we must zero it. Otherwise try to set >>> non-NULL local_err will crash. >> >> OK, but wouldn’t it be better hygiene to set it to NULL every time it is >> freed? (There is a second instance of error_report_err() in this >> function. I’m a bit worried we might introduce another local_err use >> after that one at some point in the future, and forget to run the cocci >> script then.) > > Yes, it's better. But if we now decide to fix all such things, it would be > huge series. May be too huge for 5.0.. > > So I decided to fix only real obvious problems now. Reasonable, yes. > Hmm huge or not? > > Ok, let's try with less restrictions: > > --- a/scripts/coccinelle/error-use-after-free.cocci > +++ b/scripts/coccinelle/error-use-after-free.cocci > @@ -28,7 +28,7 @@ expression err; > > fn(...) > { > - <... > + ... when any > ( > error_free(err); > + err = NULL; > @@ -46,7 +46,5 @@ expression err; > + err = NULL; > ) > ... when != err = NULL > - when != exit(...) > - fn2(..., err, ...) > - ...> > + when any > } > > > on block/ directory: > > spatch --sp-file scripts/coccinelle/error-use-after-free.cocci > --macro-file scripts/cocci-macro-file.h --in-place --no-show-diff > --use-gitgrep block > git diff --stat > scripts/coccinelle/error-use-after-free.cocci | 6 ++---- > block/block-backend.c | 1 + > block/commit.c | 4 ++++ > block/crypto.c | 1 + > block/file-posix.c | 5 +++++ > block/mirror.c | 2 ++ > block/monitor/block-hmp-cmds.c | 4 ++++ > block/parallels.c | 3 +++ > block/qapi-sysemu.c | 2 ++ > block/qapi.c | 1 + > block/qcow.c | 2 ++ > block/qcow2-cluster.c | 1 + > block/qcow2-refcount.c | 1 + > block/qcow2-snapshot.c | 3 +++ > block/qcow2.c | 4 ++++ > block/replication.c | 1 + > block/sheepdog.c | 13 +++++++++++++ > block/stream.c | 1 + > block/vdi.c | 2 ++ > block/vhdx.c | 2 ++ > block/vmdk.c | 2 ++ > block/vpc.c | 2 ++ > block/vvfat.c | 2 ++ > 23 files changed, 61 insertions(+), 4 deletions(-) > > > If you want, I'll send series. > >> >> Are the cocci scripts run regularly by someone? E.g. as part of a pull >> to master? > > I'm afraid not. I have a plan in my mind, to make python checkcode, > which will > work in pair with checkpatch somehow, and will work on workdir instead of > patch. It will simplify significantly adding different code checks, > including > starting coccinelle scripts. Hm. I think we need to prepare for noone running the cocci scripts (i.e., we should use the above variant with less restrictions so that future patches are less likely to reintroduce the problem), or we need to ensure the cocci scripts are run regularly. We can of course also do both. In this case I think it makes sense to do a less-restricted version, because I think it can never hurt to set pointers to NULL after freeing them. (We could do an exception for when the error-freeing is immediately followed by a goto out, but I think that would make it too complicated.) I’d like to start running the cocci scripts myself before every pull request, but unfortunately there are still a number of diffs in the block area. I think I’ll send a series to fix those and then I can run the scripts regularly to prevent regressions. So I’ll leave it up to you whether you think a less-restricted version would make sense. Max
On 24.03.20 16:36, Vladimir Sementsov-Ogievskiy wrote: > local_err is used again in mirror_exit_common() after > bdrv_set_backing_hd(), so we must zero it. Otherwise try to set > non-NULL local_err will crash. > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- > block/mirror.c | 1 + > 1 file changed, 1 insertion(+) Considering Dave has taken patches 4 and 5, I think it makes sense for me to take this one now; so, thanks for the patch and the reviews, applied to my block branch: https://git.xanclic.moe/XanClic/qemu/commits/branch/block Max
On 3/25/20 6:11 AM, Max Reitz wrote: > On 24.03.20 16:36, Vladimir Sementsov-Ogievskiy wrote: >> local_err is used again in mirror_exit_common() after >> bdrv_set_backing_hd(), so we must zero it. Otherwise try to set >> non-NULL local_err will crash. > > OK, but wouldn’t it be better hygiene to set it to NULL every time it is > freed? If we change the signature to error_report_err(&local_err), where error_report_err both reports the error (if any) AND sets local_err to NULL, then we fix the problem for all callers. It's a global search-and-replace job (Coccinelle is great for that) to update all callers to the new signature. > (There is a second instance of error_report_err() in this > function. I’m a bit worried we might introduce another local_err use > after that one at some point in the future, and forget to run the cocci > script then.) > > Are the cocci scripts run regularly by someone? E.g. as part of a pull > to master? I'm not aware of any automated procedure for it at the moment; rather, it is still ad hoc as someone notices something needs to be re-run. But there was another thread about someone considering automating Cocci scripts as part of the Euler robot...
Max Reitz <mreitz@redhat.com> writes: > On 25.03.20 12:47, Vladimir Sementsov-Ogievskiy wrote: >> 25.03.2020 14:11, Max Reitz wrote: >>> On 24.03.20 16:36, Vladimir Sementsov-Ogievskiy wrote: >>>> local_err is used again in mirror_exit_common() after >>>> bdrv_set_backing_hd(), so we must zero it. Otherwise try to set >>>> non-NULL local_err will crash. >>> >>> OK, but wouldn’t it be better hygiene to set it to NULL every time it is >>> freed? (There is a second instance of error_report_err() in this >>> function. I’m a bit worried we might introduce another local_err use >>> after that one at some point in the future, and forget to run the cocci >>> script then.) >> >> Yes, it's better. But if we now decide to fix all such things, it would be >> huge series. May be too huge for 5.0.. >> >> So I decided to fix only real obvious problems now. > > Reasonable, yes. In particular since we have a tree-wide transformation waiting for 5.1. >> Hmm huge or not? >> >> Ok, let's try with less restrictions: >> >> --- a/scripts/coccinelle/error-use-after-free.cocci >> +++ b/scripts/coccinelle/error-use-after-free.cocci >> @@ -28,7 +28,7 @@ expression err; >> >> fn(...) >> { >> - <... >> + ... when any >> ( >> error_free(err); >> + err = NULL; >> @@ -46,7 +46,5 @@ expression err; >> + err = NULL; >> ) >> ... when != err = NULL >> - when != exit(...) >> - fn2(..., err, ...) >> - ...> >> + when any >> } >> >> >> on block/ directory: >> >> spatch --sp-file scripts/coccinelle/error-use-after-free.cocci >> --macro-file scripts/cocci-macro-file.h --in-place --no-show-diff >> --use-gitgrep block >> git diff --stat >> scripts/coccinelle/error-use-after-free.cocci | 6 ++---- >> block/block-backend.c | 1 + >> block/commit.c | 4 ++++ >> block/crypto.c | 1 + >> block/file-posix.c | 5 +++++ >> block/mirror.c | 2 ++ >> block/monitor/block-hmp-cmds.c | 4 ++++ >> block/parallels.c | 3 +++ >> block/qapi-sysemu.c | 2 ++ >> block/qapi.c | 1 + >> block/qcow.c | 2 ++ >> block/qcow2-cluster.c | 1 + >> block/qcow2-refcount.c | 1 + >> block/qcow2-snapshot.c | 3 +++ >> block/qcow2.c | 4 ++++ >> block/replication.c | 1 + >> block/sheepdog.c | 13 +++++++++++++ >> block/stream.c | 1 + >> block/vdi.c | 2 ++ >> block/vhdx.c | 2 ++ >> block/vmdk.c | 2 ++ >> block/vpc.c | 2 ++ >> block/vvfat.c | 2 ++ >> 23 files changed, 61 insertions(+), 4 deletions(-) >> >> >> If you want, I'll send series. >> >>> >>> Are the cocci scripts run regularly by someone? E.g. as part of a pull >>> to master? >> >> I'm afraid not. I have a plan in my mind, to make python checkcode, >> which will >> work in pair with checkpatch somehow, and will work on workdir instead of >> patch. It will simplify significantly adding different code checks, >> including >> starting coccinelle scripts. CI also runs make check, so that's another place you can hook into. Not sure Coccinelle is fast enough for running it all the time. > Hm. I think we need to prepare for noone running the cocci scripts > (i.e., we should use the above variant with less restrictions so that > future patches are less likely to reintroduce the problem), or we need > to ensure the cocci scripts are run regularly. > > We can of course also do both. In this case I think it makes sense to > do a less-restricted version, because I think it can never hurt to set > pointers to NULL after freeing them. (We could do an exception for when > the error-freeing is immediately followed by a goto out, but I think > that would make it too complicated.) Same reasoning applies to all kinds of resource deallocation, not just error_free(). Perhaps the world should use g_free() & friends only via g_clear_pointer(). For better or worse, it doesn't. Related: "[PATCH v7 01/11] qapi/error: add (Error **errp) cleaning APIs". > I’d like to start running the cocci scripts myself before every pull > request, but unfortunately there are still a number of diffs in the > block area. I think I’ll send a series to fix those and then I can run > the scripts regularly to prevent regressions. So I’ll leave it up to > you whether you think a less-restricted version would make sense. > > Max
diff --git a/block/mirror.c b/block/mirror.c index 447051dbc6..6203e5946e 100644 --- a/block/mirror.c +++ b/block/mirror.c @@ -678,6 +678,7 @@ static int mirror_exit_common(Job *job) bdrv_set_backing_hd(target_bs, backing, &local_err); if (local_err) { error_report_err(local_err); + local_err = NULL; ret = -EPERM; } }
local_err is used again in mirror_exit_common() after bdrv_set_backing_hd(), so we must zero it. Otherwise try to set non-NULL local_err will crash. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> --- block/mirror.c | 1 + 1 file changed, 1 insertion(+)