potential corruption in synclink driver

potential corruption in synclink driver

[Dan Carpenter]

Hi Jiri, I hate to bother you with this, but you're the TTY expert.  I'm
getting the following static checker warning:

	drivers/tty/synclink.c:4057 save_tx_buffer_request()
	error: 'BufferSize' from user is not capped properly

drivers/tty/synclink.c
  4047  static int save_tx_buffer_request(struct mgsl_struct *info,const char *Buffer, unsigned int BufferSize)
  4048  {
  4049          struct tx_holding_buffer *ptx;
  4050  
  4051          if ( info->tx_holding_count >= info->num_tx_holding_buffers ) {
  4052                  return 0;               /* all buffers in use */
  4053          }
  4054  
  4055          ptx = &info->tx_holding_buffers[info->put_tx_holding_index];
  4056          ptx->buffer_size = BufferSize;
  4057          memcpy( ptx->buffer, Buffer, BufferSize);
                                             ^^^^^^^^^^
  4058  
  4059          ++info->tx_holding_count;
  4060          if ( ++info->put_tx_holding_index >= info->num_tx_holding_buffers)
  4061                  info->put_tx_holding_index=0;
  4062  
  4063          return 1;
  4064  }

ptx->buffer is allocated in mgsl_alloc_intermediate_txbuffer_memory()
and it can be up to "info->max_frame_size" bytes which is a number
between 4096 and 65535.

The way I read it, BufferSize comes from do_tty_write() and it could be
up to 65536.  That's obviously one higher than 65535.  But if
->max_frame_size is 4096 then that's a lot higher.

This looks like a potential buffer overflow but I don't know the TTY
layer enough to be sure.

regards,
dan carpenter

Re: potential corruption in synclink driver

[Jiri Slaby]

On 12/09/2014, 09:52 AM, Dan Carpenter wrote:
> Hi Jiri, I hate to bother you with this, but you're the TTY expert.  I'm
> getting the following static checker warning:
> 
> 	drivers/tty/synclink.c:4057 save_tx_buffer_request()
> 	error: 'BufferSize' from user is not capped properly
> 
> drivers/tty/synclink.c
>   4047  static int save_tx_buffer_request(struct mgsl_struct *info,const char *Buffer, unsigned int BufferSize)
>   4048  {
>   4049          struct tx_holding_buffer *ptx;
>   4050  
>   4051          if ( info->tx_holding_count >= info->num_tx_holding_buffers ) {
>   4052                  return 0;               /* all buffers in use */
>   4053          }
>   4054  
>   4055          ptx = &info->tx_holding_buffers[info->put_tx_holding_index];
>   4056          ptx->buffer_size = BufferSize;
>   4057          memcpy( ptx->buffer, Buffer, BufferSize);
>                                              ^^^^^^^^^^
>   4058  
>   4059          ++info->tx_holding_count;
>   4060          if ( ++info->put_tx_holding_index >= info->num_tx_holding_buffers)
>   4061                  info->put_tx_holding_index=0;
>   4062  
>   4063          return 1;
>   4064  }
> 
> ptx->buffer is allocated in mgsl_alloc_intermediate_txbuffer_memory()
> and it can be up to "info->max_frame_size" bytes which is a number
> between 4096 and 65535.
> 
> The way I read it, BufferSize comes from do_tty_write() and it could be
> up to 65536.  That's obviously one higher than 65535.  But if
> ->max_frame_size is 4096 then that's a lot higher.
> 
> This looks like a potential buffer overflow but I don't know the TTY
> layer enough to be sure.

Yes and no :). In theory, the only line discipline which can handle
large chunks is hdlc. That one limits the count to 4096 by default. But
you are right, if someone sets maxframe in hdlc to the maximum value
(65535) and won't set maxframe of synclinc, they will have a buffer
overflow.

These two guys are so old and ugly, maybe unused, so that I don't even
know if they are worth fixing. In the normal case (no maxframe override
on commandline/module param), they should be safe.

Maybe we can drop the support to override by parameters...

thanks,
-- 
js
suse labs

Re: potential corruption in synclink driver

[One Thousand Gnomes]

> Yes and no :). In theory, the only line discipline which can handle
> large chunks is hdlc. That one limits the count to 4096 by default. But
> you are right, if someone sets maxframe in hdlc to the maximum value
> (65535) and won't set maxframe of synclinc, they will have a buffer
> overflow.
> 
> These two guys are so old and ugly, maybe unused, so that I don't even

They are still made, which puts them ahead of about 3/4 of the less
maintained crap under drivers/

> know if they are worth fixing. In the normal case (no maxframe override
> on commandline/module param), they should be safe.
> 
> Maybe we can drop the support to override by parameters...

Why do we care ? You have to be root to override the parameters anyway. If
you can set those values you can already compromise the box anyway you
feel fit. The problem is "if I am sufficiently expert to know about
obscure config options, have the privilege to set them and screw up
totally while using those privileges"

The options are important for certain special case uses where you have >
4K frame sizes. A check on oversize frames might not be a bad idea but I
don't think dropping it makes sense.

Alan