Fix invalid hpack decoding #1874

EvgeniiMekhanik · 2023-04-21T15:43:40Z

Continue decode hpack value same as we do it
if this value was not devide between several
frames.

const-t

LGTM

fw/sock.c

fw/sync_socket.h

EvgeniiMekhanik · 2023-05-18T10:03:55Z

fw/ss_skb.c

@@ -393,6 +393,7 @@ __split_linear_data(struct sk_buff *skb_head, struct sk_buff *skb, char *pspt,
 	}
 	skb->tail -= tail_len;
 	skb->data_len += tail_len;
+	skb->truesize += tail_len;


Please look for this place carefully! This code is necessary to prevent warning in kernel with small MTU and it makes akb truesize correct. But i don't fully understand why

As I see it's correct fix. We need to adjust truesize when we move data from linear part to paged fragments.

Lat's see on truesize in kernel, it's consist of size of skb data + sizeof(sk_buff) + sizeof(skb_shared_info). It means size of reserved linear data will be included in truesize during skb allocating. Because we don't change preallocated size of skb head when we manipulate with it, we must not change truesize.
However, when we move data from head to paged we increase size of skb(data_len) by adding new fragment, even if it points to already allocated data within skb. Thus, we must increment truesize. Just for clarity, if we will realloc the skb, we can allocate less data to headroom and in this case we can not change truesize.

Continue decode hpack value same as we do it if this value was not devide between several frames.

In case when skb_room is equal to zero and we allocate a new skb, we should move some fragments from the first skb only if it contains body. Also we should allocate new skb if skb frags is equal to MAX_SKB_FRAGS, not use next skb, because next skb already contain headers!

In case when frame is not completely received and first part of frame was prossed with error we should not zero `ctx->to_read` value, since we should read the remaining part of the frame.

fw/http_match.c

const-t · 2023-05-22T13:46:42Z

fw/ss_skb.c

@@ -393,6 +393,7 @@ __split_linear_data(struct sk_buff *skb_head, struct sk_buff *skb, char *pspt,
 	}
 	skb->tail -= tail_len;
 	skb->data_len += tail_len;
+	skb->truesize += tail_len;


As I see it's correct fix. We need to adjust truesize when we move data from linear part to paged fragments.

Lat's see on truesize in kernel, it's consist of size of skb data + sizeof(sk_buff) + sizeof(skb_shared_info). It means size of reserved linear data will be included in truesize during skb allocating. Because we don't change preallocated size of skb head when we manipulate with it, we must not change truesize.
However, when we move data from head to paged we increase size of skb(data_len) by adding new fragment, even if it points to already allocated data within skb. Thus, we must increment truesize. Just for clarity, if we will realloc the skb, we can allocate less data to headroom and in this case we can not change truesize.

krizhanovsky

It seems there are several issues

fw/hpack.h

krizhanovsky · 2023-05-22T21:45:27Z

fw/ss_skb.c

@@ -1311,6 +1318,7 @@ ss_skb_init_for_xmit(struct sk_buff *skb)

 	skb_dst_drop(skb);
 	INIT_LIST_HEAD(&skb->tcp_tsorted_anchor);
+	skb->tail_lock = 0;


ss_skb_init_for_xmit() is called for all skbs being sent via ss_do_send(), so a message, transformed splitting the linear part (and setting skb->tail_lock = 1), might go via this code and the kernel will try to amend the tail of the linear part.

Currently kernel can't work with tail_lock. If skb passed to the kernel code with tail_lock = 1 it leads to panic.

I'm not sure if I understood you correctly. We have skb_is_nonlinear() and skb_tailroom_locked() working with tail_lock, which apparently must not crash. There is some bug around tail_lock, which we need to fix, or we should fix both the function to assert BUG_ON(skb->tail_lock).

Does this PR close #1860? If yes, then we need at least to add this issue to #1859 and make much better explanation what's wrong with tail_lock in the issue. I'm confused with the current state of tail_lock in our code.

Answer:
ss_skb_init_for_xmit is called before we pass skb to the kernel code. We can't pass skb to the kernel code with tail_lock = 1, because kernel can pass this skb to the __skb_put and it will lead to crash.

We agreed to add /* TODO #1859 #1860 ... */ comment and merge the PR.

fw/sock.c

fw/hpack.c

fw/ss_skb.c

fw/http_match.c

krizhanovsky

The pull request looks good to merge with the cleanup and data layout improvement. The only topic to discuss, which stops me from approving, is the unclear tail_lock logic. Probably we just should move it to #1859 . Shouldn't we merge #1860 and #1859?

fw/hpack.h

fw/http_parser.c

krizhanovsky · 2023-05-25T21:09:55Z

fw/ss_skb.c

@@ -1311,6 +1318,7 @@ ss_skb_init_for_xmit(struct sk_buff *skb)

 	skb_dst_drop(skb);
 	INIT_LIST_HEAD(&skb->tcp_tsorted_anchor);
+	skb->tail_lock = 0;


