From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754981AbbB0QHs (ORCPT ); Fri, 27 Feb 2015 11:07:48 -0500 Received: from mail-wg0-f41.google.com ([74.125.82.41]:45428 "EHLO mail-wg0-f41.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751776AbbB0QHq (ORCPT ); Fri, 27 Feb 2015 11:07:46 -0500 Message-ID: <54F0964F.6060503@suse.cz> Date: Fri, 27 Feb 2015 17:07:43 +0100 From: Jiri Slaby User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.5.0 MIME-Version: 1.0 To: Dan Carpenter CC: Greg Kroah-Hartman , linux-kernel@vger.kernel.org Subject: Re: potential corruption in synclink driver References: <20141209085205.GA28655@mwanda> In-Reply-To: <20141209085205.GA28655@mwanda> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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