On Thu, Nov 29, 2012 at 12:40:49PM +0800, Chen Gang wrote:
> Hello Greg Kroah-Hartman:
>
> for MAX_ASYNC_BUFFER_SIZE:
> it is defined as 4096;
> but for the max buffer size which it processes, is 65535.
> so suggest to #define MAX_ASYNC_BUFFER_SIZE 0x10000 (better than 0xffff)
Please, send tty questions to the [email protected] list
also.
And, I really don't understand here, why do you want to change this?
What is it going to change? And why?
greg k-h
于 2012年11月29日 13:13, Greg KH 写道:
> On Thu, Nov 29, 2012 at 12:40:49PM +0800, Chen Gang wrote:
>> Hello Greg Kroah-Hartman:
>>
>> for MAX_ASYNC_BUFFER_SIZE:
>> it is defined as 4096;
>> but for the max buffer size which it processes, is 65535.
>> so suggest to #define MAX_ASYNC_BUFFER_SIZE 0x10000 (better than 0xffff)
>
> Please, send tty questions to the [email protected] list
> also.
I cc to [email protected], in this reply.
I referenced the file MAINTAINERS, before sent original mail:
it seems all drivers/tty/serial/* are relative with
[email protected]. but our case is not relative
drivers/tty/serial. so for not bother them, I am not send to them,
originally.
in MAINTAINERS, line 7438, I find for common of driver/tty/*, can send
to you and no cc, so I send you and cc to [email protected].
next time, for all tty questions, I will cc to
[email protected] (not cc to [email protected]).
>
> And, I really don't understand here, why do you want to change this?
> What is it going to change? And why?
>
Why:
for the context MGSLPC_INFO *info in drivers/char/pcmcia/synclink_cs.c
info->max_frame_size can be the value between 4096 .. 65535 (can be
set by its module input parameter)
info->flag_buf length is 4096 (MAX_ASYNC_BUFFER_SIZE)
in function rx_get_frame
the framesize is limit by info->max_frame_size, but may still be
larger that 4096.
when call function ldisc_receive_buf, info->flag_buf is equal to
4096, but framesize can be more than 4096. it will cause memory over flow.
What:
#define MAX_ASYNC_BUFFER_SIZE 0x10000 (instead of 4096, originally).
let it match the max frame size.
At last:
my suggestion may be incorrect, need relative member (who expert about
it) to help checking.
welcome another suggestion or completions.
thanks.
gchen.
> greg k-h
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
>
--
Chen Gang
Asianux Corporation
Signed-off-by: Joe Perches <[email protected]>
---
MAINTAINERS | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/MAINTAINERS b/MAINTAINERS
index afc0b27..255dafb 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7658,6 +7658,7 @@ K: ^Subject:.*(?i)trivial
TTY LAYER
M: Greg Kroah-Hartman <[email protected]>
M: Jiri Slaby <[email protected]>
+L: [email protected]
S: Supported
T: git git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git
F: drivers/tty/
于 2012年11月29日 14:14, Joe Perches 写道:
> Signed-off-by: Joe Perches <[email protected]>
> ---
> MAINTAINERS | 1 +
> 1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index afc0b27..255dafb 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -7658,6 +7658,7 @@ K: ^Subject:.*(?i)trivial
> TTY LAYER
> M: Greg Kroah-Hartman <[email protected]>
> M: Jiri Slaby <[email protected]>
> +L: [email protected]
> S: Supported
> T: git git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git
> F: drivers/tty/
>
>
>
>
thank you very much.
--
Chen Gang
Asianux Corporation
On 11/29/2012 07:14 AM, Joe Perches wrote:
> Signed-off-by: Joe Perches <[email protected]>
> ---
> MAINTAINERS | 1 +
> 1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index afc0b27..255dafb 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -7658,6 +7658,7 @@ K: ^Subject:.*(?i)trivial
> TTY LAYER
> M: Greg Kroah-Hartman <[email protected]>
> M: Jiri Slaby <[email protected]>
> +L: [email protected]
This might have the effect that people will try to reach us at that
list. But this is not true at least for me. serial != tty.
thanks,
--
js
suse labs
On Thu, Nov 29, 2012 at 01:57:59PM +0800, Chen Gang wrote:
> > And, I really don't understand here, why do you want to change this?
> > What is it going to change? And why?
> >
>
> Why:
> for the context MGSLPC_INFO *info in drivers/char/pcmcia/synclink_cs.c
> info->max_frame_size can be the value between 4096 .. 65535 (can be
> set by its module input parameter)
> info->flag_buf length is 4096 (MAX_ASYNC_BUFFER_SIZE)
> in function rx_get_frame
> the framesize is limit by info->max_frame_size, but may still be
> larger that 4096.
> when call function ldisc_receive_buf, info->flag_buf is equal to
> 4096, but framesize can be more than 4096. it will cause memory over flow.
Do you use that pcmcia driver for anything? Are those cards still
around?
> What:
> #define MAX_ASYNC_BUFFER_SIZE 0x10000 (instead of 4096, originally).
> let it match the max frame size.
>
> At last:
> my suggestion may be incorrect, need relative member (who expert about
> it) to help checking.
That driver might be incorrect, yes, care to make up a patch for it and
test it to verify it fixes the problem?
thanks,
greg k-h
于 2012年11月30日 02:32, Greg KH 写道:
> On Thu, Nov 29, 2012 at 01:57:59PM +0800, Chen Gang wrote:
>>> And, I really don't understand here, why do you want to change this?
>>> What is it going to change? And why?
>>>
>>
>> Why:
>> for the context MGSLPC_INFO *info in drivers/char/pcmcia/synclink_cs.c
>> info->max_frame_size can be the value between 4096 .. 65535 (can be
>> set by its module input parameter)
>> info->flag_buf length is 4096 (MAX_ASYNC_BUFFER_SIZE)
>> in function rx_get_frame
>> the framesize is limit by info->max_frame_size, but may still be
>> larger that 4096.
>> when call function ldisc_receive_buf, info->flag_buf is equal to
>> 4096, but framesize can be more than 4096. it will cause memory over flow.
>
> Do you use that pcmcia driver for anything? Are those cards still
> around?
I am not use them.
I am just through code review (so it is only a suggestion).
this issue has effect with 4 synclink drivers
I checked their source code, all of them have the same issue.
drivers/char/pcmcia/synclink_cs.c:213: char flag_buf[MAX_ASYNC_BUFFER_SIZE];
drivers/tty/synclink_gt.c:320: char flag_buf[MAX_ASYNC_BUFFER_SIZE];
drivers/tty/synclink.c:294: char flag_buf[MAX_ASYNC_BUFFER_SIZE];
drivers/tty/synclinkmp.c:265: char flag_buf[MAX_ASYNC_BUFFER_SIZE];
by the way, for the char_buf, has already useless (can be removed)
drivers/tty/synclink_gt.c:321: char char_buf[MAX_ASYNC_BUFFER_SIZE];
drivers/tty/synclink.c:295: char char_buf[MAX_ASYNC_BUFFER_SIZE];
drivers/tty/synclinkmp.c:266: char char_buf[MAX_ASYNC_BUFFER_SIZE];
>
>> What:
>> #define MAX_ASYNC_BUFFER_SIZE 0x10000 (instead of 4096, originally).
>> let it match the max frame size.
>>
>> At last:
>> my suggestion may be incorrect, need relative member (who expert about
>> it) to help checking.
>
> That driver might be incorrect, yes, care to make up a patch for it and
> test it to verify it fixes the problem?
>
and now Alan Cox has his own opinions
at least, I think it is valuable to continue discussing about it.
if Alan Cox agree with it (but it seems not), I will make patch, and try to perform test.
also welcome another members to help testing.
> thanks,
>
> greg k-h
>
>
--
Chen Gang
Asianux Corporation
On 11/29/2012 8:52 PM, Chen Gang wrote:
> 于 2012年11月30日 02:32, Greg KH 写道:
>> On Thu, Nov 29, 2012 at 01:57:59PM +0800, Chen Gang wrote:
>>>> And, I really don't understand here, why do you want to change this?
>>>> What is it going to change? And why?
>>>
>>> Why:
>>> for the context MGSLPC_INFO *info in drivers/char/pcmcia/synclink_cs.c
>>> info->max_frame_size can be the value between 4096 .. 65535 (can be
>>> set by its module input parameter)
>>> info->flag_buf length is 4096 (MAX_ASYNC_BUFFER_SIZE)
>>> in function rx_get_frame
>>> the framesize is limit by info->max_frame_size, but may still be
>>> larger that 4096.
>>> when call function ldisc_receive_buf, info->flag_buf is equal to
>>> 4096, but framesize can be more than 4096. it will cause memory over flow.
The confusion centers on calling the line discipline receive_buf
function with a data buffer larger than the flag buffer.
The synclink drivers support asynchronous and synchronous (HDLC)
serial communications.
In asynchronous mode, the tty flip buffer is used to feed
data to the line discipline. In this mode, the above argument
does not apply. The receive_buf function is not called directly.
In synchronous mode, the driver calls the line discipline
receive_buf function directly to feed one HDLC frame
of data per call. Maintaining frame boundaries is needed
in this mode. This is done only with the N_HDLC line
discipline which expects this format and ignores the flag buffer.
The flag buffer passed is just a place holder to meet the
calling conventions of the line discipline receive_buf function.
The only danger is if:
1. driver is configured for synchronous mode
2. driver is configured for frames > 4K
3. line discipline other than N_HDLC is selected
In this case the line discipline might try to access
beyond the end of the flag buffer. This is a non-functional
configuration that would not occur on purpose.
Increasing the flag buffer size would prevent a problem
in this degenerate case of purposeful misconfiguration.
This would be at the expense of larger allocations that are
not used.
I think the correct fix is for me to change the direct
calls to pass the same buffer for both data and flag and
add a comment describing the fact the flag buffer is ignored
when using N_HDLC. That way a misconfigured setup won't
cause problems and no unneeded allocations are made.
My suggestion is to leave it as is for now until I can make
those changes. I admit the current code is ugly enough to
cause confusion (sorry Chen Gang), but I don't see any immediate danger.
--
Paul Fulghum
MicroGate Systems, Ltd.
=Customer Driven, by Design=
(800)444-1982 (US Sales)
(512)345-7791 x102 (Direct)
(512)343-9046 (Fax)
Central Time Zone (GMT -6h)
http://www.microgate.com
Fix call to line discipline receive_buf by synclink drivers.
Dummy flag buffer argument is ignored by N_HDLC line discipline but might
be of insufficient size if accessed by a different line discipline
selected by mistake. Calls are changed to use data buffer argument for
both data and flag buffer so valid memory is provided if the wrong
line discipline is used. Unused char_buf and flag_buf are removed.
Signed-off-by: Paul Fulghum <[email protected]>
--- a/drivers/char/pcmcia/synclink_cs.c 2012-11-26 14:15:45.000000000 -0600
+++ b/drivers/char/pcmcia/synclink_cs.c 2012-11-30 12:50:23.000000000 -0600
@@ -210,7 +210,6 @@ typedef struct _mgslpc_info {
char testing_irq;
unsigned int init_error; /* startup error (DIAGS) */
- char flag_buf[MAX_ASYNC_BUFFER_SIZE];
bool drop_rts_on_tx_done;
struct _input_signal_events input_signal_events;
@@ -3707,7 +3706,16 @@ static bool rx_get_frame(MGSLPC_INFO *in
hdlcdev_rx(info, buf->data, framesize);
else
#endif
- ldisc_receive_buf(tty, buf->data, info->flag_buf, framesize);
+ {
+ /*
+ * Call N_HDLC line discipline directly to maintain
+ * frame boundaries. Reuse the data buffer argument for the
+ * flag buffer argument. The flag buffer is ignored by N_HDLC.
+ * If a different line discipline is selected by mistake it
+ * will have valid memory for both arguments.
+ */
+ ldisc_receive_buf(tty, buf->data, buf->data, framesize);
+ }
}
}
--- a/drivers/tty/synclink.c 2012-11-26 14:15:45.000000000 -0600
+++ b/drivers/tty/synclink.c 2012-11-30 12:59:29.000000000 -0600
@@ -291,8 +291,6 @@ struct mgsl_struct {
bool lcr_mem_requested;
u32 misc_ctrl_value;
- char flag_buf[MAX_ASYNC_BUFFER_SIZE];
- char char_buf[MAX_ASYNC_BUFFER_SIZE];
bool drop_rts_on_tx_done;
bool loopmode_insert_requested;
@@ -6661,7 +6659,17 @@ static bool mgsl_get_rx_frame(struct mgs
hdlcdev_rx(info,info->intermediate_rxbuffer,framesize);
else
#endif
- ldisc_receive_buf(tty, info->intermediate_rxbuffer, info->flag_buf, framesize);
+ {
+ /*
+ * Call N_HDLC line discipline directly to maintain
+ * frame boundaries. Reuse the data buffer argument for the
+ * flag buffer argument. The flag buffer is ignored by N_HDLC.
+ * If a different line discipline is selected by mistake it
+ * will have valid memory for both arguments.
+ */
+ ldisc_receive_buf(tty, info->intermediate_rxbuffer,
+ info->intermediate_rxbuffer, framesize);
+ }
}
}
/* Free the buffers used by this frame. */
@@ -6833,7 +6841,15 @@ static bool mgsl_get_raw_rx_frame(struct
memcpy( info->intermediate_rxbuffer, pBufEntry->virt_addr, framesize);
info->icount.rxok++;
- ldisc_receive_buf(tty, info->intermediate_rxbuffer, info->flag_buf, framesize);
+ /*
+ * Call N_HDLC line discipline directly to maintain
+ * block boundaries. Reuse the data buffer argument for the
+ * flag buffer argument. The flag buffer is ignored by N_HDLC.
+ * If a different line discipline is selected by mistake it
+ * will have valid memory for both arguments.
+ */
+ ldisc_receive_buf(tty, info->intermediate_rxbuffer,
+ info->intermediate_rxbuffer, framesize);
}
/* Free the buffers used by this frame. */
--- a/drivers/tty/synclinkmp.c 2012-11-26 14:15:45.000000000 -0600
+++ b/drivers/tty/synclinkmp.c 2012-11-30 13:01:36.000000000 -0600
@@ -262,8 +262,6 @@ typedef struct _synclinkmp_info {
bool sca_statctrl_requested;
u32 misc_ctrl_value;
- char flag_buf[MAX_ASYNC_BUFFER_SIZE];
- char char_buf[MAX_ASYNC_BUFFER_SIZE];
bool drop_rts_on_tx_done;
struct _input_signal_events input_signal_events;
@@ -4979,8 +4977,17 @@ CheckAgain:
hdlcdev_rx(info,info->tmp_rx_buf,framesize);
else
#endif
- ldisc_receive_buf(tty,info->tmp_rx_buf,
- info->flag_buf, framesize);
+ {
+ /*
+ * Call N_HDLC line discipline directly to maintain
+ * frame boundaries. Reuse the data buffer argument for the
+ * flag buffer argument. The flag buffer is ignored by N_HDLC.
+ * If a different line discipline is selected by mistake it
+ * will have valid memory for both arguments.
+ */
+ ldisc_receive_buf(tty, info->tmp_rx_buf,
+ info->tmp_rx_buf, framesize);
+ }
}
}
/* Free the buffers used by this frame. */
--- a/drivers/tty/synclink_gt.c 2012-11-26 14:15:45.000000000 -0600
+++ b/drivers/tty/synclink_gt.c 2012-11-30 12:53:25.000000000 -0600
@@ -317,8 +317,6 @@ struct slgt_info {
unsigned char *tx_buf;
int tx_count;
- char flag_buf[MAX_ASYNC_BUFFER_SIZE];
- char char_buf[MAX_ASYNC_BUFFER_SIZE];
bool drop_rts_on_tx_done;
struct _input_signal_events input_signal_events;
@@ -4760,7 +4758,16 @@ check_again:
hdlcdev_rx(info,info->tmp_rbuf, framesize);
else
#endif
- ldisc_receive_buf(tty, info->tmp_rbuf, info->flag_buf, framesize);
+ {
+ /*
+ * Call N_HDLC line discipline directly to maintain
+ * frame boundaries. Reuse the data buffer argument for the
+ * flag buffer argument. The flag buffer is ignored by N_HDLC.
+ * If a different line discipline is selected by mistake it
+ * will have valid memory for both arguments.
+ */
+ ldisc_receive_buf(tty, info->tmp_rbuf, info->tmp_rbuf, framesize);
+ }
}
}
free_rbufs(info, start, end);
@@ -4793,9 +4800,17 @@ static bool rx_get_buf(struct slgt_info
}
DBGDATA(info, info->rbufs[i].buf, count, "rx");
DBGINFO(("rx_get_buf size=%d\n", count));
- if (count)
+ if (count) {
+ /*
+ * Call N_HDLC line discipline directly to maintain
+ * block boundaries. Reuse the data buffer argument for the
+ * flag buffer argument. The flag buffer is ignored by N_HDLC.
+ * If a different line discipline is selected by mistake it
+ * will have valid memory for both arguments.
+ */
ldisc_receive_buf(info->port.tty, info->rbufs[i].buf,
- info->flag_buf, count);
+ info->rbufs[i].buf, count);
+ }
free_rbufs(info, i, i);
return true;
}