Skip to content

Commit 971dfcb

Browse files
f0rm2l1nmartinkpetersen
f0rm2l1n
authored and
martinkpetersen
committed
scsi: iscsi: Add length check for nlattr payload
The current NETLINK_ISCSI netlink parsing loop checks every nlmsg to make sure the length is bigger than sizeof(struct iscsi_uevent) and then calls iscsi_if_recv_msg(). nlh = nlmsg_hdr(skb); if (nlh->nlmsg_len < sizeof(*nlh) + sizeof(*ev) || skb->len < nlh->nlmsg_len) { break; } ... err = iscsi_if_recv_msg(skb, nlh, &group); Hence, in iscsi_if_recv_msg() the nlmsg_data can be safely converted to iscsi_uevent as the length is already checked. However, in other cases the length of nlattr payload is not checked before the payload is converted to other data structures. One example is iscsi_set_path() which converts the payload to type iscsi_path without any checks: params = (struct iscsi_path *)((char *)ev + sizeof(*ev)); Whereas iscsi_if_transport_conn() correctly checks the pdu_len: pdu_len = nlh->nlmsg_len - sizeof(*nlh) - sizeof(*ev); if ((ev->u.send_pdu.hdr_size > pdu_len) .. err = -EINVAL; To sum up, some code paths called in iscsi_if_recv_msg() do not check the length of the data (see below picture) and directly convert the data to another data structure. This could result in an out-of-bound reads and heap dirty data leakage. _________ nlmsg_len(nlh) _______________ / \ +----------+--------------+---------------------------+ | nlmsghdr | iscsi_uevent | data | +----------+--------------+---------------------------+ \ / iscsi_uevent->u.set_param.len Fix the issue by adding the length check before accessing it. To clean up the code, an additional parameter named rlen is added. The rlen is calculated at the beginning of iscsi_if_recv_msg() which avoids duplicated calculation. Fixes: ac20c7b ("[SCSI] iscsi_transport: Added Ping support") Fixes: 4351477 ("[SCSI] iscsi class: Add new NETLINK_ISCSI messages for cnic/bnx2i driver.") Fixes: 1d9bf13 ("[SCSI] iscsi class: add iscsi host set param event") Fixes: 01cb225 ("[SCSI] iscsi: add target discvery event to transport class") Fixes: 264faaa ("[SCSI] iscsi: add transport end point callbacks") Fixes: fd7255f ("[SCSI] iscsi: add sysfs attrs for uspace sync up") Signed-off-by: Lin Ma <linma@zju.edu.cn> Link: https://lore.kernel.org/r/20230725024529.428311-1-linma@zju.edu.cn Reviewed-by: Chris Leech <cleech@redhat.com> Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
1 parent 89e637c commit 971dfcb

File tree

1 file changed

+43
-29
lines changed

1 file changed

+43
-29
lines changed

‎drivers/scsi/scsi_transport_iscsi.c‎

Lines changed: 43 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -3014,14 +3014,15 @@ iscsi_if_destroy_conn(struct iscsi_transport *transport, struct iscsi_uevent *ev
30143014
}
30153015

30163016
static int
3017-
iscsi_if_set_param(struct iscsi_transport *transport, struct iscsi_uevent *ev)
3017+
iscsi_if_set_param(struct iscsi_transport *transport, struct iscsi_uevent *ev, u32 rlen)
30183018
{
30193019
char *data = (char*)ev + sizeof(*ev);
30203020
struct iscsi_cls_conn *conn;
30213021
struct iscsi_cls_session *session;
30223022
int err = 0, value = 0, state;
30233023

3024-
if (ev->u.set_param.len > PAGE_SIZE)
3024+
if (ev->u.set_param.len > rlen ||
3025+
ev->u.set_param.len > PAGE_SIZE)
30253026
return -EINVAL;
30263027

30273028
session = iscsi_session_lookup(ev->u.set_param.sid);
@@ -3118,15 +3119,18 @@ static int iscsi_if_ep_disconnect(struct iscsi_transport *transport,
31183119

31193120
static int
31203121
iscsi_if_transport_ep(struct iscsi_transport *transport,
3121-
struct iscsi_uevent *ev, int msg_type)
3122+
struct iscsi_uevent *ev, int msg_type, u32 rlen)
31223123
{
31233124
struct iscsi_endpoint *ep;
31243125
int rc = 0;
31253126

31263127
switch (msg_type) {
31273128
case ISCSI_UEVENT_TRANSPORT_EP_CONNECT_THROUGH_HOST:
31283129
case ISCSI_UEVENT_TRANSPORT_EP_CONNECT:
3129-
rc = iscsi_if_ep_connect(transport, ev, msg_type);
3130+
if (rlen < sizeof(struct sockaddr))
3131+
rc = -EINVAL;
3132+
else
3133+
rc = iscsi_if_ep_connect(transport, ev, msg_type);
31303134
break;
31313135
case ISCSI_UEVENT_TRANSPORT_EP_POLL:
31323136
if (!transport->ep_poll)
@@ -3150,12 +3154,15 @@ iscsi_if_transport_ep(struct iscsi_transport *transport,
31503154

31513155
static int
31523156
iscsi_tgt_dscvr(struct iscsi_transport *transport,
3153-
struct iscsi_uevent *ev)
3157+
struct iscsi_uevent *ev, u32 rlen)
31543158
{
31553159
struct Scsi_Host *shost;
31563160
struct sockaddr *dst_addr;
31573161
int err;
31583162

3163+
if (rlen < sizeof(*dst_addr))
3164+
return -EINVAL;
3165+
31593166
if (!transport->tgt_dscvr)
31603167
return -EINVAL;
31613168

@@ -3176,7 +3183,7 @@ iscsi_tgt_dscvr(struct iscsi_transport *transport,
31763183

31773184
static int
31783185
iscsi_set_host_param(struct iscsi_transport *transport,
3179-
struct iscsi_uevent *ev)
3186+
struct iscsi_uevent *ev, u32 rlen)
31803187
{
31813188
char *data = (char*)ev + sizeof(*ev);
31823189
struct Scsi_Host *shost;
@@ -3185,7 +3192,8 @@ iscsi_set_host_param(struct iscsi_transport *transport,
31853192
if (!transport->set_host_param)
31863193
return -ENOSYS;
31873194

3188-
if (ev->u.set_host_param.len > PAGE_SIZE)
3195+
if (ev->u.set_host_param.len > rlen ||
3196+
ev->u.set_host_param.len > PAGE_SIZE)
31893197
return -EINVAL;
31903198

31913199
shost = scsi_host_lookup(ev->u.set_host_param.host_no);
@@ -3202,12 +3210,15 @@ iscsi_set_host_param(struct iscsi_transport *transport,
32023210
}
32033211

32043212
static int
3205-
iscsi_set_path(struct iscsi_transport *transport, struct iscsi_uevent *ev)
3213+
iscsi_set_path(struct iscsi_transport *transport, struct iscsi_uevent *ev, u32 rlen)
32063214
{
32073215
struct Scsi_Host *shost;
32083216
struct iscsi_path *params;
32093217
int err;
32103218

3219+
if (rlen < sizeof(*params))
3220+
return -EINVAL;
3221+
32113222
if (!transport->set_path)
32123223
return -ENOSYS;
32133224

@@ -3267,12 +3278,15 @@ iscsi_set_iface_params(struct iscsi_transport *transport,
32673278
}
32683279

32693280
static int
3270-
iscsi_send_ping(struct iscsi_transport *transport, struct iscsi_uevent *ev)
3281+
iscsi_send_ping(struct iscsi_transport *transport, struct iscsi_uevent *ev, u32 rlen)
32713282
{
32723283
struct Scsi_Host *shost;
32733284
struct sockaddr *dst_addr;
32743285
int err;
32753286

3287+
if (rlen < sizeof(*dst_addr))
3288+
return -EINVAL;
3289+
32763290
if (!transport->send_ping)
32773291
return -ENOSYS;
32783292

@@ -3770,13 +3784,12 @@ iscsi_get_host_stats(struct iscsi_transport *transport, struct nlmsghdr *nlh)
37703784
}
37713785

37723786
static int iscsi_if_transport_conn(struct iscsi_transport *transport,
3773-
struct nlmsghdr *nlh)
3787+
struct nlmsghdr *nlh, u32 pdu_len)
37743788
{
37753789
struct iscsi_uevent *ev = nlmsg_data(nlh);
37763790
struct iscsi_cls_session *session;
37773791
struct iscsi_cls_conn *conn = NULL;
37783792
struct iscsi_endpoint *ep;
3779-
uint32_t pdu_len;
37803793
int err = 0;
37813794

37823795
switch (nlh->nlmsg_type) {
@@ -3861,8 +3874,6 @@ static int iscsi_if_transport_conn(struct iscsi_transport *transport,
38613874

38623875
break;
38633876
case ISCSI_UEVENT_SEND_PDU:
3864-
pdu_len = nlh->nlmsg_len - sizeof(*nlh) - sizeof(*ev);
3865-
38663877
if ((ev->u.send_pdu.hdr_size > pdu_len) ||
38673878
(ev->u.send_pdu.data_size > (pdu_len - ev->u.send_pdu.hdr_size))) {
38683879
err = -EINVAL;
@@ -3892,6 +3903,7 @@ iscsi_if_recv_msg(struct sk_buff *skb, struct nlmsghdr *nlh, uint32_t *group)
38923903
struct iscsi_internal *priv;
38933904
struct iscsi_cls_session *session;
38943905
struct iscsi_endpoint *ep = NULL;
3906+
u32 rlen;
38953907

38963908
if (!netlink_capable(skb, CAP_SYS_ADMIN))
38973909
return -EPERM;
@@ -3911,6 +3923,13 @@ iscsi_if_recv_msg(struct sk_buff *skb, struct nlmsghdr *nlh, uint32_t *group)
39113923

39123924
portid = NETLINK_CB(skb).portid;
39133925

3926+
/*
3927+
* Even though the remaining payload may not be regarded as nlattr,
3928+
* (like address or something else), calculate the remaining length
3929+
* here to ease following length checks.
3930+
*/
3931+
rlen = nlmsg_attrlen(nlh, sizeof(*ev));
3932+
39143933
switch (nlh->nlmsg_type) {
39153934
case ISCSI_UEVENT_CREATE_SESSION:
39163935
err = iscsi_if_create_session(priv, ep, ev,
@@ -3967,15 +3986,15 @@ iscsi_if_recv_msg(struct sk_buff *skb, struct nlmsghdr *nlh, uint32_t *group)
39673986
err = -EINVAL;
39683987
break;
39693988
case ISCSI_UEVENT_SET_PARAM:
3970-
err = iscsi_if_set_param(transport, ev);
3989+
err = iscsi_if_set_param(transport, ev, rlen);
39713990
break;
39723991
case ISCSI_UEVENT_CREATE_CONN:
39733992
case ISCSI_UEVENT_DESTROY_CONN:
39743993
case ISCSI_UEVENT_STOP_CONN:
39753994
case ISCSI_UEVENT_START_CONN:
39763995
case ISCSI_UEVENT_BIND_CONN:
39773996
case ISCSI_UEVENT_SEND_PDU:
3978-
err = iscsi_if_transport_conn(transport, nlh);
3997+
err = iscsi_if_transport_conn(transport, nlh, rlen);
39793998
break;
39803999
case ISCSI_UEVENT_GET_STATS:
39814000
err = iscsi_if_get_stats(transport, nlh);
@@ -3984,23 +4003,22 @@ iscsi_if_recv_msg(struct sk_buff *skb, struct nlmsghdr *nlh, uint32_t *group)
39844003
case ISCSI_UEVENT_TRANSPORT_EP_POLL:
39854004
case ISCSI_UEVENT_TRANSPORT_EP_DISCONNECT:
39864005
case ISCSI_UEVENT_TRANSPORT_EP_CONNECT_THROUGH_HOST:
3987-
err = iscsi_if_transport_ep(transport, ev, nlh->nlmsg_type);
4006+
err = iscsi_if_transport_ep(transport, ev, nlh->nlmsg_type, rlen);
39884007
break;
39894008
case ISCSI_UEVENT_TGT_DSCVR:
3990-
err = iscsi_tgt_dscvr(transport, ev);
4009+
err = iscsi_tgt_dscvr(transport, ev, rlen);
39914010
break;
39924011
case ISCSI_UEVENT_SET_HOST_PARAM:
3993-
err = iscsi_set_host_param(transport, ev);
4012+
err = iscsi_set_host_param(transport, ev, rlen);
39944013
break;
39954014
case ISCSI_UEVENT_PATH_UPDATE:
3996-
err = iscsi_set_path(transport, ev);
4015+
err = iscsi_set_path(transport, ev, rlen);
39974016
break;
39984017
case ISCSI_UEVENT_SET_IFACE_PARAMS:
3999-
err = iscsi_set_iface_params(transport, ev,
4000-
nlmsg_attrlen(nlh, sizeof(*ev)));
4018+
err = iscsi_set_iface_params(transport, ev, rlen);
40014019
break;
40024020
case ISCSI_UEVENT_PING:
4003-
err = iscsi_send_ping(transport, ev);
4021+
err = iscsi_send_ping(transport, ev, rlen);
40044022
break;
40054023
case ISCSI_UEVENT_GET_CHAP:
40064024
err = iscsi_get_chap(transport, nlh);
@@ -4009,13 +4027,10 @@ iscsi_if_recv_msg(struct sk_buff *skb, struct nlmsghdr *nlh, uint32_t *group)
40094027
err = iscsi_delete_chap(transport, ev);
40104028
break;
40114029
case ISCSI_UEVENT_SET_FLASHNODE_PARAMS:
4012-
err = iscsi_set_flashnode_param(transport, ev,
4013-
nlmsg_attrlen(nlh,
4014-
sizeof(*ev)));
4030+
err = iscsi_set_flashnode_param(transport, ev, rlen);
40154031
break;
40164032
case ISCSI_UEVENT_NEW_FLASHNODE:
4017-
err = iscsi_new_flashnode(transport, ev,
4018-
nlmsg_attrlen(nlh, sizeof(*ev)));
4033+
err = iscsi_new_flashnode(transport, ev, rlen);
40194034
break;
40204035
case ISCSI_UEVENT_DEL_FLASHNODE:
40214036
err = iscsi_del_flashnode(transport, ev);
@@ -4030,8 +4045,7 @@ iscsi_if_recv_msg(struct sk_buff *skb, struct nlmsghdr *nlh, uint32_t *group)
40304045
err = iscsi_logout_flashnode_sid(transport, ev);
40314046
break;
40324047
case ISCSI_UEVENT_SET_CHAP:
4033-
err = iscsi_set_chap(transport, ev,
4034-
nlmsg_attrlen(nlh, sizeof(*ev)));
4048+
err = iscsi_set_chap(transport, ev, rlen);
40354049
break;
40364050
case ISCSI_UEVENT_GET_HOST_STATS:
40374051
err = iscsi_get_host_stats(transport, nlh);

0 commit comments

Comments
 (0)