lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:	Tue, 05 Oct 2010 13:40:51 +0900
From:	Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>
To:	kosaki.motohiro@...fujitsu.com
Cc:	linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: Is it legal to return positive value when do_execve() succeeds?

KOSAKI Motohiro wrote:
> > BAD_ADDR() macro is useless because 1) do_brk() and do_mmap() return
> > only either valid pointer or error code 2) when kernel and userland have
> > perfectly different address space (such as old 4G:4G separation), to
> > compare TASK_SIZE has no good meaning.
> > 
> > Then, this patch change it to use IS_ERR_VALUE instead (as other a lot
> > of places).

Using IS_ERR_VALUE() makes sense for me.

> Ouch, this patch is completely corrupted. please ignore it.

No problem.
I just browsed fs/binfmt_*.c for similar cases.
Line number is as of 2.6.36-rc6 .





fs/binfmt_aout.c
 46 #define BAD_ADDR(x)     ((unsigned long)(x) >= TASK_SIZE)
 47 
 48 static int set_brk(unsigned long start, unsigned long end)
 49 {
 50         start = PAGE_ALIGN(start);
 51         end = PAGE_ALIGN(end);
 52         if (end > start) {
 53                 unsigned long addr;
 54                 down_write(&current->mm->mmap_sem);
 55                 addr = do_brk(start, end - start);
 56                 up_write(&current->mm->mmap_sem);
 57                 if (BAD_ADDR(addr))
 58                         return addr;
 59         }
 60         return 0;
 61 }

Should be "if (IS_ERR_VALUE(error))" as well?


282                 down_write(&current->mm->mmap_sem);
283                 error = do_brk(text_addr & PAGE_MASK, map_size);
284                 up_write(&current->mm->mmap_sem);
285                 if (error != (text_addr & PAGE_MASK)) {

Should be "if (IS_ERR_VALUE(error))" as well?

286                         send_sig(SIGKILL, current, 0);
287                         return error;
288                 }


327                 down_write(&current->mm->mmap_sem);
328                 error = do_mmap(bprm->file, N_TXTADDR(ex), ex.a_text,
329                         PROT_READ | PROT_EXEC,
330                         MAP_FIXED | MAP_PRIVATE | MAP_DENYWRITE | MAP_EXECUTABLE,
331                         fd_offset);
332                 up_write(&current->mm->mmap_sem);
333 
334                 if (error != N_TXTADDR(ex)) {

Should be "if (IS_ERR_VALUE(error))" as well?

335                         send_sig(SIGKILL, current, 0);
336                         return error;
337                 }
338 
339                 down_write(&current->mm->mmap_sem);
340                 error = do_mmap(bprm->file, N_DATADDR(ex), ex.a_data,
341                                 PROT_READ | PROT_WRITE | PROT_EXEC,
342                                 MAP_FIXED | MAP_PRIVATE | MAP_DENYWRITE | MAP_EXECUTABLE,
343                                 fd_offset + ex.a_text);
344                 up_write(&current->mm->mmap_sem);
345                 if (error != N_DATADDR(ex)) {

Should be "if (IS_ERR_VALUE(error))" as well?

346                         send_sig(SIGKILL, current, 0);
347                         return error;
348                 }





fs/binfmt_elf_fdpic.c
416         install_exec_creds(bprm);
417         current->flags &= ~PF_FORKNOEXEC;
418         if (create_elf_fdpic_tables(bprm, current->mm,
419                                     &exec_params, &interp_params) < 0)
420                 goto error_kill;
421 

Why retval == 0 here and retval < 0 elsewhere in this function when
the current process is to be killed?


459 
460         /* unrecoverable error - kill the process */
461 error_kill:
462         send_sig(SIGSEGV, current, 0);

Why not to SIGKILL rather than SIGSEGV?

463         goto error;
464 
465 }





fs/binfmt_flat.c
280         while ((ret = zlib_inflate(&strm, Z_NO_FLUSH)) == Z_OK) {
281                 ret = bprm->file->f_op->read(bprm->file, buf, LBUFSIZE, &fpos);
282                 if (ret <= 0)
283                         break;
284                 len -= ret;
285 
286                 strm.next_in = buf;
287                 strm.avail_in = ret;
288                 strm.total_in = 0;
289         }
290 
291         if (ret < 0) {
292                 DBG_FLT("binfmt_flat: decompression failed (%d), %s\n",
293                         ret, strm.msg);
294                 goto out_zlib;
295         }
296 
297         retval = 0;

