Re: [PATCH] iscsi_tcp: bound max_r2t in iscsi_sw_tcp_conn_set_param()
Mike Christie
On 12/31/2011 04:01 PM, Xi Wang wrote:
> A large max_r2t could lead to integer overflow in subsequent call to
> iscsi_tcp_r2tpool_alloc(), allocating a smaller buffer than expected
> and leading to out-of-bounds write.
Are you actually hitting this and if so with what tools and target? And
when using greater than 1 do you see any throughput improvements?
> Signed-off-by: Xi Wang <xi....@gmail.com>
> Cc: sta...@vger.kernel.org
> ---
> drivers/scsi/iscsi_tcp.c | 4 ++--
> 1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/scsi/iscsi_tcp.c b/drivers/scsi/iscsi_tcp.c
> index 7c34d8e..9a1bf21 100644
> --- a/drivers/scsi/iscsi_tcp.c
> +++ b/drivers/scsi/iscsi_tcp.c
> @@ -687,7 +687,7 @@ static int iscsi_sw_tcp_conn_set_param(struct iscsi_cls_conn *cls_conn,
> struct iscsi_session *session = conn->session;
> struct iscsi_tcp_conn *tcp_conn = conn->dd_data;
> struct iscsi_sw_tcp_conn *tcp_sw_conn = tcp_conn->dd_data;
> - int value;
> + int value = 0;
>
> switch(param) {
> case ISCSI_PARAM_HDRDGST_EN:
> @@ -700,7 +700,7 @@ static int iscsi_sw_tcp_conn_set_param(struct iscsi_cls_conn *cls_conn,
> break;
> case ISCSI_PARAM_MAX_R2T:
> sscanf(buf, "%d", &value);
> - if (value <= 0 || !is_power_of_2(value))
> + if (value <= 0 || value > 65536 || !is_power_of_2(value))
> return -EINVAL;
> if (session->max_r2t == value)
> break;
What about the attached patch? It also fixes cxgb*i.
Mike Christie
> On Jan 2, 2012, at 9:18 AM, Xi Wang wrote:
>> On Jan 2, 2012, at 2:05 AM, Mike Christie wrote:
>>> What about the attached patch? It also fixes cxgb*i.
>>
>> Looks good to me. Thanks!
>
> BTW, should max_r2t be in the range 1..65535 according to RFC 3720?
Yeah.
> Using ushort seems to limit the upper bound to 32768, though probably
> no one wants to set it to such a large value.
I thought short was 32767 and ushort was 65535. Is a short like long
where it is different on different archs?
Xi Wang
iscsi_tcp_r2tpool_alloc(), allocating a smaller buffer than expected
and leading to out-of-bounds write.
Signed-off-by: Xi Wang <xi....@gmail.com>
--
1.7.5.4
Xi Wang
> when using greater than 1 do you see any throughput improvements?
No, I was worrying about security.
> What about the attached patch? It also fixes cxgb*i.
> <0001-libiscsi_tcp-fix-max_r2t-manipulation.patch>
Looks good to me. Thanks!
- xi
Xi Wang
> I thought short was 32767 and ushort was 65535. Is a short like long
> where it is different on different archs?
it is 65535, but the patch has an extra check is_power_of_2(), right?
- xi
Xi Wang
BTW, should max_r2t be in the range 1..65535 according to RFC 3720?
Using ushort seems to limit the upper bound to 32768, though probably
no one wants to set it to such a large value.
- xi
Ulrich Windl
May I ask the related question, _why_ is a power of two needed for value (R2T)?
Regards,
Ulrich
>>> Xi Wang <xi....@gmail.com> schrieb am 31.12.2011 um 23:01 in Nachricht
<1325368867-13696-1-g...@gmail.com>:
Mike Christie
> Hi!
>
> May I ask the related question, _why_ is a power of two needed for value (R2T)?
>
The iscsi code uses this kernel data struct called a kfifo which
requires the size to be a power of 2. And actually as I look at it now
to double check, the comments for kfifo say you can pass it whatever
size you want and it will now be rounded up for you.
Mike Christie
I meant rounded down.
Greg KH
What ever happened to this patch, is it in Linus's tree yet?
thanks,
greg k-h
Xi Wang
> What ever happened to this patch, is it in Linus's tree yet?
I cannot find the patch showing up there, though it was reposted at
http://www.spinics.net/lists/linux-scsi/msg56957.html
- xi