On 11/4/22 4:50 AM, Maurizio Lombardi wrote:
> In case a malicious initiator sends some random data immediately after a
> login PDU; the iscsi_target_sk_data_ready() callback will
> schedule the login_work and, at the same time,
> the negotiation may end without clearing the LOGIN_FLAGS_INITIAL_PDU flag
> (because no additional PDU exchanges are required to complete the login).
>
> The login has been completed but the login_work function
> will find the LOGIN_FLAGS_INITIAL_PDU flag set and will
> never stop from rescheduling itself;
> at this point, if the initiator drops the connection, the iscsit_conn
> structure will be freed, login_work will dereference a released
> socket structure and the kernel crashes.
>
> BUG: kernel NULL pointer dereference, address: 0000000000000230
> PF: supervisor write access in kernel mode
> PF: error_code(0x0002) - not-present page
> Workqueue: events iscsi_target_do_login_rx [iscsi_target_mod]
> RIP: 0010:_raw_read_lock_bh+0x15/0x30
> Call trace:
> iscsi_target_do_login_rx+0x75/0x3f0 [iscsi_target_mod]
> process_one_work+0x1e8/0x3c0
>
> Fix this bug by forcing login_work to stop after the login has been
> completed and the socket callbacks have been restored.
>
> Signed-off-by: Maurizio Lombardi <mlombard@xxxxxxxxxx>
> ---
> drivers/target/iscsi/iscsi_target_nego.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/target/iscsi/iscsi_target_nego.c b/drivers/target/iscsi/iscsi_target_nego.c
> index f2919319ad38..e58bdffe2931 100644
> --- a/drivers/target/iscsi/iscsi_target_nego.c
> +++ b/drivers/target/iscsi/iscsi_target_nego.c
> @@ -1058,6 +1058,7 @@ static int iscsi_target_do_login(struct iscsit_conn *conn, struct iscsi_login *l
> login->tsih = conn->sess->tsih;
> login->login_complete = 1;
> iscsi_target_restore_sock_callbacks(conn);
> + cancel_delayed_work_sync(&conn->login_work);
You can remove the cancel_delayed_work in iscsi_target_do_login_rx
in the chunk that checks for 1 being returned since you do it here
now.
For the error path, I think you could also move the cancel_delayed_work_sync
from them and put it in here.
If we leave it to the callers, in iscsi_target_do_login_rx in the "goto err"
handling should this be reversed and do you want to use the _sync call like above?
iscsi_target_restore_sock_callbacks(conn);
cancel_delayed_work(&conn->login_work);
> if (iscsi_target_do_tx_login_io(conn,
> login) < 0)
> return -1;
