| Message ID | 20220802101939.3972556-1-lijinlin3@huawei.com |
|---|---|
| State | New |
| Headers | show |
| Series | scsi: iscsi: iscsi_tcp: Fix null-ptr-deref while calling getpeername() | expand |
On 8/3/2022 12:25 AM, Mike Christie wrote: > On 8/2/22 6:23 AM, lijinlin (A) wrote: >> So sorry, this patch has problem, please ignore. >> > > Was the issue the fget use? >> I know I gave the suggestion to do the get, but seeing it now makes > me think I was wrong and it's getting too messy. > I use get_file() in local, and test the patch can fix this null-ptr-deref. But I got an INFO report as below, it only appears once in multiple tests. I'm not sure if this info report represents a possible problem with the patch. So I ask for ignore it. INFO: trying to register non-static key. The code is fine but needs lockdep annotation, or maybe you didn't initialize this object before use? turning off the locking correctness validator. CPU: 21 PID: 1074 Comm: cat Not tainted 5.19.0 #44 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.15.0-0-g2dd4b9b3f840-prebuilt.qemu.org 04/01/2014 Call Trace: <TASK> dump_stack_lvl+0x49/0x63 dump_stack+0x10/0x16 register_lock_class+0x483/0x490 ? reacquire_held_locks+0xcb/0x1e0 ? release_sock+0x1e/0xb0 __lock_acquire.constprop.0+0x4e/0x530 ? lock_release+0x142/0x2d0 lock_acquire+0xc3/0x1b0 ? iscsi_sw_tcp_host_get_param+0xa4/0x120 _raw_spin_lock_bh+0x34/0x50 ? iscsi_sw_tcp_host_get_param+0xa4/0x120 iscsi_sw_tcp_host_get_param+0xa4/0x120 show_host_param_ISCSI_HOST_PARAM_IPADDRESS+0x56/0x70 dev_attr_show+0x1d/0x50 sysfs_kf_seq_show+0xad/0x120 kernfs_seq_show+0x2c/0x40 seq_read_iter+0x12e/0x4d0 ? aa_file_perm+0x177/0x5a0 kernfs_fop_read_iter+0x183/0x210 new_sync_read+0xfe/0x180 ? 0xffffffff81000000 vfs_read+0x14d/0x1a0 ksys_read+0x6d/0xf0 __x64_sys_read+0x1a/0x20 do_syscall_64+0x3b/0x90 entry_SYSCALL_64_after_hwframe+0x63/0xcd > Let's just add a mutex for getting/setting the tcp_sw_conn->sock in > the non-io paths (io paths are flushed/locked already). Something like > this (patch is only compile tested): > This patch is clean, I have tested it and it is effective. Please push this patch to the mainline, Thanks. Jinlin > diff --git a/drivers/scsi/iscsi_tcp.c b/drivers/scsi/iscsi_tcp.c > index 9fee70d6434a..c1696472965e 100644 > --- a/drivers/scsi/iscsi_tcp.c > +++ b/drivers/scsi/iscsi_tcp.c
Hi Li,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on mkp-scsi/for-next]
[also build test WARNING on jejb-scsi/for-next linus/master v5.19 next-20220728]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Li-Jinlin/scsi-iscsi-iscsi_tcp-Fix-null-ptr-deref-while-calling-getpeername/20220802-173945
base: https://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git for-next
config: x86_64-allyesconfig (https://download.01.org/0day-ci/archive/20220803/202208032214.0FELL5gK-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-3) 11.3.0
reproduce (this is a W=1 build):
# https://github.com/intel-lab-lkp/linux/commit/ccc367df3fdba07b24eeda721ca928cce50f40d2
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Li-Jinlin/scsi-iscsi-iscsi_tcp-Fix-null-ptr-deref-while-calling-getpeername/20220802-173945
git checkout ccc367df3fdba07b24eeda721ca928cce50f40d2
# save the config file
mkdir build_dir && cp config build_dir/.config
make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/scsi/
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
drivers/scsi/iscsi_tcp.c: In function 'iscsi_sw_tcp_conn_get_param':
>> drivers/scsi/iscsi_tcp.c:798:26: warning: passing argument 1 of 'fget' makes integer from pointer without a cast [-Wint-conversion]
798 | fget(sock->file);
| ~~~~^~~~~~
| |
| struct file *
In file included from drivers/scsi/iscsi_tcp.c:25:
include/linux/file.h:48:39: note: expected 'unsigned int' but argument is of type 'struct file *'
48 | extern struct file *fget(unsigned int fd);
| ~~~~~~~~~~~~~^~
drivers/scsi/iscsi_tcp.c: In function 'iscsi_sw_tcp_host_get_param':
drivers/scsi/iscsi_tcp.c:852:26: warning: passing argument 1 of 'fget' makes integer from pointer without a cast [-Wint-conversion]
852 | fget(sock->file);
| ~~~~^~~~~~
| |
| struct file *
In file included from drivers/scsi/iscsi_tcp.c:25:
include/linux/file.h:48:39: note: expected 'unsigned int' but argument is of type 'struct file *'
48 | extern struct file *fget(unsigned int fd);
| ~~~~~~~~~~~~~^~
vim +/fget +798 drivers/scsi/iscsi_tcp.c
777
778 static int iscsi_sw_tcp_conn_get_param(struct iscsi_cls_conn *cls_conn,
779 enum iscsi_param param, char *buf)
780 {
781 struct iscsi_conn *conn = cls_conn->dd_data;
782 struct iscsi_tcp_conn *tcp_conn = conn->dd_data;
783 struct iscsi_sw_tcp_conn *tcp_sw_conn = tcp_conn->dd_data;
784 struct sockaddr_in6 addr;
785 struct socket *sock;
786 int rc;
787
788 switch(param) {
789 case ISCSI_PARAM_CONN_PORT:
790 case ISCSI_PARAM_CONN_ADDRESS:
791 case ISCSI_PARAM_LOCAL_PORT:
792 spin_lock_bh(&conn->session->frwd_lock);
793 if (!tcp_sw_conn || !tcp_sw_conn->sock) {
794 spin_unlock_bh(&conn->session->frwd_lock);
795 return -ENOTCONN;
796 }
797 sock = tcp_sw_conn->sock;
> 798 fget(sock->file);
799 spin_unlock_bh(&conn->session->frwd_lock);
800
801 if (param == ISCSI_PARAM_LOCAL_PORT)
802 rc = kernel_getsockname(sock,
803 (struct sockaddr *)&addr);
804 else
805 rc = kernel_getpeername(sock,
806 (struct sockaddr *)&addr);
807 spin_lock_bh(&conn->session->frwd_lock);
808 sockfd_put(sock);
809 spin_unlock_bh(&conn->session->frwd_lock);
810 if (rc < 0)
811 return rc;
812
813 return iscsi_conn_get_addr_param((struct sockaddr_storage *)
814 &addr, param, buf);
815 default:
816 return iscsi_conn_get_param(cls_conn, param, buf);
817 }
818
819 return 0;
820 }
821
Hi Li,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on mkp-scsi/for-next]
[also build test ERROR on jejb-scsi/for-next linus/master v5.19 next-20220805]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Li-Jinlin/scsi-iscsi-iscsi_tcp-Fix-null-ptr-deref-while-calling-getpeername/20220802-173945
base: https://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git for-next
config: x86_64-randconfig-r033-20220801 (https://download.01.org/0day-ci/archive/20220808/202208080020.xQk6IIBw-lkp@intel.com/config)
compiler: clang version 16.0.0 (https://github.com/llvm/llvm-project 52cd00cabf479aa7eb6dbb063b7ba41ea57bce9e)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/ccc367df3fdba07b24eeda721ca928cce50f40d2
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Li-Jinlin/scsi-iscsi-iscsi_tcp-Fix-null-ptr-deref-while-calling-getpeername/20220802-173945
git checkout ccc367df3fdba07b24eeda721ca928cce50f40d2
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/scsi/
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
>> drivers/scsi/iscsi_tcp.c:798:8: error: incompatible pointer to integer conversion passing 'struct file *' to parameter of type 'unsigned int' [-Wint-conversion]
fget(sock->file);
^~~~~~~~~~
include/linux/file.h:48:39: note: passing argument to parameter 'fd' here
extern struct file *fget(unsigned int fd);
^
drivers/scsi/iscsi_tcp.c:852:8: error: incompatible pointer to integer conversion passing 'struct file *' to parameter of type 'unsigned int' [-Wint-conversion]
fget(sock->file);
^~~~~~~~~~
include/linux/file.h:48:39: note: passing argument to parameter 'fd' here
extern struct file *fget(unsigned int fd);
^
2 errors generated.
vim +798 drivers/scsi/iscsi_tcp.c
777
778 static int iscsi_sw_tcp_conn_get_param(struct iscsi_cls_conn *cls_conn,
779 enum iscsi_param param, char *buf)
780 {
781 struct iscsi_conn *conn = cls_conn->dd_data;
782 struct iscsi_tcp_conn *tcp_conn = conn->dd_data;
783 struct iscsi_sw_tcp_conn *tcp_sw_conn = tcp_conn->dd_data;
784 struct sockaddr_in6 addr;
785 struct socket *sock;
786 int rc;
787
788 switch(param) {
789 case ISCSI_PARAM_CONN_PORT:
790 case ISCSI_PARAM_CONN_ADDRESS:
791 case ISCSI_PARAM_LOCAL_PORT:
792 spin_lock_bh(&conn->session->frwd_lock);
793 if (!tcp_sw_conn || !tcp_sw_conn->sock) {
794 spin_unlock_bh(&conn->session->frwd_lock);
795 return -ENOTCONN;
796 }
797 sock = tcp_sw_conn->sock;
> 798 fget(sock->file);
799 spin_unlock_bh(&conn->session->frwd_lock);
800
801 if (param == ISCSI_PARAM_LOCAL_PORT)
802 rc = kernel_getsockname(sock,
803 (struct sockaddr *)&addr);
804 else
805 rc = kernel_getpeername(sock,
806 (struct sockaddr *)&addr);
807 spin_lock_bh(&conn->session->frwd_lock);
808 sockfd_put(sock);
809 spin_unlock_bh(&conn->session->frwd_lock);
810 if (rc < 0)
811 return rc;
812
813 return iscsi_conn_get_addr_param((struct sockaddr_storage *)
814 &addr, param, buf);
815 default:
816 return iscsi_conn_get_param(cls_conn, param, buf);
817 }
818
819 return 0;
820 }
821
Hi Li,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on mkp-scsi/for-next]
[also build test WARNING on jejb-scsi/for-next linus/master v5.19]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Li-Jinlin/scsi-iscsi-iscsi_tcp-Fix-null-ptr-deref-while-calling-getpeername/20220802-173945
base: https://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git for-next
config: powerpc-randconfig-s051-20220801 (https://download.01.org/0day-ci/archive/20220808/202208081109.mO6WgY4E-lkp@intel.com/config)
compiler: powerpc-linux-gcc (GCC) 12.1.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# apt-get install sparse
# sparse version: v0.6.4-39-gce1a6720-dirty
# https://github.com/intel-lab-lkp/linux/commit/ccc367df3fdba07b24eeda721ca928cce50f40d2
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Li-Jinlin/scsi-iscsi-iscsi_tcp-Fix-null-ptr-deref-while-calling-getpeername/20220802-173945
git checkout ccc367df3fdba07b24eeda721ca928cce50f40d2
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=powerpc SHELL=/bin/bash drivers/scsi/
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>
sparse warnings: (new ones prefixed by >>)
>> drivers/scsi/iscsi_tcp.c:798:26: sparse: sparse: incorrect type in argument 1 (different base types) @@ expected unsigned int fd @@ got struct file *file @@
drivers/scsi/iscsi_tcp.c:798:26: sparse: expected unsigned int fd
drivers/scsi/iscsi_tcp.c:798:26: sparse: got struct file *file
drivers/scsi/iscsi_tcp.c:852:26: sparse: sparse: incorrect type in argument 1 (different base types) @@ expected unsigned int fd @@ got struct file *file @@
drivers/scsi/iscsi_tcp.c:852:26: sparse: expected unsigned int fd
drivers/scsi/iscsi_tcp.c:852:26: sparse: got struct file *file
vim +798 drivers/scsi/iscsi_tcp.c
777
778 static int iscsi_sw_tcp_conn_get_param(struct iscsi_cls_conn *cls_conn,
779 enum iscsi_param param, char *buf)
780 {
781 struct iscsi_conn *conn = cls_conn->dd_data;
782 struct iscsi_tcp_conn *tcp_conn = conn->dd_data;
783 struct iscsi_sw_tcp_conn *tcp_sw_conn = tcp_conn->dd_data;
784 struct sockaddr_in6 addr;
785 struct socket *sock;
786 int rc;
787
788 switch(param) {
789 case ISCSI_PARAM_CONN_PORT:
790 case ISCSI_PARAM_CONN_ADDRESS:
791 case ISCSI_PARAM_LOCAL_PORT:
792 spin_lock_bh(&conn->session->frwd_lock);
793 if (!tcp_sw_conn || !tcp_sw_conn->sock) {
794 spin_unlock_bh(&conn->session->frwd_lock);
795 return -ENOTCONN;
796 }
797 sock = tcp_sw_conn->sock;
> 798 fget(sock->file);
799 spin_unlock_bh(&conn->session->frwd_lock);
800
801 if (param == ISCSI_PARAM_LOCAL_PORT)
802 rc = kernel_getsockname(sock,
803 (struct sockaddr *)&addr);
804 else
805 rc = kernel_getpeername(sock,
806 (struct sockaddr *)&addr);
807 spin_lock_bh(&conn->session->frwd_lock);
808 sockfd_put(sock);
809 spin_unlock_bh(&conn->session->frwd_lock);
810 if (rc < 0)
811 return rc;
812
813 return iscsi_conn_get_addr_param((struct sockaddr_storage *)
814 &addr, param, buf);
815 default:
816 return iscsi_conn_get_param(cls_conn, param, buf);
817 }
818
819 return 0;
820 }
821
diff --git a/drivers/scsi/iscsi_tcp.c b/drivers/scsi/iscsi_tcp.c index 9fee70d6434a..63532dc3970d 100644 --- a/drivers/scsi/iscsi_tcp.c +++ b/drivers/scsi/iscsi_tcp.c @@ -612,8 +612,8 @@ static void iscsi_sw_tcp_release_conn(struct iscsi_conn *conn) spin_lock_bh(&session->frwd_lock); tcp_sw_conn->sock = NULL; - spin_unlock_bh(&session->frwd_lock); sockfd_put(sock); + spin_unlock_bh(&session->frwd_lock); } static void iscsi_sw_tcp_conn_destroy(struct iscsi_cls_conn *cls_conn) @@ -756,7 +756,7 @@ static int iscsi_sw_tcp_conn_get_param(struct iscsi_cls_conn *cls_conn, return -ENOTCONN; } sock = tcp_sw_conn->sock; - sock_hold(sock->sk); + fget(sock->file); spin_unlock_bh(&conn->session->frwd_lock); if (param == ISCSI_PARAM_LOCAL_PORT) @@ -765,7 +765,9 @@ static int iscsi_sw_tcp_conn_get_param(struct iscsi_cls_conn *cls_conn, else rc = kernel_getpeername(sock, (struct sockaddr *)&addr); - sock_put(sock->sk); + spin_lock_bh(&conn->session->frwd_lock); + sockfd_put(sock); + spin_unlock_bh(&conn->session->frwd_lock); if (rc < 0) return rc; @@ -808,12 +810,14 @@ static int iscsi_sw_tcp_host_get_param(struct Scsi_Host *shost, spin_unlock_bh(&session->frwd_lock); return -ENOTCONN; } - sock_hold(sock->sk); + fget(sock->file); spin_unlock_bh(&session->frwd_lock); rc = kernel_getsockname(sock, (struct sockaddr *)&addr); - sock_put(sock->sk); + spin_lock_bh(&conn->session->frwd_lock); + sockfd_put(sock); + spin_unlock_bh(&conn->session->frwd_lock); if (rc < 0) return rc;
I got a kernel NULL pointer derference report as below: BUG: kernel NULL pointer dereference, address: 0000000000000038 CPU: 4 PID: 1050 Comm: cat Not tainted 5.19.0 #38 RIP: 0010:kernel_getpeername+0xf/0x30 Call Trace: <TASK> ? iscsi_sw_tcp_conn_get_param+0x11f/0x17f show_conn_ep_param_ISCSI_PARAM_CONN_ADDRESS+0x90/0xb0 dev_attr_show+0x1d/0x50 sysfs_kf_seq_show+0xad/0x120 kernfs_seq_show+0x2c/0x40 seq_read_iter+0x12e/0x4d0 ? aa_file_perm+0x177/0x590 kernfs_fop_read_iter+0x183/0x210 new_sync_read+0xfe/0x180 ? 0xffffffff81000000 vfs_read+0x14d/0x1a0 ksys_read+0x6d/0xf0 __x64_sys_read+0x1a/0x20 do_syscall_64+0x3b/0x90 entry_SYSCALL_64_after_hwframe+0x63/0xcd The problem scenario is: CPU1 CPU2 ------------------------- ------------------------ iscsi_sw_tcp_conn_get_param spin_lock_bh(&frwd_lock) if (!tcp_sw_conn || !tcp_sw_conn->sock) spin_unlock_bh(&frwd_lock) return -ENOTCONN sock = tcp_sw_conn->sock; sock_hold(sock->sk) spin_unlock_bh(&frwd_lock) iscsi_sw_tcp_release_conn spin_lock_bh(&frwd_lock) tcp_sw_conn->sock = NULL spin_unlock_bh(&frwd_lock) sockfd_put(sock) task_work __fput sock_close __sock_release sock->sk = NULL sock->ops = NULL sock->file = NULL kernel_getpeername sock->ops->getname ^^^^^^^^^ NULL pointer dereference of sock->ops sock_hold() and sock_put() can't ensure that sock_close() won't be called before calling getpeername() or getsockname(). Using fget() and sockfd_put() replace sock_hold() and sock_put(), and put them under frwd_lock protection, to ensure that the socket struct is preserved until after the getpeernaem() or getsockname() complete. Fixes: bcf3a2953d36 ("scsi: iscsi: iscsi_tcp: Avoid holding spinlock while calling getpeername()") Signed-off-by: Li Jinlin <lijinlin3@huawei.com> --- drivers/scsi/iscsi_tcp.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-)