On Fri, Nov 12, 2010 at 2:47 AM, Joe Eykholt <jeykholt@xxxxxxxxx> wrote:
>
>
> On 11/11/10 5:45 AM, Hillf Danton wrote:
>> On Wed, Nov 10, 2010 at 2:15 AM, Joe Eykholt <jeykholt@xxxxxxxxx> wrote:
>>> On 11/9/10 6:01 AM, Hillf Danton wrote:
>>>> Ring buffers are setup for exchanging data between K and U spaces, but
>>>> they could not survive multiple open operations.
>>>>
>>>> The registered misc interface is monitored and prevented from multiple
>>>> opens for fixing the vulnerability.
>>>>
>>>> A typo, -BUSY, is also cleaned up.
>>>>
>>>> btw, the ring buffers could be setup in a per file manner?
>>>>
>>>> Signed-off-by: Hillf Danton <dhillf@xxxxxxxxx>
>>>> ---
>>>>
>>>> --- a/drivers/scsi/scsi_tgt_if.c   Â2010-09-13 07:07:38.000000000 +0800
>>>> +++ b/drivers/scsi/scsi_tgt_if.c   Â2010-11-09 21:42:48.000000000 +0800
>>>> @@ -85,7 +85,7 @@ static int tgt_uspace_send_event(u32 typ
>>>> Â Â Â if (!ev->hdr.status)
>>>> Â Â Â Â Â Â Â tgt_ring_idx_inc(ring);
>>>> Â Â Â else
>>>> - Â Â Â Â Â Â err = -BUSY;
>>>> + Â Â Â Â Â Â err = -EBUSY;
>>>>
>>>> Â Â Â spin_unlock_irqrestore(&ring->tr_lock, flags);
>>>>
>>>> @@ -319,20 +319,33 @@ static int tgt_mmap(struct file *filp, s
>>>> Â Â Â return err;
>>>> Â}
>>>>
>>>> +static unsigned long tgt_open_cnt = 0;
>>>> +
>>>> Âstatic int tgt_open(struct inode *inode, struct file *file)
>>>> Â{
>>>> + Â Â if (tgt_open_cnt)
>>>> + Â Â Â Â Â Â return -EBUSY;
>>>> + Â Â tgt_open_cnt++;
>>>
>>> Since there's no locking, there's still a tiny hole where
>>> simultaneous opens could succeed. ÂConsider using an atomic.
>>> Good find and good fix otherwise.
>>>
>> Would you please, Joe, show the atomic version?
>> thanks//Hillf
>
> I take it back. ÂThere's no good atomic version.
> The best I came up with was:
> In open:
> Â Â Â Âif (atomic_inc_return(&tgt_open_cnt) != 1)
> Â Â Â Â Â Â Â Âreturn -EBUSY;
>
> Then in release (since its the last close):
> Â Â Â Âatomic_set(&tgt_open_cnt, 0);
>
> There's still a hole that this might overflow, and I don't see
> the best way to fix that without test-and-set or compare-and-swap.
> We can't just decrement it since the last close will clear it.
Great operation, thanks.
A good lesson already offered by Wilcox, you see Joe, clearing only
necessary when the final closing.
>
> So the best thing would be to use your
> version but protect it with the tx_ring.tr_lock.
> I would rename tgt_open_cnt to just tgt_busy,
> and make it a u8 since it will be 1 or 0.
But u8 is not native word, and the spin_lock_irq is enough, I think. //Hillf
>
> Â Â Â Âint error = 0;
>
> Â Â Â Âspin_lock_irq(&tx_ring.tr_lock);
> Â Â Â Âif (tgt_busy)
> Â Â Â Â Â Â Â Âerror = -EBUSY;
> Â Â Â Âelse {
> Â Â Â Â Â Â Â Âtgt_busy = 1;
> Â Â Â Â Â Â Â Âtx_ring.tr_idx = 0;
> Â Â Â Â Â Â Â Ârx_ring.tr_idx = 0;
> Â Â Â Â}
> Â Â Â Âspin_unlock_irq(&tx_ring.tr_lock);
> Â Â Â Âreturn error;
>
> Then in release:
> Â Â Â Âspin_lock_irq(&tx_ring.tr_lock);
> Â Â Â Âtgt_busy = 0;
> Â Â Â Âspin_unlock_irq(&tx_ring.tr_lock);
>
>>>> +
>>>> Â Â Â tx_ring.tr_idx = rx_ring.tr_idx = 0;
>>>>
>>>> Â Â Â cycle_kernel_lock();
>>>> Â Â Â return 0;
>>>> Â}
>>>>
>>>> +static int tgt_release(struct inode *inode, struct file *file)
>>>> +{
>>>> + Â Â tgt_open_cnt--;
>>>> + Â Â return 0;
>>>> +}
>>>> +
>>>> Âstatic const struct file_operations tgt_fops = {
>>>>    .owner     Â= THIS_MODULE,
>>>>    .open      = tgt_open,
>>>>    .poll      = tgt_poll,
>>>>    .write     Â= tgt_write,
>>>>    .mmap      = tgt_mmap,
>>>> +   .release    Â= tgt_release,
>>>> Â};
>>>>
>>>> Âstatic struct miscdevice tgt_miscdev = {
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
>>>> the body of a message to majordomo@xxxxxxxxxxxxxxx
>>>> More majordomo info at Âhttp://vger.kernel.org/majordomo-info.html
>>>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
