Kernel throws a runtime object-size-mismatch error in skbuff queue
helpers like in [1]. This happens every time there is a pattern
like the below:
int skbuf_xmit(struct sk_buff *skb)
{
struct sk_buff_head list;
__skb_queue_head_init(&list);
__skb_queue_tail(&list, skb); <-- offending call
return do_xmit(net, &list);
}
and the kernel is build with clang and -fsanitize=undefined flag set.
The reason is that the functions __skb_queue_[tail|head]() access the
struct sk_buff_head object via a pointer to struct sk_buff, which is
much bigger in size than the sk_buff_head. This could cause undefined
behavior and clang is complaining:
UBSAN: object-size-mismatch in ./include/linux/skbuff.h:2023:28
member access within address ffffc90000cb71c0 with insufficient space
for an object of type 'struct sk_buff'
Suppress the error with __attribute__((no_sanitize("undefined")))
in the skb helpers.
[1] https://syzkaller.appspot.com/bug?id=5d9f0bca58cea80f272b73500df67dcd9e35c886
Cc: "Nathan Chancellor" <[email protected]>
Cc: "Nick Desaulniers" <[email protected]>
Cc: "Jakub Kicinski" <[email protected]>
Cc: "David S. Miller" <[email protected]>
Cc: "Jonathan Lemon" <[email protected]>
Cc: "Alexander Lobakin" <[email protected]>
Cc: "Willem de Bruijn" <[email protected]>
Cc: "Paolo Abeni" <[email protected]>
Cc: "Cong Wang" <[email protected]>
Cc: "Kevin Hao" <[email protected]>
Cc: "Ilias Apalodimas" <[email protected]>
Cc: "Marco Elver" <[email protected]>
Cc: <[email protected]>
Cc: <[email protected]>
Cc: <[email protected]>
Signed-off-by: Tadeusz Struk <[email protected]>
---
include/linux/skbuff.h | 49 ++++++++++++++++++++++++------------------
1 file changed, 28 insertions(+), 21 deletions(-)
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 0bd6520329f6..8ec46e3a503d 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -1933,9 +1933,10 @@ static inline void skb_queue_head_init_class(struct sk_buff_head *list,
* The "__skb_xxxx()" functions are the non-atomic ones that
* can only be called with interrupts disabled.
*/
-static inline void __skb_insert(struct sk_buff *newsk,
- struct sk_buff *prev, struct sk_buff *next,
- struct sk_buff_head *list)
+static inline void __no_sanitize_undefined
+__skb_insert(struct sk_buff *newsk,
+ struct sk_buff *prev, struct sk_buff *next,
+ struct sk_buff_head *list)
{
/* See skb_queue_empty_lockless() and skb_peek_tail()
* for the opposite READ_ONCE()
@@ -1966,8 +1967,9 @@ static inline void __skb_queue_splice(const struct sk_buff_head *list,
* @list: the new list to add
* @head: the place to add it in the first list
*/
-static inline void skb_queue_splice(const struct sk_buff_head *list,
- struct sk_buff_head *head)
+static inline void __no_sanitize_undefined
+skb_queue_splice(const struct sk_buff_head *list,
+ struct sk_buff_head *head)
{
if (!skb_queue_empty(list)) {
__skb_queue_splice(list, (struct sk_buff *) head, head->next);
@@ -1982,8 +1984,9 @@ static inline void skb_queue_splice(const struct sk_buff_head *list,
*
* The list at @list is reinitialised
*/
-static inline void skb_queue_splice_init(struct sk_buff_head *list,
- struct sk_buff_head *head)
+static inline void __no_sanitize_undefined
+skb_queue_splice_init(struct sk_buff_head *list,
+ struct sk_buff_head *head)
{
if (!skb_queue_empty(list)) {
__skb_queue_splice(list, (struct sk_buff *) head, head->next);
@@ -1997,8 +2000,9 @@ static inline void skb_queue_splice_init(struct sk_buff_head *list,
* @list: the new list to add
* @head: the place to add it in the first list
*/
-static inline void skb_queue_splice_tail(const struct sk_buff_head *list,
- struct sk_buff_head *head)
+static inline void __no_sanitize_undefined
+skb_queue_splice_tail(const struct sk_buff_head *list,
+ struct sk_buff_head *head)
{
if (!skb_queue_empty(list)) {
__skb_queue_splice(list, head->prev, (struct sk_buff *) head);
@@ -2014,8 +2018,9 @@ static inline void skb_queue_splice_tail(const struct sk_buff_head *list,
* Each of the lists is a queue.
* The list at @list is reinitialised
*/
-static inline void skb_queue_splice_tail_init(struct sk_buff_head *list,
- struct sk_buff_head *head)
+static inline void __no_sanitize_undefined
+skb_queue_splice_tail_init(struct sk_buff_head *list,
+ struct sk_buff_head *head)
{
if (!skb_queue_empty(list)) {
__skb_queue_splice(list, head->prev, (struct sk_buff *) head);
@@ -2035,9 +2040,10 @@ static inline void skb_queue_splice_tail_init(struct sk_buff_head *list,
*
* A buffer cannot be placed on two lists at the same time.
*/
-static inline void __skb_queue_after(struct sk_buff_head *list,
- struct sk_buff *prev,
- struct sk_buff *newsk)
+static inline void __no_sanitize_undefined
+__skb_queue_after(struct sk_buff_head *list,
+ struct sk_buff *prev,
+ struct sk_buff *newsk)
{
__skb_insert(newsk, prev, prev->next, list);
}
@@ -2045,9 +2051,10 @@ static inline void __skb_queue_after(struct sk_buff_head *list,
void skb_append(struct sk_buff *old, struct sk_buff *newsk,
struct sk_buff_head *list);
-static inline void __skb_queue_before(struct sk_buff_head *list,
- struct sk_buff *next,
- struct sk_buff *newsk)
+static inline void __no_sanitize_undefined
+__skb_queue_before(struct sk_buff_head *list,
+ struct sk_buff *next,
+ struct sk_buff *newsk)
{
__skb_insert(newsk, next->prev, next, list);
}
@@ -2062,8 +2069,8 @@ static inline void __skb_queue_before(struct sk_buff_head *list,
*
* A buffer cannot be placed on two lists at the same time.
*/
-static inline void __skb_queue_head(struct sk_buff_head *list,
- struct sk_buff *newsk)
+static inline void __no_sanitize_undefined
+__skb_queue_head(struct sk_buff_head *list, struct sk_buff *newsk)
{
__skb_queue_after(list, (struct sk_buff *)list, newsk);
}
@@ -2079,8 +2086,8 @@ void skb_queue_head(struct sk_buff_head *list, struct sk_buff *newsk);
*
* A buffer cannot be placed on two lists at the same time.
*/
-static inline void __skb_queue_tail(struct sk_buff_head *list,
- struct sk_buff *newsk)
+static inline void __no_sanitize_undefined
+__skb_queue_tail(struct sk_buff_head *list, struct sk_buff *newsk)
{
__skb_queue_before(list, (struct sk_buff *)list, newsk);
}
--
2.33.1
On Thu, 11 Nov 2021 at 01:36, Tadeusz Struk <[email protected]> wrote:
> Kernel throws a runtime object-size-mismatch error in skbuff queue
> helpers like in [1]. This happens every time there is a pattern
> like the below:
>
> int skbuf_xmit(struct sk_buff *skb)
> {
> struct sk_buff_head list;
>
> __skb_queue_head_init(&list);
> __skb_queue_tail(&list, skb); <-- offending call
>
> return do_xmit(net, &list);
> }
>
> and the kernel is build with clang and -fsanitize=undefined flag set.
> The reason is that the functions __skb_queue_[tail|head]() access the
> struct sk_buff_head object via a pointer to struct sk_buff, which is
> much bigger in size than the sk_buff_head. This could cause undefined
> behavior and clang is complaining:
>
> UBSAN: object-size-mismatch in ./include/linux/skbuff.h:2023:28
> member access within address ffffc90000cb71c0 with insufficient space
> for an object of type 'struct sk_buff'
The config includes CONFIG_UBSAN_OBJECT_SIZE, right? Normally that's
disabled by default, probably why nobody has noticed these much.
> Suppress the error with __attribute__((no_sanitize("undefined")))
> in the skb helpers.
Isn't there a better way, because doing this might also suppress other
issues wholesale. __no_sanitize_undefined should be the last resort.
> [1] https://syzkaller.appspot.com/bug?id=5d9f0bca58cea80f272b73500df67dcd9e35c886
>
> Cc: "Nathan Chancellor" <[email protected]>
> Cc: "Nick Desaulniers" <[email protected]>
> Cc: "Jakub Kicinski" <[email protected]>
> Cc: "David S. Miller" <[email protected]>
> Cc: "Jonathan Lemon" <[email protected]>
> Cc: "Alexander Lobakin" <[email protected]>
> Cc: "Willem de Bruijn" <[email protected]>
> Cc: "Paolo Abeni" <[email protected]>
> Cc: "Cong Wang" <[email protected]>
> Cc: "Kevin Hao" <[email protected]>
> Cc: "Ilias Apalodimas" <[email protected]>
> Cc: "Marco Elver" <[email protected]>
> Cc: <[email protected]>
> Cc: <[email protected]>
> Cc: <[email protected]>
>
> Signed-off-by: Tadeusz Struk <[email protected]>
> ---
> include/linux/skbuff.h | 49 ++++++++++++++++++++++++------------------
> 1 file changed, 28 insertions(+), 21 deletions(-)
>
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index 0bd6520329f6..8ec46e3a503d 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -1933,9 +1933,10 @@ static inline void skb_queue_head_init_class(struct sk_buff_head *list,
> * The "__skb_xxxx()" functions are the non-atomic ones that
> * can only be called with interrupts disabled.
> */
> -static inline void __skb_insert(struct sk_buff *newsk,
> - struct sk_buff *prev, struct sk_buff *next,
> - struct sk_buff_head *list)
> +static inline void __no_sanitize_undefined
> +__skb_insert(struct sk_buff *newsk,
> + struct sk_buff *prev, struct sk_buff *next,
> + struct sk_buff_head *list)
> {
> /* See skb_queue_empty_lockless() and skb_peek_tail()
> * for the opposite READ_ONCE()
> @@ -1966,8 +1967,9 @@ static inline void __skb_queue_splice(const struct sk_buff_head *list,
> * @list: the new list to add
> * @head: the place to add it in the first list
> */
> -static inline void skb_queue_splice(const struct sk_buff_head *list,
> - struct sk_buff_head *head)
> +static inline void __no_sanitize_undefined
> +skb_queue_splice(const struct sk_buff_head *list,
> + struct sk_buff_head *head)
> {
> if (!skb_queue_empty(list)) {
> __skb_queue_splice(list, (struct sk_buff *) head, head->next);
> @@ -1982,8 +1984,9 @@ static inline void skb_queue_splice(const struct sk_buff_head *list,
> *
> * The list at @list is reinitialised
> */
> -static inline void skb_queue_splice_init(struct sk_buff_head *list,
> - struct sk_buff_head *head)
> +static inline void __no_sanitize_undefined
> +skb_queue_splice_init(struct sk_buff_head *list,
> + struct sk_buff_head *head)
> {
> if (!skb_queue_empty(list)) {
> __skb_queue_splice(list, (struct sk_buff *) head, head->next);
> @@ -1997,8 +2000,9 @@ static inline void skb_queue_splice_init(struct sk_buff_head *list,
> * @list: the new list to add
> * @head: the place to add it in the first list
> */
> -static inline void skb_queue_splice_tail(const struct sk_buff_head *list,
> - struct sk_buff_head *head)
> +static inline void __no_sanitize_undefined
> +skb_queue_splice_tail(const struct sk_buff_head *list,
> + struct sk_buff_head *head)
> {
> if (!skb_queue_empty(list)) {
> __skb_queue_splice(list, head->prev, (struct sk_buff *) head);
> @@ -2014,8 +2018,9 @@ static inline void skb_queue_splice_tail(const struct sk_buff_head *list,
> * Each of the lists is a queue.
> * The list at @list is reinitialised
> */
> -static inline void skb_queue_splice_tail_init(struct sk_buff_head *list,
> - struct sk_buff_head *head)
> +static inline void __no_sanitize_undefined
> +skb_queue_splice_tail_init(struct sk_buff_head *list,
> + struct sk_buff_head *head)
> {
> if (!skb_queue_empty(list)) {
> __skb_queue_splice(list, head->prev, (struct sk_buff *) head);
> @@ -2035,9 +2040,10 @@ static inline void skb_queue_splice_tail_init(struct sk_buff_head *list,
> *
> * A buffer cannot be placed on two lists at the same time.
> */
> -static inline void __skb_queue_after(struct sk_buff_head *list,
> - struct sk_buff *prev,
> - struct sk_buff *newsk)
> +static inline void __no_sanitize_undefined
> +__skb_queue_after(struct sk_buff_head *list,
> + struct sk_buff *prev,
> + struct sk_buff *newsk)
> {
> __skb_insert(newsk, prev, prev->next, list);
> }
> @@ -2045,9 +2051,10 @@ static inline void __skb_queue_after(struct sk_buff_head *list,
> void skb_append(struct sk_buff *old, struct sk_buff *newsk,
> struct sk_buff_head *list);
>
> -static inline void __skb_queue_before(struct sk_buff_head *list,
> - struct sk_buff *next,
> - struct sk_buff *newsk)
> +static inline void __no_sanitize_undefined
> +__skb_queue_before(struct sk_buff_head *list,
> + struct sk_buff *next,
> + struct sk_buff *newsk)
> {
> __skb_insert(newsk, next->prev, next, list);
> }
> @@ -2062,8 +2069,8 @@ static inline void __skb_queue_before(struct sk_buff_head *list,
> *
> * A buffer cannot be placed on two lists at the same time.
> */
> -static inline void __skb_queue_head(struct sk_buff_head *list,
> - struct sk_buff *newsk)
> +static inline void __no_sanitize_undefined
> +__skb_queue_head(struct sk_buff_head *list, struct sk_buff *newsk)
> {
> __skb_queue_after(list, (struct sk_buff *)list, newsk);
> }
> @@ -2079,8 +2086,8 @@ void skb_queue_head(struct sk_buff_head *list, struct sk_buff *newsk);
> *
> * A buffer cannot be placed on two lists at the same time.
> */
> -static inline void __skb_queue_tail(struct sk_buff_head *list,
> - struct sk_buff *newsk)
> +static inline void __no_sanitize_undefined
> +__skb_queue_tail(struct sk_buff_head *list, struct sk_buff *newsk)
> {
> __skb_queue_before(list, (struct sk_buff *)list, newsk);
> }
> --
> 2.33.1
>
Hi Marco,
On 11/11/21 01:51, Marco Elver wrote:
> On Thu, 11 Nov 2021 at 01:36, Tadeusz Struk<[email protected]> wrote:
>> Kernel throws a runtime object-size-mismatch error in skbuff queue
>> helpers like in [1]. This happens every time there is a pattern
>> like the below:
>>
>> int skbuf_xmit(struct sk_buff *skb)
>> {
>> struct sk_buff_head list;
>>
>> __skb_queue_head_init(&list);
>> __skb_queue_tail(&list, skb); <-- offending call
>>
>> return do_xmit(net, &list);
>> }
>>
>> and the kernel is build with clang and -fsanitize=undefined flag set.
>> The reason is that the functions __skb_queue_[tail|head]() access the
>> struct sk_buff_head object via a pointer to struct sk_buff, which is
>> much bigger in size than the sk_buff_head. This could cause undefined
>> behavior and clang is complaining:
>>
>> UBSAN: object-size-mismatch in ./include/linux/skbuff.h:2023:28
>> member access within address ffffc90000cb71c0 with insufficient space
>> for an object of type 'struct sk_buff'
> The config includes CONFIG_UBSAN_OBJECT_SIZE, right? Normally that's
> disabled by default, probably why nobody has noticed these much.
Right, in all the defconfigs CONFIG_UBSAN_OBJECT_SIZE is not set.
>
>> Suppress the error with __attribute__((no_sanitize("undefined")))
>> in the skb helpers.
> Isn't there a better way, because doing this might also suppress other
> issues wholesale. __no_sanitize_undefined should be the last resort.
>
The other way to fix it would be to make the struct sk_buff_head
equal in size with struct sk_buff:
struct sk_buff_head {
- /* These two members must be first. */
- struct sk_buff *next;
- struct sk_buff *prev;
+ union {
+ struct {
+ /* These two members must be first. */
+ struct sk_buff *next;
+ struct sk_buff *prev;
- __u32 qlen;
- spinlock_t lock;
+ __u32 qlen;
+ spinlock_t lock;
+ };
+ struct sk_buff __prv;
+ };
};
but that's much more invasive, and I don't even have means to
quantify this in terms of final binary size and performance
impact. I think that would be a flat out no go.
From the other hand if you look at the __skb_queue functions
they don't do much and at all so there is no much room for
other issues really. I followed the suggestion in [1]:
"if your function deliberately contains possible ..., you can
use __attribute__((no_sanitize... "
[1] https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html
--
Thanks,
Tadeusz
On Thu, 11 Nov 2021 at 16:46, Tadeusz Struk <[email protected]> wrote:
>
> Hi Marco,
> On 11/11/21 01:51, Marco Elver wrote:
> > On Thu, 11 Nov 2021 at 01:36, Tadeusz Struk<[email protected]> wrote:
> >> Kernel throws a runtime object-size-mismatch error in skbuff queue
> >> helpers like in [1]. This happens every time there is a pattern
> >> like the below:
> >>
> >> int skbuf_xmit(struct sk_buff *skb)
> >> {
> >> struct sk_buff_head list;
> >>
> >> __skb_queue_head_init(&list);
> >> __skb_queue_tail(&list, skb); <-- offending call
> >>
> >> return do_xmit(net, &list);
> >> }
> >>
> >> and the kernel is build with clang and -fsanitize=undefined flag set.
> >> The reason is that the functions __skb_queue_[tail|head]() access the
> >> struct sk_buff_head object via a pointer to struct sk_buff, which is
> >> much bigger in size than the sk_buff_head. This could cause undefined
> >> behavior and clang is complaining:
> >>
> >> UBSAN: object-size-mismatch in ./include/linux/skbuff.h:2023:28
> >> member access within address ffffc90000cb71c0 with insufficient space
> >> for an object of type 'struct sk_buff'
> > The config includes CONFIG_UBSAN_OBJECT_SIZE, right? Normally that's
> > disabled by default, probably why nobody has noticed these much.
>
> Right, in all the defconfigs CONFIG_UBSAN_OBJECT_SIZE is not set.
>
> >
> >> Suppress the error with __attribute__((no_sanitize("undefined")))
> >> in the skb helpers.
> > Isn't there a better way, because doing this might also suppress other
> > issues wholesale. __no_sanitize_undefined should be the last resort.
> >
>
> The other way to fix it would be to make the struct sk_buff_head
> equal in size with struct sk_buff:
>
> struct sk_buff_head {
> - /* These two members must be first. */
> - struct sk_buff *next;
> - struct sk_buff *prev;
> + union {
> + struct {
> + /* These two members must be first. */
> + struct sk_buff *next;
> + struct sk_buff *prev;
>
> - __u32 qlen;
> - spinlock_t lock;
> + __u32 qlen;
> + spinlock_t lock;
> + };
> + struct sk_buff __prv;
> + };
> };
>
> but that's much more invasive, and I don't even have means to
> quantify this in terms of final binary size and performance
> impact. I think that would be a flat out no go.
>
> From the other hand if you look at the __skb_queue functions
> they don't do much and at all so there is no much room for
> other issues really. I followed the suggestion in [1]:
>
> "if your function deliberately contains possible ..., you can
> use __attribute__((no_sanitize... "
That general advice might not be compatible with what the kernel
wants, especially since UBSAN_OBJECT_SIZE is normally disabled and I
think known to cause these issues in the kernel.
I'll defer to maintainers to decide what would be the preferred way of
handling this.
On 11/11/21 07:52, Marco Elver wrote:
>> The other way to fix it would be to make the struct sk_buff_head
>> equal in size with struct sk_buff:
>>
>> struct sk_buff_head {
>> - /* These two members must be first. */
>> - struct sk_buff *next;
>> - struct sk_buff *prev;
>> + union {
>> + struct {
>> + /* These two members must be first. */
>> + struct sk_buff *next;
>> + struct sk_buff *prev;
>>
>> - __u32 qlen;
>> - spinlock_t lock;
>> + __u32 qlen;
>> + spinlock_t lock;
>> + };
>> + struct sk_buff __prv;
>> + };
>> };
>>
>> but that's much more invasive, and I don't even have means to
>> quantify this in terms of final binary size and performance
>> impact. I think that would be a flat out no go.
>>
>> From the other hand if you look at the __skb_queue functions
>> they don't do much and at all so there is no much room for
>> other issues really. I followed the suggestion in [1]:
>>
>> "if your function deliberately contains possible ..., you can
>> use __attribute__((no_sanitize... "
> That general advice might not be compatible with what the kernel
> wants, especially since UBSAN_OBJECT_SIZE is normally disabled and I
> think known to cause these issues in the kernel.
>
> I'll defer to maintainers to decide what would be the preferred way of
> handling this.
Sure, I would also like to know if there is a better way of fixing this.
Thanks for your feedback.
--
Thanks,
Tadeusz
On Thu, 11 Nov 2021 08:01:26 -0800 Tadeusz Struk wrote:
> > That general advice might not be compatible with what the kernel
> > wants, especially since UBSAN_OBJECT_SIZE is normally disabled and I
> > think known to cause these issues in the kernel.
> >
> > I'll defer to maintainers to decide what would be the preferred way of
> > handling this.
>
> Sure, I would also like to know if there is a better way of fixing this.
> Thanks for your feedback.
I remember Dave was working thru the tree at some point to clean up all
skb->next/skb->prev accesses so that we can switch over to using normal
list helpers.
I'm not sure if that stalled due to lack of time or some fundamental
problems.
Seems like finishing that would let us clean up such misuses?
From: Jakub Kicinski <[email protected]>
Date: Thu, 11 Nov 2021 09:54:44 -0800
> I'm not sure if that stalled due to lack of time or some fundamental
> problems.
ran out of time, then had a stroke...
> Seems like finishing that would let us clean up such misuses?
yes it would
On 11/12/21 07:42, David Miller wrote:
> From: Jakub Kicinski <[email protected]>
> Date: Thu, 11 Nov 2021 09:54:44 -0800
>
>> I'm not sure if that stalled due to lack of time or some fundamental
>> problems.
>
> ran out of time, then had a stroke...
>
>> Seems like finishing that would let us clean up such misuses?
>
> yes it would
>
so since there is not better way of suppressing the issue atm are
you ok with taking this fix for now?
--
Thanks,
Tadeusz
On Thu, 18 Nov 2021 08:05:01 -0800 Tadeusz Struk wrote:
> On 11/12/21 07:42, David Miller wrote:
> > From: Jakub Kicinski <[email protected]>
> > Date: Thu, 11 Nov 2021 09:54:44 -0800
> >
> >> I'm not sure if that stalled due to lack of time or some fundamental
> >> problems.
> >
> > ran out of time, then had a stroke...
> >
> >> Seems like finishing that would let us clean up such misuses?
> >
> > yes it would
>
> so since there is not better way of suppressing the issue atm are
> you ok with taking this fix for now?
I vote no on sprinkling ugly tags around to silence some random
checkers warning. We already have too many of them. They are
meaningless and confusing to people reading the code.
This is not a fundamental problem, the solution is clear.
On 11/18/21 08:38, Jakub Kicinski wrote:
> On Thu, 18 Nov 2021 08:05:01 -0800 Tadeusz Struk wrote:
>> On 11/12/21 07:42, David Miller wrote:
>>> From: Jakub Kicinski <[email protected]>
>>> Date: Thu, 11 Nov 2021 09:54:44 -0800
>>>
>>>> I'm not sure if that stalled due to lack of time or some fundamental
>>>> problems.
>>>
>>> ran out of time, then had a stroke...
>>>
>>>> Seems like finishing that would let us clean up such misuses?
>>>
>>> yes it would
>>
>> so since there is not better way of suppressing the issue atm are
>> you ok with taking this fix for now?
>
> I vote no on sprinkling ugly tags around to silence some random
> checkers warning. We already have too many of them. They are
> meaningless and confusing to people reading the code.
>
> This is not a fundamental problem, the solution is clear.
>
Fair enough.
David, did you post your work somewhere if someone would like to pick
it up and finish it?
--
Thanks,
Tadeusz