lkml.org 
[lkml]   [2008]   [Mar]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] [SCSI] gdth: Allocate sense_buffer to prevent NULL pointer dereference
On Tue, 11 Mar 2008, Boaz Harrosh wrote:

> > You can first get to the scsi_device in isd200_ata_command().
>
> I was afraid of that. I don't think I want to call scsi_get_command
> from within .queuecommand. I will leave the code hacked as today.

What are you talking about? isd200_ata_command isn't called by
queuecommand.

> > The last
> > place you can get to the scsi_device (if one exists!) is
> > quiesce_and_remove_host() -- but that's part of the core, not specific
> > to isd200.
> >
>
> Here two, it looks like I need to introduce a new function pointer for isd200

Why? And why do you need to get to the scsi_device in the first place?

> I'll leave it for now. Though I know this is not the last I'll see of this driver.
>
> >> (And one more stupid question. Why does isd200_init_info allocates the info
> >> structure but the isd200_free_info_ptrs does not free it, it kind of
> >> limits the way it can be allocated, no?)
> >
> > Not at all. isd200_free_info_ptrs() frees everything pointed to by the
> > info structure, and the info structure itself is freed later on by the
> > usb-storage core in usb_stor_release_resources().
> >
>
> OK so in isd200_get_inquiry_data() at the end near the call to:
> us->extra_destructor(info);
> us->extra = NULL;
>
> It leaks the info.

Yes. The three lines of code there are unnecessary. You should remove
them (and the comment) instead of adding more somewhere else. Or if
you want to keep them, just add a line to kfree(us->extra) before
us->extra is set to NULL.

> Please ACK the first patch sent, so James can put it in scsi-rc-fixes as part
> of the sense_buffer effort for 2.6.25-rc

The first patch is okay except for style violations:

> @@ -294,6 +294,7 @@ struct isd200_info {
> unsigned char MaxLUNs;
> struct scsi_cmnd srb;
> struct scatterlist sg;
> + u8* sense_buffer;

This should be

u8 *sense_buffer;

And

> isd200_free_info_ptrs(info);
> kfree(info);
> retStatus = ISD200_ERROR;
> }
> + else
> + info->srb.sense_buffer = info->sense_buffer;

The "else" should go on the same line as the closing brace, and it
should have its own opening brace.

Alan Stern



\
 
 \ /
  Last update: 2008-03-11 20:21    [W:0.078 / U:0.156 seconds]
©2003-2014 Jasper Spaans. Advertise on this site