From: 王贇 <yun.wang@linux.alibaba.com>
To: Jakub Kicinski <kuba@kernel.org>
Cc: Jamal Hadi Salim <jhs@mojatatu.com>,
Cong Wang <xiyou.wangcong@gmail.com>,
Jiri Pirko <jiri@resnulli.us>,
"David S. Miller" <davem@davemloft.net>,
"open list:TC subsystem" <netdev@vger.kernel.org>
Subject: Re: [PATCH] net: prevent user from passing illegal stab size
Date: Fri, 24 Sep 2021 10:13:33 +0800 [thread overview]
Message-ID: <d124f8c3-b8b5-f086-330a-a4e5252778a5@linux.alibaba.com> (raw)
In-Reply-To: <20210923090054.3556deb0@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
On 2021/9/24 上午12:00, Jakub Kicinski wrote:
> On Thu, 23 Sep 2021 17:08:13 +0800 王贇 wrote:
>> We observed below report when playing with netlink sock:
>>
>> UBSAN: shift-out-of-bounds in net/sched/sch_api.c:580:10
>> shift exponent 249 is too large for 32-bit type
>> CPU: 0 PID: 685 Comm: a.out Not tainted
>> Call Trace:
>> dump_stack_lvl+0x8d/0xcf
>> ubsan_epilogue+0xa/0x4e
>> __ubsan_handle_shift_out_of_bounds+0x161/0x182
>> __qdisc_calculate_pkt_len+0xf0/0x190
>> __dev_queue_xmit+0x2ed/0x15b0
>>
>> it seems like kernel won't check the stab size_log passing from
>> user, and will use the insane value later to calculate pkt_len.
>>
>> This patch just add a check on the size_log to avoid insane
>> calculation.
>>
>> Reported-by: Abaci <abaci@linux.alibaba.com>
>> Signed-off-by: Michael Wang <yun.wang@linux.alibaba.com>
>> ---
>> include/uapi/linux/pkt_sched.h | 1 +
>> net/sched/sch_api.c | 3 +++
>> 2 files changed, 4 insertions(+)
>>
>> diff --git a/include/uapi/linux/pkt_sched.h b/include/uapi/linux/pkt_sched.h
>> index ec88590..fa194a0 100644
>> --- a/include/uapi/linux/pkt_sched.h
>> +++ b/include/uapi/linux/pkt_sched.h
>> @@ -98,6 +98,7 @@ struct tc_ratespec {
>> };
>>
>> #define TC_RTAB_SIZE 1024
>> +#define TC_LOG_MAX 30
>
> Adding a uAPI define is not necessary.
Yes, I'll move this to somewhere else.
>
>> struct tc_sizespec {
>> unsigned char cell_log;
>> diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
>> index 5e90e9b..1b6b8f8 100644
>> --- a/net/sched/sch_api.c
>> +++ b/net/sched/sch_api.c
>> @@ -513,6 +513,9 @@ static struct qdisc_size_table *qdisc_get_stab(struct nlattr *opt,
>> return stab;
>> }
>>
>> + if (s->size_log > TC_LOG_MAX)
>> + return ERR_PTR(-EINVAL);
>
> Looks sane, please add an extack message.
>
> Why not cover cell_log as well while at it?
You're right, will add message and check this one too in v2 :-)
Regards,
Michael Wang
>
>> stab = kmalloc(sizeof(*stab) + tsize * sizeof(u16), GFP_KERNEL);
>> if (!stab)
>> return ERR_PTR(-ENOMEM);
next prev parent reply other threads:[~2021-09-24 2:13 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-09-23 9:08 [PATCH] net: prevent user from passing illegal stab size 王贇
2021-09-23 16:00 ` Jakub Kicinski
2021-09-24 2:13 ` 王贇 [this message]
2021-09-24 2:35 ` [PATCH v2] " 王贇
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=d124f8c3-b8b5-f086-330a-a4e5252778a5@linux.alibaba.com \
--to=yun.wang@linux.alibaba.com \
--cc=davem@davemloft.net \
--cc=jhs@mojatatu.com \
--cc=jiri@resnulli.us \
--cc=kuba@kernel.org \
--cc=netdev@vger.kernel.org \
--cc=xiyou.wangcong@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.