Re: [PATCH 2/6] block/mirror: fix use after free of local_err

[Max Reitz]

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

signature.asc
Description: OpenPGP digital signature