Received-SPF: pass (google.com: domain of linux-kernel+bounces-145478-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
From: Jay Vosburgh <jay.vosburgh@canonical.com>
To: Sam Sun <samsun1006219@gmail.com>
cc: Hangbin Liu <liuhangbin@gmail.com>, linux-kernel@vger.kernel.org,
    netdev@vger.kernel.org, andy@greyhouse.net, davem@davemloft.net,
    Eric Dumazet <edumazet@google.com>, kuba@kernel.org,
    pabeni@redhat.com
Subject: Re: [PATCH net v1] drivers/net/bonding: Fix out-of-bounds read in bond_option_arp_ip_targets_set()
In-reply-to: <CAEkJfYOebGdmKLtn4HXHJ2-CMzig=M+Sc7T0d6ghZcXY_iY5YA@mail.gmail.com>
References: <CAEkJfYPYF-nNB2oiXfXwjPG0VVB2Bd8Q8kAq+74J=R+4HkngWw@mail.gmail.com> <ZhzYCZyfsWgYWxIe@Laptop-X1> <CAEkJfYOebGdmKLtn4HXHJ2-CMzig=M+Sc7T0d6ghZcXY_iY5YA@mail.gmail.com>
Comments: In-reply-to Sam Sun <samsun1006219@gmail.com>
   message dated "Mon, 15 Apr 2024 16:46:24 +0800."
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Date: Mon, 15 Apr 2024 08:43:08 -0700
Message-ID: <12281.1713195788@famine>

Sam Sun <samsun1006219@gmail.com> wrote:

>On Mon, Apr 15, 2024 at 3:32=E2=80=AFPM Hangbin Liu <liuhangbin@gmail.com>=
 wrote:
>>
>> On Mon, Apr 15, 2024 at 11:40:31AM +0800, Sam Sun wrote:
>> > In function bond_option_arp_ip_targets_set(), if newval->string is an
>> > empty string, newval->string+1 will point to the byte after the
>> > string, causing an out-of-bound read.
>> >
>> > BUG: KASAN: slab-out-of-bounds in strlen+0x7d/0xa0 lib/string.c:418
>> > Read of size 1 at addr ffff8881119c4781 by task syz-executor665/8107
>> > CPU: 1 PID: 8107 Comm: syz-executor665 Not tainted 6.7.0-rc7 #1
>> > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.15.0-1 0=
4/01/2014
>> > Call Trace:
>> >  <TASK>
>> >  __dump_stack lib/dump_stack.c:88 [inline]
>> >  dump_stack_lvl+0xd9/0x150 lib/dump_stack.c:106
>> >  print_address_description mm/kasan/report.c:364 [inline]
>> >  print_report+0xc1/0x5e0 mm/kasan/report.c:475
>> >  kasan_report+0xbe/0xf0 mm/kasan/report.c:588
>> >  strlen+0x7d/0xa0 lib/string.c:418
>> >  __fortify_strlen include/linux/fortify-string.h:210 [inline]
>> >  in4_pton+0xa3/0x3f0 net/core/utils.c:130
>> >  bond_option_arp_ip_targets_set+0xc2/0x910
>> > drivers/net/bonding/bond_options.c:1201
>> >  __bond_opt_set+0x2a4/0x1030 drivers/net/bonding/bond_options.c:767
>> >  __bond_opt_set_notify+0x48/0x150 drivers/net/bonding/bond_options.c:7=
92
>> >  bond_opt_tryset_rtnl+0xda/0x160 drivers/net/bonding/bond_options.c:817
>> >  bonding_sysfs_store_option+0xa1/0x120 drivers/net/bonding/bond_sysfs.=
c:156
>> >  dev_attr_store+0x54/0x80 drivers/base/core.c:2366
>> >  sysfs_kf_write+0x114/0x170 fs/sysfs/file.c:136
>> >  kernfs_fop_write_iter+0x337/0x500 fs/kernfs/file.c:334
>> >  call_write_iter include/linux/fs.h:2020 [inline]
>> >  new_sync_write fs/read_write.c:491 [inline]
>> >  vfs_write+0x96a/0xd80 fs/read_write.c:584
>> >  ksys_write+0x122/0x250 fs/read_write.c:637
>> >  do_syscall_x64 arch/x86/entry/common.c:52 [inline]
>> >  do_syscall_64+0x40/0x110 arch/x86/entry/common.c:83
>> >  entry_SYSCALL_64_after_hwframe+0x63/0x6b
>> > ---[ end trace ]---
>> >
>> > Fix it by adding a check of string length before using it.
>> >
>> > Reported-by: Yue Sun <samsun1006219@gmail.com>
>>
>> Not sure if there is a need to add Reported-by yourself if you are the a=
uthor.
>>
>> Also you need a Fixes tag if the patch target is net tree.
>
>Sorry for missing the Fixes tag, I will add it to patch. I am also not
>sure if I should add Reported-by here, since it's my first time to
>commit a patch for linux.

	The submitting-patches.rst file in Documentation/ isn't
explicit, but the intent seems to be that Reported-by is for a bug
report from a third party that isn't involved in creating the fix.  I
don't think you need it here, just a Signed-off-by.

>> > Signed-off-by: Yue Sun <samsun1006219@gmail.com>
>> > ---
>> >  drivers/net/bonding/bond_options.c | 3 ++-
>> >  1 file changed, 2 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/drivers/net/bonding/bond_options.c
>> > b/drivers/net/bonding/bond_options.c
>> > index 4cdbc7e084f4..db8d99ca1de0 100644
>> > --- a/drivers/net/bonding/bond_options.c
>> > +++ b/drivers/net/bonding/bond_options.c
>> > @@ -1214,7 +1214,8 @@ static int bond_option_arp_ip_targets_set(struct
>> > bonding *bond,
>> >      __be32 target;
>> >
>> >      if (newval->string) {
>> > -        if (!in4_pton(newval->string+1, -1, (u8 *)&target, -1, NULL))=
 {
>> > +        if (!(strlen(newval->string)) ||
>> > +            !in4_pton(newval->string + 1, -1, (u8 *)&target, -1, NULL=
)) {
>> >              netdev_err(bond->dev, "invalid ARP target %pI4 specified\=
n",
>> >                     &target);
>>
>> Do we need to init target first if !(strlen(newval->string)) ?
>>
>Good question. I think we don't need to init target first, since in
>original logic in4_pton() also leave target untouched if any error
>occurs. If !(strlen(newval->string)), bond_option_arp_ip_targets_set()
>just ret and target is still untouched. But I am not sure about it.

	I think the original code is incorrect, as target will be
uninitialized if in4_pton() fails.  The netdev_err() message shouldn't
include target at all, it will never contain useful information.

	-J

>If anyone finds other problems, please let me know.
>
>Thanks,
>Yue
>> Thanks
>> Hangbin
>> >              return ret;
>> > --
>> > 2.34.1
>

---
	-Jay Vosburgh, jay.vosburgh@canonical.com