I'm not sure if I understood you correctly. We have skb_is_nonlinear() and skb_tailroom_locked() working with tail_lock, which apparently must not crash. There is some bug around tail_lock, which we need to fix, or we should fix both the function to assert BUG_ON(skb->tail_lock).

Does this PR close #1860? If yes, then we need at least to add this issue to #1859 and make much better explanation what's wrong with tail_lock in the issue. I'm confused with the current state of tail_lock in our code.

Answer:
ss_skb_init_for_xmit is called before we pass skb to the kernel code. We can't pass skb to the kernel code with tail_lock = 1, because kernel can pass this skb to the __skb_put and it will lead to crash.

krizhanovsky

LGTM

krizhanovsky · 2023-05-26T15:27:47Z

fw/ss_skb.c

@@ -1311,6 +1318,7 @@ ss_skb_init_for_xmit(struct sk_buff *skb)

 	skb_dst_drop(skb);
 	INIT_LIST_HEAD(&skb->tcp_tsorted_anchor);
+	skb->tail_lock = 0;


We agreed to add /* TODO #1859 #1860 ... */ comment and merge the PR.

We should set skb->tail_lock for the new skbs, because it could be set previously during some skb fragmentation. This field should be zero when we pass skb to the kernel code.

Previously we sent FIN to remote peer during socket closing, but didn't wait for ack to this FIN. Moreover if we could not transmit pending skbs we closed socket and lost these skbs. Now we don't drop connection until last ACK is come.

We should abort and drop all server connections, which are not closed gracefully.

After `skb_fragment` call we should upodate `skb` and `ptr` values using new values from `it`. Also improve work of `ss_skb_cutoff_data` function. If we deal with single skb we should update pointers in TfwStr only when new skb was allocated during `skb_fragment`. Closes #1864

Function `tfw_http_search_host_forwarded` worked incorrectly with TCP segmentation. We can't compare chunk by chunk with "host=", because "host=" can be separated between several chunks.

Before http request trailer headers check, we have to make sure that requerst is fully parsed.

- Change `offset` size to 16 bit and move it before `index` field. - Change `index` size to 32 bit because it is enough and it is better to struct layout. - Remove checking that forwarded host and header host is equal.

- Copy `__extract_request_authority` function to unit tests and use to set req->host value in appropriate tests as we do it in tempesta. - Fix unit tests according to changes in `match_host` function.

Moving body logic extracted to function and do it only if we have the body. Pointer to body never be NULL more safer not use it wthout check the body length.

EvgeniiMekhanik requested review from krizhanovsky and const-t April 21, 2023 15:43

EvgeniiMekhanik force-pushed the MekhanikEvgenii-part-of-1867 branch 2 times, most recently from add4f16 to 6dfe462 Compare April 25, 2023 13:39

EvgeniiMekhanik marked this pull request as draft April 25, 2023 13:39

EvgeniiMekhanik force-pushed the MekhanikEvgenii-part-of-1867 branch from 6dfe462 to 1715bb1 Compare April 26, 2023 14:58

EvgeniiMekhanik marked this pull request as ready for review April 26, 2023 20:58

EvgeniiMekhanik force-pushed the MekhanikEvgenii-part-of-1867 branch from 0c66f11 to 0b640b6 Compare May 3, 2023 11:25

EvgeniiMekhanik marked this pull request as draft May 3, 2023 13:20

EvgeniiMekhanik force-pushed the MekhanikEvgenii-part-of-1867 branch 4 times, most recently from bcfec45 to 76d1d7a Compare May 5, 2023 17:52

EvgeniiMekhanik marked this pull request as ready for review May 5, 2023 17:52

const-t approved these changes May 11, 2023

View reviewed changes

fw/sock.c Show resolved Hide resolved

fw/sync_socket.h Outdated Show resolved Hide resolved

EvgeniiMekhanik force-pushed the MekhanikEvgenii-part-of-1867 branch 4 times, most recently from 45dbefd to a090214 Compare May 18, 2023 09:53

EvgeniiMekhanik requested a review from const-t May 18, 2023 10:01

EvgeniiMekhanik commented May 18, 2023

View reviewed changes

EvgeniiMekhanik force-pushed the MekhanikEvgenii-part-of-1867 branch from a090214 to 67b232a Compare May 18, 2023 11:59

EvgeniiMekhanik added 3 commits May 19, 2023 14:20

Fix invalid hpack decoding

839e734

Continue decode hpack value same as we do it if this value was not devide between several frames.

Fix invalid handling of postponed frames

e617327

In case when frame is not completely received and first part of frame was prossed with error we should not zero `ctx->to_read` value, since we should read the remaining part of the frame.

EvgeniiMekhanik force-pushed the MekhanikEvgenii-part-of-1867 branch 3 times, most recently from f975347 to 6650f76 Compare May 19, 2023 16:42

const-t approved these changes May 22, 2023

View reviewed changes

krizhanovsky requested changes May 22, 2023

View reviewed changes

EvgeniiMekhanik force-pushed the MekhanikEvgenii-part-of-1867 branch 2 times, most recently from 5c0a5f8 to 5b0caf5 Compare May 24, 2023 08:04

