Re: potential-null-pointer-dereference-in-amiga-serial-driver.patch added to -mm tree

Re: potential-null-pointer-dereference-in-amiga-serial-driver.patch added to -mm tree

[Marcelo Tosatti]

Hi Alexey,

On Tue, May 31, 2005 at 07:49:15PM +0400, Alexey Dobriyan wrote:
> On Tuesday 31 May 2005 13:08, akpm@osdl.org wrote:
> > A pointer is dereferenced before it is null-checked.
> 
> > --- 25/drivers/char/amiserial.c~potential-null-pointer-dereference-in-amiga-serial-driver
> > +++ 25-akpm/drivers/char/amiserial.c
> 
> >  static void rs_put_char(struct tty_struct *tty, unsigned char ch)
> >  {
> > -	struct async_struct *info = (struct async_struct *)tty->driver_data;
> > +	struct async_struct *info;
> >  	unsigned long flags;
> >  
> > +	if (!tty)
> > +		return;
> 
> Can ->put_char be ever called with tty being NULL? From my reading of
> drivers/char/n_tty.c it can't.

Nope it can't, but the change makes the code more readable IMO, while handling
a NULL "tty" argument properly (which the old version pretends to, but doesnt).

> Every single time ->put_char is used a-la
> 
> 	tty->driver->put_char(tty, '\r');
> 
> So, tty will be dereferenced before function call. Same for static inline
> put_char() there.  

Re: potential-null-pointer-dereference-in-amiga-serial-driver.patch added to -mm tree

[Alexey Dobriyan]

On Tuesday 31 May 2005 13:08, akpm@osdl.org wrote:
> A pointer is dereferenced before it is null-checked.

> --- 25/drivers/char/amiserial.c~potential-null-pointer-dereference-in-amiga-serial-driver
> +++ 25-akpm/drivers/char/amiserial.c

>  static void rs_put_char(struct tty_struct *tty, unsigned char ch)
>  {
> -	struct async_struct *info = (struct async_struct *)tty->driver_data;
> +	struct async_struct *info;
>  	unsigned long flags;
>  
> +	if (!tty)
> +		return;

Can ->put_char be ever called with tty being NULL? From my reading of
drivers/char/n_tty.c it can't.

Every single time ->put_char is used a-la

	tty->driver->put_char(tty, '\r');

So, tty will be dereferenced before function call. Same for static inline
put_char() there.

> +
> +	info = tty->driver_data;

>  static int rs_write(struct tty_struct * tty, const unsigned char *buf, int count)
>  {

> -	struct async_struct *info = (struct async_struct *)tty->driver_data;
> +	struct async_struct *info;
>  	unsigned long flags;
>  
> +	if (!tty)
> +		return 0;

Same question.

> +
> +	info = tty->driver_data;

Re: potential-null-pointer-dereference-in-amiga-serial-driver.patch added to -mm tree

[Greg KH]

On Tue, May 31, 2005 at 09:22:15AM -0300, Marcelo Tosatti wrote:
> 
> Hi Alexey,
> 
> On Tue, May 31, 2005 at 07:49:15PM +0400, Alexey Dobriyan wrote:
> > On Tuesday 31 May 2005 13:08, akpm@osdl.org wrote:
> > > A pointer is dereferenced before it is null-checked.
> > 
> > > --- 25/drivers/char/amiserial.c~potential-null-pointer-dereference-in-amiga-serial-driver
> > > +++ 25-akpm/drivers/char/amiserial.c
> > 
> > >  static void rs_put_char(struct tty_struct *tty, unsigned char ch)
> > >  {
> > > -	struct async_struct *info = (struct async_struct *)tty->driver_data;
> > > +	struct async_struct *info;
> > >  	unsigned long flags;
> > >  
> > > +	if (!tty)
> > > +		return;
> > 
> > Can ->put_char be ever called with tty being NULL? From my reading of
> > drivers/char/n_tty.c it can't.
> 
> Nope it can't, but the change makes the code more readable IMO, while handling
> a NULL "tty" argument properly (which the old version pretends to, but doesnt).

But unneeded checks like this are not encouraged in the kernel.  As the
tty pointer can never be null, don't worry about it.

thanks,

greg k-h

Re: potential-null-pointer-dereference-in-amiga-serial-driver.patch added to -mm tree

[Marcelo Tosatti]

On Wed, Jun 01, 2005 at 10:21:08PM -0700, Greg KH wrote:
> On Tue, May 31, 2005 at 09:22:15AM -0300, Marcelo Tosatti wrote:
> > 
> > Hi Alexey,
> > 
> > On Tue, May 31, 2005 at 07:49:15PM +0400, Alexey Dobriyan wrote:
> > > On Tuesday 31 May 2005 13:08, akpm@osdl.org wrote:
> > > > A pointer is dereferenced before it is null-checked.
> > > 
> > > > --- 25/drivers/char/amiserial.c~potential-null-pointer-dereference-in-amiga-serial-driver
> > > > +++ 25-akpm/drivers/char/amiserial.c
> > > 
> > > >  static void rs_put_char(struct tty_struct *tty, unsigned char ch)
> > > >  {
> > > > -	struct async_struct *info = (struct async_struct *)tty->driver_data;
> > > > +	struct async_struct *info;
> > > >  	unsigned long flags;
> > > >  
> > > > +	if (!tty)
> > > > +		return;
> > > 
> > > Can ->put_char be ever called with tty being NULL? From my reading of
> > > drivers/char/n_tty.c it can't.
> > 
> > Nope it can't, but the change makes the code more readable IMO, while handling
> > a NULL "tty" argument properly (which the old version pretends to, but doesnt).
> 
> But unneeded checks like this are not encouraged in the kernel.  As the
> tty pointer can never be null, don't worry about it.

OK - so better just remove the check. Julien, care to follow Greg's 
recommendation and refresh the patch? 

Re: potential-null-pointer-dereference-in-amiga-serial-driver.patch added to -mm tree

[Julien TINNES]

[-- Attachment #1: Type: text/plain, Size: 146 bytes --]


> OK - so better just remove the check. Julien, care to follow Greg's 
> recommendation and refresh the patch? 

Here it is.


-- 
Julien TINNES

[-- Attachment #2: 2.6-amiserial-nocheck.patch --]
[-- Type: text/x-patch, Size: 560 bytes --]

--- linux-2.6.11.orig/drivers/char/amiserial.c	2005-05-17 10:55:03.000000000 +0200
+++ linux-2.6.11/drivers/char/amiserial.c	2005-06-09 11:29:04.000000000 +0200
@@ -867,7 +867,7 @@
 	if (serial_paranoia_check(info, tty->name, "rs_put_char"))
 		return;
 
-	if (!tty || !info->xmit.buf)
+	if (!info->xmit.buf)
 		return;
 
 	local_irq_save(flags);
@@ -916,7 +916,7 @@
 	if (serial_paranoia_check(info, tty->name, "rs_write"))
 		return 0;
 
-	if (!tty || !info->xmit.buf || !tmp_buf)
+	if (!info->xmit.buf || !tmp_buf)
 		return 0;
 
 	local_save_flags(flags);