[OpenWrt-Devel] [PATCH uci 2/2] build: Add -Wclobbered to detect problems with longjmp

[Hauke Mehrtens]

On 11/4/19 4:29 AM, Yousong Zhou wrote:
> Hi Hauke
> 
> On Sat, 2 Nov 2019 at 00:07, Hauke Mehrtens <hauke at hauke-m.de> wrote:
>>
>> When we jump back to a save point in UCI_THROW() with longjmp all the
>> registers will be reset to the old values when we called UCI_TRAP_SAVE()
>> last time, but the memory is not restored. This will revert all the
>> variables which are stored in registers, but not the variables stored on
>> the stack.
>>
>> Mark all the variables which the compiler could put into a register as
>> volatile to store them safely on the stack and make sure they have the
>> defined current values also after longjmp was called.
>>
>> This also activates a compiler warning which should warn us in such
>> cases.
>> This could fix some potential problem in error paths like the one
>> reported in CVE-2019-15513.
>>
>> Signed-off-by: Hauke Mehrtens <hauke at hauke-m.de>
> 
> Not sure I understand the internals right.  It seems to me a few
> changes below may not be necessary.  The -Wclobber check can produce
> false-positives right?  Are these changes made mainly for "better safe
> than regret"?

Hi Yousong,

Yes, some of the volatiles are not necessary, but I activated the
warning -Wclobbered and GCC is not so intelligent and warns for all
variables which are written after setjmp() is called.
When we activate the warning everyone gets this warning and no one will
introduce such a problem again, but we probably have volatile also in
some places where it is not needed.

I want to handle this part of the setjmp() manpage:
--------------------------------
The  compiler  may  optimize  variables into registers, and longjmp()
may restore the values of other registers in addition to the stack
pointer and program counter.  Consequently, the values of automatic
variables are unspecified after a call to longjmp() if they meet all the
following criteria:
* they are local to the function that made the corresponding setjmp()
  call;
* their values are changed between the calls to setjmp() and longjmp();
  and
* they are not declared as volatile.
--------------------------------

The GCC documentation says this:
--------------------------------
-Wclobbered
    Warn for variables that might be changed by longjmp or vfork. This
warning is also enabled by -Wextra.
--------------------------------

Hauke

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/openwrt-devel/attachments/20191104/cf61703d/attachment.sig>
-------------- next part --------------
_______________________________________________
openwrt-devel mailing list
openwrt-devel at lists.openwrt.org
https://lists.openwrt.org/mailman/listinfo/openwrt-devel