EvgeniiMekhanik requested review from krizhanovsky and const-t May 24, 2023 08:04

const-t approved these changes May 25, 2023

View reviewed changes

krizhanovsky reviewed May 25, 2023

View reviewed changes

EvgeniiMekhanik force-pushed the MekhanikEvgenii-part-of-1867 branch 2 times, most recently from 090ea1b to 5082e1f Compare May 26, 2023 08:17

krizhanovsky approved these changes May 26, 2023

View reviewed changes

EvgeniiMekhanik and others added 10 commits May 26, 2023 18:33

Fix error in ss_skb_init_for_xmit function

c620245

We should set skb->tail_lock for the new skbs, because it could be set previously during some skb fragmentation. This field should be zero when we pass skb to the kernel code.

Rework ss_do_close function.

a60ec4b

Previously we sent FIN to remote peer during socket closing, but didn't wait for ack to this FIN. Moreover if we could not transmit pending skbs we closed socket and lost these skbs. Now we don't drop connection until last ACK is come.

Fix according review and fix server connection closing

b530886

We should abort and drop all server connections, which are not closed gracefully.

Increase skb truesize when we move data to fragment

4626678

Fix invalid matching in case of TCP segmentation

30e06f7

Function `tfw_http_search_host_forwarded` worked incorrectly with TCP segmentation. We can't compare chunk by chunk with "host=", because "host=" can be separated between several chunks.

Fix incorrect frang trailer header check

81c6a28

Before http request trailer headers check, we have to make sure that requerst is fully parsed.

Fix according review

cf7b9cb

- Change `offset` size to 16 bit and move it before `index` field. - Change `index` size to 32 bit because it is enough and it is better to struct layout. - Remove checking that forwarded host and header host is equal.

Fix unit tests.

2055a7b

- Copy `__extract_request_authority` function to unit tests and use to set req->host value in appropriate tests as we do it in tempesta. - Fix unit tests according to changes in `match_host` function.

Cosmetic patсh for __tfw_http_msg_expand_from_pool()

2417cda

Moving body logic extracted to function and do it only if we have the body. Pointer to body never be NULL more safer not use it wthout check the body length.

EvgeniiMekhanik force-pushed the MekhanikEvgenii-part-of-1867 branch from 69236c0 to 2417cda Compare May 26, 2023 15:33

krizhanovsky merged commit 9e3b4b4 into master May 26, 2023

krizhanovsky deleted the MekhanikEvgenii-part-of-1867 branch May 26, 2023 15:45

EvgeniiMekhanik mentioned this pull request May 26, 2023

huffman decoder does not work as expected with TCP segmentation 1 byte #1871

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix invalid hpack decoding #1874

Fix invalid hpack decoding #1874

EvgeniiMekhanik commented Apr 21, 2023

const-t left a comment

EvgeniiMekhanik May 18, 2023

const-t May 22, 2023 •

edited

Loading

const-t May 22, 2023 •

edited

Loading

krizhanovsky left a comment •

edited

Loading

krizhanovsky May 22, 2023

EvgeniiMekhanik May 23, 2023

krizhanovsky May 25, 2023 •

edited by EvgeniiMekhanik

Loading

krizhanovsky May 26, 2023

krizhanovsky left a comment

krizhanovsky May 25, 2023 •

edited by EvgeniiMekhanik

Loading

krizhanovsky left a comment

krizhanovsky May 26, 2023

Fix invalid hpack decoding #1874

Fix invalid hpack decoding #1874

Conversation

EvgeniiMekhanik commented Apr 21, 2023

const-t left a comment

Choose a reason for hiding this comment

EvgeniiMekhanik May 18, 2023

Choose a reason for hiding this comment

const-t May 22, 2023 • edited Loading

Choose a reason for hiding this comment

const-t May 22, 2023 • edited Loading

Choose a reason for hiding this comment

krizhanovsky left a comment • edited Loading

Choose a reason for hiding this comment

krizhanovsky May 22, 2023

Choose a reason for hiding this comment

EvgeniiMekhanik May 23, 2023

Choose a reason for hiding this comment

krizhanovsky May 25, 2023 • edited by EvgeniiMekhanik Loading

Choose a reason for hiding this comment

krizhanovsky May 26, 2023

Choose a reason for hiding this comment

krizhanovsky left a comment

Choose a reason for hiding this comment

krizhanovsky May 25, 2023 • edited by EvgeniiMekhanik Loading

Choose a reason for hiding this comment

krizhanovsky left a comment

Choose a reason for hiding this comment

krizhanovsky May 26, 2023

Choose a reason for hiding this comment

const-t May 22, 2023 •

edited

Loading

const-t May 22, 2023 •

edited

Loading

krizhanovsky left a comment •

edited

Loading

krizhanovsky May 25, 2023 •

edited by EvgeniiMekhanik

Loading

krizhanovsky May 25, 2023 •

edited by EvgeniiMekhanik

Loading