| Message ID | tencent_146C309740E8F6ECD2CC5C7ADA6E202D450A@qq.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | tls: fix WARNING in __sk_msg_free | expand |
Edward Adam Davis wrote: > Syzbot constructed 32 scatterlists, and the data members in struct sk_msg_sg > can only store a maximum of MAX_MSG_FRAGS scatterlists. > However, the value of MAX_MSG_FRAGS=CONFIG_MAX_SKB_FRAG is less than 32, which > leads to the warning reported here. > > Prevent similar issues from occurring by checking whether sg.end is greater > than MAX_MSG_FRAGS. > > Reported-and-tested-by: syzbot+f2977222e0e95cec15c8@syzkaller.appspotmail.com > Signed-off-by: Edward Adam Davis <eadavis@qq.com> > --- > net/tls/tls_sw.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c > index e37b4d2e2acd..68dbe821f61d 100644 > --- a/net/tls/tls_sw.c > +++ b/net/tls/tls_sw.c > @@ -1016,6 +1016,8 @@ static int tls_sw_sendmsg_locked(struct sock *sk, struct msghdr *msg, > > msg_pl = &rec->msg_plaintext; > msg_en = &rec->msg_encrypted; > + if (msg_pl->sg.end >= MAX_MSG_FRAGS) > + return -EINVAL; > > orig_size = msg_pl->sg.size; > full_record = false; > -- > 2.43.0 > I'll test this in a bit, but I suspect this error is because even if the msg_pl is full (the sg.end == MAX_MSG_FRAGS) the code is missing a full_record=true set to force the loop to do the send and abort. My opinion is we should never iterated the loop if the msg_pl was full. I think something like this is actually needed. diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c index e37b4d2e2acd..9cfa6f8d51e3 100644 --- a/net/tls/tls_sw.c +++ b/net/tls/tls_sw.c @@ -1052,8 +1052,10 @@ static int tls_sw_sendmsg_locked(struct sock *sk, struct msghdr *msg, if (ret < 0) goto send_end; tls_ctx->pending_open_record_frags = true; - if (full_record || eor || sk_msg_full(msg_pl)) + if (full_record || eor || sk_msg_full(msg_pl)) { + full_record = true; goto copied; + } continue; }
John Fastabend wrote: > Edward Adam Davis wrote: > > Syzbot constructed 32 scatterlists, and the data members in struct sk_msg_sg > > can only store a maximum of MAX_MSG_FRAGS scatterlists. > > However, the value of MAX_MSG_FRAGS=CONFIG_MAX_SKB_FRAG is less than 32, which > > leads to the warning reported here. > > > > Prevent similar issues from occurring by checking whether sg.end is greater > > than MAX_MSG_FRAGS. > > > > Reported-and-tested-by: syzbot+f2977222e0e95cec15c8@syzkaller.appspotmail.com > > Signed-off-by: Edward Adam Davis <eadavis@qq.com> > > --- > > net/tls/tls_sw.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c > > index e37b4d2e2acd..68dbe821f61d 100644 > > --- a/net/tls/tls_sw.c > > +++ b/net/tls/tls_sw.c > > @@ -1016,6 +1016,8 @@ static int tls_sw_sendmsg_locked(struct sock *sk, struct msghdr *msg, > > > > msg_pl = &rec->msg_plaintext; > > msg_en = &rec->msg_encrypted; > > + if (msg_pl->sg.end >= MAX_MSG_FRAGS) > > + return -EINVAL; > > > > orig_size = msg_pl->sg.size; > > full_record = false; > > -- > > 2.43.0 > > > > I'll test this in a bit, but I suspect this error is because even > if the msg_pl is full (the sg.end == MAX_MSG_FRAGS) the code is > missing a full_record=true set to force the loop to do the send > and abort. My opinion is we should never iterated the loop if the > msg_pl was full. > > I think something like this is actually needed. > > diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c > index e37b4d2e2acd..9cfa6f8d51e3 100644 > --- a/net/tls/tls_sw.c > +++ b/net/tls/tls_sw.c > @@ -1052,8 +1052,10 @@ static int tls_sw_sendmsg_locked(struct sock *sk, struct msghdr *msg, > if (ret < 0) > goto send_end; > tls_ctx->pending_open_record_frags = true; > - if (full_record || eor || sk_msg_full(msg_pl)) > + if (full_record || eor || sk_msg_full(msg_pl)) { > + full_record = true; > goto copied; > + } > continue; > } Actually, it needs a bit more than above. That will fix the warning, but it returns an error on when it should flush the full_record in some cases. I'll send a fix shortly.
diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c index e37b4d2e2acd..68dbe821f61d 100644 --- a/net/tls/tls_sw.c +++ b/net/tls/tls_sw.c @@ -1016,6 +1016,8 @@ static int tls_sw_sendmsg_locked(struct sock *sk, struct msghdr *msg, msg_pl = &rec->msg_plaintext; msg_en = &rec->msg_encrypted; + if (msg_pl->sg.end >= MAX_MSG_FRAGS) + return -EINVAL; orig_size = msg_pl->sg.size; full_record = false;
Syzbot constructed 32 scatterlists, and the data members in struct sk_msg_sg can only store a maximum of MAX_MSG_FRAGS scatterlists. However, the value of MAX_MSG_FRAGS=CONFIG_MAX_SKB_FRAG is less than 32, which leads to the warning reported here. Prevent similar issues from occurring by checking whether sg.end is greater than MAX_MSG_FRAGS. Reported-and-tested-by: syzbot+f2977222e0e95cec15c8@syzkaller.appspotmail.com Signed-off-by: Edward Adam Davis <eadavis@qq.com> --- net/tls/tls_sw.c | 2 ++ 1 file changed, 2 insertions(+)