lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Tue, 02 Feb 2016 09:22:54 +0100 From: Andrzej Hajda <a.hajda@...sung.com> To: Andrew Morton <akpm@...ux-foundation.org> Cc: open list <linux-kernel@...r.kernel.org>, Bartlomiej Zolnierkiewicz <b.zolnierkie@...sung.com>, Marek Szyprowski <m.szyprowski@...sung.com> Subject: Re: [PATCH v2] err.h: allow IS_ERR_VALUE to handle properly more types On 02/02/2016 07:23 AM, Andrew Morton wrote: > On Thu, 28 Jan 2016 09:27:28 +0100 Andrzej Hajda <a.hajda@...sung.com> wrote: > >> Current implementation of IS_ERR_VALUE works correctly only with >> following types: >> - unsigned long, >> - short, int, long. >> Other types are handled incorrectly either on 32-bit either on 64-bit >> either on both architectures. >> The patch fixes it by comparing argument with MAX_ERRNO casted >> to argument's type for unsigned types and comparing with zero for signed >> types. As a result all integer types bigger than char are handled properly. >> >> I have analyzed usage of IS_ERR_VALUE using coccinelle and in about 35 >> cases it is used incorrectly, ie it can hide errors depending of 32/64 bit >> architecture. Instead of fixing usage I propose to enhance the macro >> to cover more types. >> And just for the record: the macro is used 101 times with signed variables, >> I am not sure if it should be preferred over simple comparison "ret < 0", >> but the new version can do it as well. >> >> And below list of detected potential errors: >> >> ... >> >> --- a/include/linux/err.h >> +++ b/include/linux/err.h >> @@ -18,7 +18,9 @@ >> >> #ifndef __ASSEMBLY__ >> >> -#define IS_ERR_VALUE(x) unlikely((x) >= (unsigned long)-MAX_ERRNO) >> +#define IS_ERR_VALUE(x) ((typeof(x))(-1) <= 0 \ >> + ? unlikely((x) < 0) \ >> + : unlikely((x) >= (typeof(x))-MAX_ERRNO)) >> > hm, seems complicated. Can we simply cast the value to long? > > #define IS_ERR_VALUE(x) ((long)x < 0) && (long)x >= (long)-MAX_ERRNO) > > and simplify that to > > #define IS_ERR_VALUE(x) ((unsigned long)(long)x >= (unsigned long)-MAX_ERRNO) > > or something like that. It will not work with u32 on 64bit systems. Short rationales behind my implementation: 1. Typical usage pattern of the macro looks like: T x; ... x = -ESOME_ERROR; ... if (IS_ERR_VALUE(x)) ... In error assignment we have casting of -ESOME_ERROR to type T. Casting of -MAX_ERRNO to the same type in the macro assures that comparison will be sane, at least for types big enough. In short we ends at following expression (for unsigned types): (T)-ESOME_ERROR >= (T)-MAX_ERRNO In old implementation we ended at: (unsigned)(T)-ESOME_ERROR >= (unsigned)-MAX_ERRNO Different castings for -ESOME_ERROR and for -MAX_ERRNO makes this comparison incorrect for some types T. 2. Error checking is completely different for signed and unsigned vars: a. signed are compared to 0: ret < 0. b. unsigned are compared with some high value: ret >= (-MAX_ERRNO). This dualism is clearly visible and emphasized in this implementation. In old implementation IS_ERR_VALUE works correctly for some signed types due to obscure C casting rules. Summarizing: current implementation is short but tricky, answering why it works/fails for certain types is quite challenging. On the other side proposed implementation is longer but more straightforward, and of course is correct for more types :) Maybe, to make it more clear, it could be good to use separate macro for signedness: #define IS_SIGNED_TYPE(t) ((t)(-1) <= 0) #define IS_ERR_VALUE(x) (IS_SIGNED_TYPE(typeof(x)) \ ? unlikely((x) < 0) \ : unlikely((x) >= (typeof(x))-MAX_ERRNO)) Regards Andrzej
Powered by blists - more mailing lists