| Message ID | CAEkJfYPYF-nNB2oiXfXwjPG0VVB2Bd8Q8kAq+74J=R+4HkngWw@mail.gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,v1] drivers/net/bonding: Fix out-of-bounds read in bond_option_arp_ip_targets_set() | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net |
| netdev/apply | fail | Patch does not apply to net-0 |
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 04/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:792 > 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 author. Also you need a Fixes tag if the patch target is net tree. > 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)) ? Thanks Hangbin > return ret; > -- > 2.34.1
On Mon, Apr 15, 2024 at 3:32 PM 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 04/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:792 > > 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 author. > > 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. > > 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. If anyone finds other problems, please let me know. Thanks, Yue > Thanks > Hangbin > > return ret; > > -- > > 2.34.1
Sam Sun <samsun1006219@gmail.com> wrote: >On Mon, Apr 15, 2024 at 3:32 PM 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 04/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:792 >> > 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 author. >> >> 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
On Mon, Apr 15, 2024 at 11:43 PM Jay Vosburgh <jay.vosburgh@canonical.com> wrote: > 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. > Sure, I will change it in my next submission. > >> > 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 Yes I think you are right. I will remove the target address in fmt string in my next submission. Best, Yue
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);
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 04/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:792 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> Signed-off-by: Yue Sun <samsun1006219@gmail.com> --- drivers/net/bonding/bond_options.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) return ret;