zlib_inflate() may return positive return code.
Is it OK to ignore partial inflation?


372 
373 failed:
374         printk(", killing %s!\n", current->comm);
375         send_sig(SIGSEGV, current, 0);

Why not to SIGKILL rather than SIGSEGV?

376 
377         return RELOC_FAILED;
378 }


582 #ifdef CONFIG_BINFMT_ZFLAT
583                 if (flags & FLAT_FLAG_GZDATA) {
584                         result = decompress_exec(bprm, fpos, (char *) datapos, 
585                                                  data_len + (relocs * sizeof(unsigned long)), 0);
586                 } else
587 #endif
588                 {
589                         result = bprm->file->f_op->read(bprm->file, (char *) datapos,
590                                         data_len + (relocs * sizeof(unsigned long)), &fpos);
591                 }
592                 if (IS_ERR_VALUE(result)) {

Is it OK to ignore partial read/inflation?


634                 if (flags & FLAT_FLAG_GZIP) {
635                         result = decompress_exec(bprm, sizeof (struct flat_hdr),
636                                          (((char *) textpos) + sizeof (struct flat_hdr)),
637                                          (text_len + data_len + (relocs * sizeof(unsigned long))
638                                                   - sizeof (struct flat_hdr)),
639                                          0);

Why not to check inflation failure before memmove()?

640                         memmove((void *) datapos, (void *) realdatastart,
641                                         data_len + (relocs * sizeof(unsigned long)));


642                 } else if (flags & FLAT_FLAG_GZDATA) {
643                         fpos = 0;
644                         result = bprm->file->f_op->read(bprm->file,
645                                         (char *) textpos, text_len, &fpos);

Why not to check partial read?

646                         if (!IS_ERR_VALUE(result))
647                                 result = decompress_exec(bprm, text_len, (char *) datapos,
648                                                  data_len + (relocs * sizeof(unsigned long)), 0);
649                 }
650                 else
651 #endif
652                 {
653                         fpos = 0;
654                         result = bprm->file->f_op->read(bprm->file,
655                                         (char *) textpos, text_len, &fpos);

Why not to check partial read?

656                         if (!IS_ERR_VALUE(result)) {
657                                 fpos = ntohl(hdr->data_start);
658                                 result = bprm->file->f_op->read(bprm->file, (char *) datapos,
659                                         data_len + (relocs * sizeof(unsigned long)), &fpos);
660                         }
661                 }

Why not to check partial read/inflation?

662                 if (IS_ERR_VALUE(result)) {
663                         printk("Unable to read code+data+bss, errno %d\n",(int)-result);
664                         do_munmap(current->mm, textpos, text_len + data_len + extra +
665                                 MAX_SHARED_LIBS * sizeof(unsigned long));
666                         ret = result;
667                         goto err;
668                 }





fs/binfmt_som.c
150         down_write(&current->mm->mmap_sem);
151         retval = do_mmap(file, code_start, code_size, prot,
152                         flags, SOM_PAGESTART(hpuxhdr->exec_tfile));
153         up_write(&current->mm->mmap_sem);
154         if (retval < 0 && retval > -1024)
155                 goto out;

What is 1024? Why not to IS_ERR_VALUE()?





By the way, I though it would be nice to have

  do_mmap_current()
  do_brk_current()

because there are a lot of

  down_write(&current->mm->mmap_sem);
  error = do_mmap();
  up_write(&current->mm->mmap_sem);

  down_write(&current->mm->mmap_sem);
  error = do_brk();
  up_write(&current->mm->mmap_sem);

repetition.





Also, it would be nice to have

  bool kill_self_if_error(int error)
  {
  	if (!IS_ERR_VALUE(error))
  		return false;
  	send_sig(SIGKILL, current, 0);
  	return true;
  }

for avoiding

  send_sig(SIGKILL, current, 0);

many times.

Regards.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists