On Fri, Jan 8, 2021 at 9:26 PM Kees Cook [off-list ref] wrote:
quoted
On Thu, Jan 7, 2021 at 8:00 PM Jason A. Donenfeld [off-list ref] wrote:
quoted
quoted
quoted
Hi Dmitry,
On Mon, Dec 21, 2020 at 10:14 AM Dmitry Vyukov [off-list ref] wrote:
quoted
Hi Jason,
Thanks for looking into this.
Reading clang docs for ubsan:
https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html
-fsanitize=object-size: An attempt to potentially use bytes which the
optimizer can determine are not part of the object being accessed.
This will also detect some types of undefined behavior that may not
directly access memory, but are provably incorrect given the size of
the objects involved, such as invalid downcasts and calling methods on
invalid pointers. These checks are made in terms of
__builtin_object_size, and consequently may be able to detect more
problems at higher optimization levels.
From skimming though your description this seems to fall into
"provably incorrect given the size of the objects involved".
I guess it's one of these cases which trigger undefined behavior and
compiler can e.g. remove all of this code assuming it will be never
called at runtime and any branches leading to it will always branch in
other directions, or something.
Right that sort of makes sense, and I can imagine that in more general
cases the struct casting could lead to UB. But what has me scratching
my head is that syzbot couldn't reproduce. The cast happens every
time. What about that one time was special? Did the address happen to
fall on the border of a mapping? Is UBSAN non-deterministic as an
optimization? Or is there actually some mysterious UaF happening with
my usage of skbs that I shouldn't overlook?
Ahh, makes sense. So it is easily reproducible after all.
You're still of the opinion that it's a false positive, right? I
shouldn't spend more cycles on this?
No, I am not saying this is a false positive. I think it's an
undefined behavior.
Either way, we need to resolve this one way or another to unblock
testing the rest of the kernel, if not with a fix to wg, then with a
fix to ubsan, or disable this check for kernel if kernel community
decides we want to use and keep this type of C undefined behavior in
the code base intentionally.
So far I see only 2 "UBSAN: object-size-mismatch" reports on the dashboard:
https://syzkaller.appspot.com/upstream
So cleaning them up looks doable. Is there a way to restructure the
code to not invoke undefined behavior?
Right; that's my question as well.
quoted
Kees, do you have any suggestions on how to proceed? This seems to
stop testing of the whole kernel at the moment.
If it's blocking other stuff and there isn't a path to fixing it soon, then I think we'll need to disable this check (and open an issue to track it).
Oh, I see, the code is actually in skbuff.h:
static inline void __skb_queue_tail(struct sk_buff_head *list, struct
sk_buff *newsk)
{
__skb_queue_before(list, (struct sk_buff *)list, newsk);
}
It casts sk_buff_head to sk_buff relying on equal layout of some
prefix of these structs.
Is it really UB in C? UBSAN docs say:
"An attempt to potentially use bytes which the optimizer can determine
are not part of the object being accessed".
But C has POD layout for structs, right? These next/prev fields are
within sk_buff_head (otherwise things would explode).
I can imagine this may be not valid in C++, can this UBSAN check be
C++-specific? Or at least some subset of this check, I can imagine it
can detect bad bugs in C as well where things go really wrong.
If there is no quick solution proposed, I tend to disable this check
in syzbot for now. We need to clean at least common things like
sk_buff first.
On Sat, Jan 9, 2021 at 10:46 AM Dmitry Vyukov [off-list ref] wrote:
On Fri, Jan 8, 2021 at 9:26 PM Kees Cook [off-list ref] wrote:
quoted
quoted
On Thu, Jan 7, 2021 at 8:00 PM Jason A. Donenfeld [off-list ref] wrote:
quoted
quoted
quoted
Hi Dmitry,
On Mon, Dec 21, 2020 at 10:14 AM Dmitry Vyukov [off-list ref] wrote:
quoted
Hi Jason,
Thanks for looking into this.
Reading clang docs for ubsan:
https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html
-fsanitize=object-size: An attempt to potentially use bytes which the
optimizer can determine are not part of the object being accessed.
This will also detect some types of undefined behavior that may not
directly access memory, but are provably incorrect given the size of
the objects involved, such as invalid downcasts and calling methods on
invalid pointers. These checks are made in terms of
__builtin_object_size, and consequently may be able to detect more
problems at higher optimization levels.
From skimming though your description this seems to fall into
"provably incorrect given the size of the objects involved".
I guess it's one of these cases which trigger undefined behavior and
compiler can e.g. remove all of this code assuming it will be never
called at runtime and any branches leading to it will always branch in
other directions, or something.
Right that sort of makes sense, and I can imagine that in more general
cases the struct casting could lead to UB. But what has me scratching
my head is that syzbot couldn't reproduce. The cast happens every
time. What about that one time was special? Did the address happen to
fall on the border of a mapping? Is UBSAN non-deterministic as an
optimization? Or is there actually some mysterious UaF happening with
my usage of skbs that I shouldn't overlook?
Ahh, makes sense. So it is easily reproducible after all.
You're still of the opinion that it's a false positive, right? I
shouldn't spend more cycles on this?
No, I am not saying this is a false positive. I think it's an
undefined behavior.
Either way, we need to resolve this one way or another to unblock
testing the rest of the kernel, if not with a fix to wg, then with a
fix to ubsan, or disable this check for kernel if kernel community
decides we want to use and keep this type of C undefined behavior in
the code base intentionally.
So far I see only 2 "UBSAN: object-size-mismatch" reports on the dashboard:
https://syzkaller.appspot.com/upstream
So cleaning them up looks doable. Is there a way to restructure the
code to not invoke undefined behavior?
Right; that's my question as well.
quoted
Kees, do you have any suggestions on how to proceed? This seems to
stop testing of the whole kernel at the moment.
If it's blocking other stuff and there isn't a path to fixing it soon, then I think we'll need to disable this check (and open an issue to track it).
Oh, I see, the code is actually in skbuff.h:
static inline void __skb_queue_tail(struct sk_buff_head *list, struct
sk_buff *newsk)
{
__skb_queue_before(list, (struct sk_buff *)list, newsk);
}
It casts sk_buff_head to sk_buff relying on equal layout of some
prefix of these structs.
Is it really UB in C? UBSAN docs say:
"An attempt to potentially use bytes which the optimizer can determine
are not part of the object being accessed".
But C has POD layout for structs, right? These next/prev fields are
within sk_buff_head (otherwise things would explode).
I can imagine this may be not valid in C++, can this UBSAN check be
C++-specific? Or at least some subset of this check, I can imagine it
can detect bad bugs in C as well where things go really wrong.
If there is no quick solution proposed, I tend to disable this check
in syzbot for now. We need to clean at least common things like
sk_buff first.
FTR, I've disabled the following UBSAN configs:
UBSAN_MISC
UBSAN_DIV_ZERO
UBSAN_BOOL
UBSAN_OBJECT_SIZE
UBSAN_SIGNED_OVERFLOW
UBSAN_UNSIGNED_OVERFLOW
UBSAN_ENUM
UBSAN_ALIGNMENT
UBSAN_UNREACHABLE
Only these are enabled now:
UBSAN_BOUNDS
UBSAN_SHIFT
This is commit:
https://github.com/google/syzkaller/commit/2c1f2513486f21d26b1942ce77ffc782677fbf4e
On Mon, Jan 11, 2021 at 12:20 PM Dmitry Vyukov [off-list ref] wrote:
...
FTR, I've disabled the following UBSAN configs:
UBSAN_MISC
UBSAN_DIV_ZERO
UBSAN_BOOL
UBSAN_OBJECT_SIZE
UBSAN_SIGNED_OVERFLOW
UBSAN_UNSIGNED_OVERFLOW
UBSAN_ENUM
UBSAN_ALIGNMENT
UBSAN_UNREACHABLE
Only these are enabled now:
UBSAN_BOUNDS
UBSAN_SHIFT
This is commit:
https://github.com/google/syzkaller/commit/2c1f2513486f21d26b1942ce77ffc782677fbf4e
I think the commit cut too deep.
The overflows are important if folks are building with compilers other than GCC.
The aligned data accesses are important on platforms like MIPS64 and Sparc64.
Object size is important because it catches destination buffer overflows.
I don't know what's in miscellaneous. There may be something useful in there.
Jeff
On Mon, Jan 11, 2021 at 6:35 PM Jeffrey Walton [off-list ref] wrote:
On Mon, Jan 11, 2021 at 12:20 PM Dmitry Vyukov [off-list ref] wrote:
quoted
...
FTR, I've disabled the following UBSAN configs:
UBSAN_MISC
UBSAN_DIV_ZERO
UBSAN_BOOL
UBSAN_OBJECT_SIZE
UBSAN_SIGNED_OVERFLOW
UBSAN_UNSIGNED_OVERFLOW
UBSAN_ENUM
UBSAN_ALIGNMENT
UBSAN_UNREACHABLE
Only these are enabled now:
UBSAN_BOUNDS
UBSAN_SHIFT
This is commit:
https://github.com/google/syzkaller/commit/2c1f2513486f21d26b1942ce77ffc782677fbf4e
I think the commit cut too deep.
The overflows are important if folks are building with compilers other than GCC.
The aligned data accesses are important on platforms like MIPS64 and Sparc64.
Object size is important because it catches destination buffer overflows.
I don't know what's in miscellaneous. There may be something useful in there.
Hi Jeff,
See the commit for reasons why each of these is disabled.
E.g. object size, somebody first needs to fix bugs like this one.
While things like skbuff have these UBs on trivial workloads, there is
no point in involving fuzzing and making it crash on this trivial bug
all the time and stopping doing any other kernel testing as the
result.
On Mon, Jan 11, 2021 at 12:58 PM Dmitry Vyukov [off-list ref] wrote:
On Mon, Jan 11, 2021 at 6:35 PM Jeffrey Walton [off-list ref] wrote:
quoted
On Mon, Jan 11, 2021 at 12:20 PM Dmitry Vyukov [off-list ref] wrote:
quoted
...
FTR, I've disabled the following UBSAN configs:
UBSAN_MISC
UBSAN_DIV_ZERO
UBSAN_BOOL
UBSAN_OBJECT_SIZE
UBSAN_SIGNED_OVERFLOW
UBSAN_UNSIGNED_OVERFLOW
UBSAN_ENUM
UBSAN_ALIGNMENT
UBSAN_UNREACHABLE
Only these are enabled now:
UBSAN_BOUNDS
UBSAN_SHIFT
This is commit:
https://github.com/google/syzkaller/commit/2c1f2513486f21d26b1942ce77ffc782677fbf4e
I think the commit cut too deep.
The overflows are important if folks are building with compilers other than GCC.
The aligned data accesses are important on platforms like MIPS64 and Sparc64.
Object size is important because it catches destination buffer overflows.
I don't know what's in miscellaneous. There may be something useful in there.
Hi Jeff,
See the commit for reasons why each of these is disabled.
E.g. object size, somebody first needs to fix bugs like this one.
While things like skbuff have these UBs on trivial workloads, there is
no point in involving fuzzing and making it crash on this trivial bug
all the time and stopping doing any other kernel testing as the
result.
Going off-topic a bit, what would you suggest for UBSAN_OBJECT_SIZE?
It seems to me object size checking is being conflated with object
type. It seems to me they need to be split: UBSAN_OBJECT_SIZE for
actual object sizes, and UBSAN_OBJECT_TYPE for the casts.
I still have a bitter taste in my mouth from
https://www.cvedetails.com/bugtraq-bid/57602/libupnp-Multiple-Buffer-Overflow-Vulnerabilities.html.
I hate to see buffer checks go away. (And I realize the kernel folks
are more skilled than the guy who wrote libupnp).
Jeff
On Mon, Jan 11, 2021 at 7:15 PM Jeffrey Walton [off-list ref] wrote:
quoted
quoted
quoted
...
FTR, I've disabled the following UBSAN configs:
UBSAN_MISC
UBSAN_DIV_ZERO
UBSAN_BOOL
UBSAN_OBJECT_SIZE
UBSAN_SIGNED_OVERFLOW
UBSAN_UNSIGNED_OVERFLOW
UBSAN_ENUM
UBSAN_ALIGNMENT
UBSAN_UNREACHABLE
Only these are enabled now:
UBSAN_BOUNDS
UBSAN_SHIFT
This is commit:
https://github.com/google/syzkaller/commit/2c1f2513486f21d26b1942ce77ffc782677fbf4e
I think the commit cut too deep.
The overflows are important if folks are building with compilers other than GCC.
The aligned data accesses are important on platforms like MIPS64 and Sparc64.
Object size is important because it catches destination buffer overflows.
I don't know what's in miscellaneous. There may be something useful in there.
Hi Jeff,
See the commit for reasons why each of these is disabled.
E.g. object size, somebody first needs to fix bugs like this one.
While things like skbuff have these UBs on trivial workloads, there is
no point in involving fuzzing and making it crash on this trivial bug
all the time and stopping doing any other kernel testing as the
result.
Going off-topic a bit, what would you suggest for UBSAN_OBJECT_SIZE?
It seems to me object size checking is being conflated with object
type. It seems to me they need to be split: UBSAN_OBJECT_SIZE for
actual object sizes, and UBSAN_OBJECT_TYPE for the casts.
I still have a bitter taste in my mouth from
https://www.cvedetails.com/bugtraq-bid/57602/libupnp-Multiple-Buffer-Overflow-Vulnerabilities.html.
I hate to see buffer checks go away. (And I realize the kernel folks
are more skilled than the guy who wrote libupnp).
Jeff
I've filed https://bugs.llvm.org/show_bug.cgi?id=48726 for this. Does
it capture what you are asking? Let's move the discussion re ubsan
there.
However, in the first place I am suggesting fixing the code. E.g. for
sk_buff I would assume it's relatively easily fixable. A
formally legal fix I think should put sk_buff_head into sk_buff and
use it, no downsides and will eliminate the confusing "should go
first" comments.
Or as an workaround maybe we could make __skb_queue_before accept
sk_buff_head and cast the other way around.